Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses inconsistent behavior caused by bidirectional module dependencies by removing support for such relationships. The core change is to the module initialization order in ProfileLoader, which prevents modules from being marked as initialized before their subsets are fully processed, thereby avoiding the scenario where one module uses another before it's fully initialized. However, the PR removes cycle detection mechanisms without adding validation to prevent cycles from being created, which introduces potential infinite recursion issues.
Key Changes
- Modified
ProfileLoader.__init_subsets_forto mark modules as initialized after (rather than before) processing their subsets, preventing inconsistent state with bidirectional dependencies - Removed duplicate event detection from
ModuleEventProxyand simplifiedModule.on_eventto always return a context manager - Removed cycle detection from
Module._iter_brokers, simplifying the iteration logic - Removed tests that verified bidirectional module relationships worked correctly
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/loaders/test_profile_loader.py | Removed test for bidirectional profile loading to align with no longer supporting bidirectional dependencies |
| tests/core/test_module.py | Removed test for bidirectional module usage to align with no longer supporting circular module relationships |
| injection/loaders.py | Changed initialization order to mark modules as initialized after subset initialization, preventing inconsistent state but introducing potential for infinite recursion with circular dependencies |
| injection/_core/module.py | Removed cycle detection from broker iteration and event duplicate checking, simplifying code but creating infinite recursion risk if bidirectional relationships are created |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _iter_brokers(self) -> Iterator[Broker]: | ||
| for module in self.__modules: | ||
| yield from module._iter_brokers(visited) | ||
| yield from module._iter_brokers() |
There was a problem hiding this comment.
Removing cycle detection from _iter_brokers() introduces a risk of infinite recursion. If bidirectional module relationships exist (module A uses module B, and module B uses module A), this method will infinitely recurse when called by unsafe_unlocking() or all_ready().
Consider adding validation in the use() method to detect and prevent cycles when modules are added, or restore the cycle detection mechanism here.
| def __init_subsets_for(self, module: Module) -> Module: | ||
| if not self.__is_empty and not self.__is_initialized(module): | ||
| self.__mark_initialized(module) | ||
| target_modules = tuple( | ||
| self.__init_subsets_for(mod(name)) | ||
| for name in self.module_subsets.get(module.name, ()) | ||
| ) | ||
| module.init_modules(*target_modules) | ||
| self.__mark_initialized(module) |
There was a problem hiding this comment.
Moving __mark_initialized(module) after module.init_modules() prevents the inconsistent behavior that occurred when bidirectional dependencies existed (where module B would use module A before A's initialization was complete). However, this change means that circular dependencies in the module_subsets configuration will now cause infinite recursion instead of producing inconsistent state.
While failing fast is better than inconsistent behavior, it would be more user-friendly to detect circular dependencies explicitly and raise a clear error message (e.g., CircularDependencyError) rather than allowing a stack overflow. Consider adding cycle detection that raises a descriptive error when a circular dependency is detected.
| def on_event(self, event: Event, /) -> ContextManager[None]: | ||
| self_event = ModuleEventProxy(self, event) | ||
|
|
||
| if self_event.is_duplicate: | ||
| return None | ||
|
|
||
| return self.dispatch(self_event) |
There was a problem hiding this comment.
The removal of the duplicate event check changes the behavior of event propagation. Previously, is_duplicate prevented the same event from being processed multiple times in circular module relationships. Now, without this check, if modules have bidirectional relationships, events could propagate indefinitely in a loop.
Since the PR removes support for bidirectional module relationships, this change is only safe if validation is added elsewhere to prevent such relationships from being created. Without that validation, this could lead to infinite event propagation.
No description provided.