Simplify circular dependency detection#37
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors circular dependency detection in apywire to occur at initialization time rather than at runtime. The key change moves from a runtime stack-based detection mechanism to an upfront topological sort of all spec entries during Wiring() construction.
Key changes:
- Circular dependencies are now detected during
Wiring()initialization using Kahn's algorithm for topological sorting - Removed runtime circular dependency checks and the resolving stack infrastructure from both thread-safe and non-thread-safe modes
- Generator silently stops recursion when cycles are detected, deferring error handling to the runtime container
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
apywire/wiring.py |
Major refactoring: added SpecParser base class, implemented upfront topological sort for all entries, removed resolving stack logic |
apywire/runtime.py |
Removed runtime circular dependency checking (stack-based detection) from _instantiate_impl |
apywire/threads.py |
Removed resolving stack from thread-local state, keeping only lock-related state |
apywire/generator.py |
Changed from raising CircularWiringError to silently returning on cycles, inherits from SpecParser |
tests/test_threading.py |
Updated tests to expect CircularWiringError at init time instead of access time |
tests/test_single.py |
Updated tests to expect CircularWiringError at init time instead of access time |
tests/test_constant_placeholders.py |
Updated tests to expect CircularWiringError at init time instead of access time |
tests/test_generator.py |
Renamed _is_constant to _is_spec_constant, updated circular dependency test to verify silent return behavior |
tests/test_internals.py |
Removed tests for _get_resolving_stack (no longer needed) |
tests/test_edge_cases.py |
Removed resolving stack initialization tests |
docs/user-guide/circular-dependencies.md |
New guide documenting circular dependency detection behavior |
docs/user-guide/*.md |
Updated references to circular dependencies throughout documentation |
docs/api-reference.md |
Updated CircularWiringError example to show detection at init time |
Makefile |
Removed reuse dependency from format target |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with pytest.raises(CircularWiringError): | ||
| Wiring(spec) |
There was a problem hiding this comment.
The code example uses pytest.raises but doesn't import pytest. The example should either import pytest at the top or use a try/except block like the first example to maintain consistency and ensure the code is runnable as shown.
- Used topological sort for all entries, not just constants - Removed checks during instantiation, relying on instantiation-time check only. - Updated tests and documentation.
2ecb40f to
664c3e0
Compare
Description
Brief description of changes.
Type of Change
Checklist