Skip to content

WIP: Add partials to support circular references #36

Closed
alganet wants to merge 1 commit into
mainfrom
partials
Closed

WIP: Add partials to support circular references #36
alganet wants to merge 1 commit into
mainfrom
partials

Conversation

@alganet

@alganet alganet commented Dec 18, 2025

Copy link
Copy Markdown
Owner

⚠️ WORK IN PROGRESS ⚠️

  • Implements allow_partial opt-in feature that permits specs with circular dependencies to be constructed.

  • Move circular dependency detection when allow_partial=false to WiringBase constructor.

  • Initial PoC

  • Remove and stash unrelated tests added just to reach 95% coverage

  • Reach 95% coverage by covering the new features

  • Refactoring (will be dealt with in a separate PR)

  • Docs (will be dealt with in a separate PR)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This work-in-progress PR introduces support for partial construction to handle circular dependencies in the wiring system. The implementation allows objects with circular references to be instantiated by creating skeleton placeholders that are later finalized.

Key Changes:

  • New allow_partial parameter enables opt-in partial construction for circular dependencies
  • Skeleton placeholder mechanism using _apywire_partial, _apywire_event, and _apywire_failure attributes
  • Early cycle detection during container initialization using topological sorting (Kahn's algorithm)

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 24 comments.

Show a summary per file
File Description
apywire/partial.py New module implementing compiled partial construction helper
apywire/runtime.py Adds skeleton creation, finalization, and waiting logic for runtime partial construction
apywire/wiring.py Implements early cycle detection with unified dependency graph analysis
apywire/compiler.py Integrates partial construction support into compiled code generation
apywire/threads.py Updates cache checking to wait for partial skeletons to finalize
apywire/exceptions.py Adds new exception types for partial construction errors
tests/test_partial_helper.py Comprehensive tests for partial construction helper logic
tests/test_graph_detection.py Tests for cycle detection in dependency graphs
tests/test_compile_partial.py Tests for compiled partial construction behavior
tests/test_edge_cases.py Integration tests demonstrating skeleton identity preservation
tests/test_threading.py Updates threading tests for new cycle detection behavior
tests/test_single.py Updates single-threaded tests for early cycle detection
tests/test_constant_placeholders.py Updates constant placeholder cycle detection tests
tests/test_cli.py Additional CLI error handling tests
apywire/__main__.py Adds pragma comment to CLI entrypoint
Makefile Removes reuse dependency from format target
uv.lock.license Adds license header for lock file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apywire/runtime.py
Comment on lines +380 to +394
setattr(cls, "__new__", _tmp_new)
try:
ret = constructor(*pos_args, **kwargs_dict)
finally:
# Restore original __new__
if orig_new is None:
try:
delattr(cls, "__new__")
except Exception:
pass
else:
try:
setattr(cls, "__new__", orig_new)
except Exception:
pass

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temporary override of cls.__new__ (lines 380-394) is not thread-safe. In multi-threaded environments, if two threads are simultaneously resolving the same circular dependency, they could both attempt to modify cls.__new__ at the same time, leading to race conditions. One thread might restore the original __new__ while another thread is still using the temporary version, causing unpredictable behavior.

Copilot uses AI. Check for mistakes.
Comment thread apywire/partial.py
Comment on lines +121 to +134
setattr(expected_cls, "__new__", _tmp_new)
try:
ret: object = constructor_callable()
finally:
if orig_new is None:
try:
delattr(expected_cls, "__new__")
except Exception: # pragma: no cover - defensive
pass
else:
try:
setattr(expected_cls, "__new__", orig_new)
except Exception:
pass

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temporary override of expected_cls.__new__ is not thread-safe. Multiple threads could simultaneously modify this class attribute, leading to race conditions where one thread's temporary __new__ is used by another thread, or where the restoration happens at the wrong time. This is a critical issue in multi-threaded environments.

Copilot uses AI. Check for mistakes.
Comment thread apywire/runtime.py
Comment on lines +414 to +416
exc = PartialConstructionError(
"failed to initialize skeleton for %r: %s"
% (name, e)

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format string uses old-style % formatting instead of f-strings, which is inconsistent with the f-string formatting used on line 437 in the same function. For consistency and modern Python best practices, this should use f-string formatting.

Copilot uses AI. Check for mistakes.
Comment thread apywire/wiring.py
Comment on lines +418 to 419
# `_find_wiredref_names` removed — unused.

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on line 418 states "_find_wiredref_names removed — unused" but there is no corresponding removal in the diff. This comment appears to be inaccurate or misleading since the method being referenced doesn't appear to exist in the shown diff context.

Suggested change
# `_find_wiredref_names` removed — unused.

Copilot uses AI. Check for mistakes.
Comment thread apywire/wiring.py
Comment on lines +522 to +523
# Duplicate `_WiredRef` definition removed — the class is defined above near
# the top of the module to keep related logic together.

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on line 522-523 states "Duplicate _WiredRef definition removed" but there is no corresponding removal in the diff. This comment appears to be documenting code changes that are not visible in this diff, which could be confusing for reviewers.

Suggested change
# Duplicate `_WiredRef` definition removed — the class is defined above near
# the top of the module to keep related logic together.
# `_WiredRef` is defined near the top of the module to keep related logic together.

Copilot uses AI. Check for mistakes.
Comment thread apywire/partial.py
else:
try:
setattr(expected_cls, "__new__", orig_new)
except Exception:

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change
except Exception:
except Exception: # pragma: no cover - defensive
# Best-effort cleanup: ignore failure to restore __new__

Copilot uses AI. Check for mistakes.
Comment thread apywire/runtime.py
Comment on lines +388 to +393
except Exception:
pass
else:
try:
setattr(cls, "__new__", orig_new)
except Exception:

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change
except Exception:
pass
else:
try:
setattr(cls, "__new__", orig_new)
except Exception:
except Exception:
# Best-effort cleanup: failure to remove __new__ is non-fatal
pass
else:
try:
setattr(cls, "__new__", orig_new)
except Exception:
# Best-effort cleanup: failure to restore __new__ is non-fatal

Copilot uses AI. Check for mistakes.
Comment thread apywire/runtime.py
Comment on lines +388 to +393
except Exception:
pass
else:
try:
setattr(cls, "__new__", orig_new)
except Exception:

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change
except Exception:
pass
else:
try:
setattr(cls, "__new__", orig_new)
except Exception:
except Exception:
# Best-effort cleanup: if we cannot delete the temporary
# __new__ attribute, ignore the error to avoid masking
# the original construction exception.
pass
else:
try:
setattr(cls, "__new__", orig_new)
except Exception:
# Best-effort cleanup: if we cannot restore the original
# __new__ method, ignore the error to avoid masking the
# original construction exception.

Copilot uses AI. Check for mistakes.
Comment thread apywire/partial.py
if orig_new is None:
try:
delattr(expected_cls, "__new__")
except Exception:

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change
except Exception:
except Exception: # Defensive: best-effort cleanup; ignore failures restoring __new__

Copilot uses AI. Check for mistakes.
Comment thread apywire/partial.py
else:
try:
setattr(expected_cls, "__new__", orig_new)
except Exception:

Copilot AI Dec 19, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change
except Exception:
except Exception:
# Best-effort restoration of __new__; if this fails,
# we keep the temporary __new__ and still propagate
# the original construction error to the caller.

Copilot uses AI. Check for mistakes.
@alganet

alganet commented Dec 19, 2025

Copy link
Copy Markdown
Owner Author

The __new__ thread unsafety caught me by surprise. While it could be avoided somehow, I think it's better to look for other way to support partials.

Also, this PR got too large too fast. I'll make another one that just focuses on the circular dependency check on the WiringBase constructor.

@alganet alganet closed this Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants