Skip to content

Wkt/pr434 review#438

Open
goodboy wants to merge 7 commits intomainfrom
wkt/pr434_review
Open

Wkt/pr434 review#438
goodboy wants to merge 7 commits intomainfrom
wkt/pr434_review

Conversation

@goodboy
Copy link
Copy Markdown
Owner

@goodboy goodboy commented Apr 9, 2026

Clone of #434 which review adjustments added on and should test CI.

Prolly should either PR this into the fork for that PR or have the author cherry the needed commits (after rebase to main ofc).

mahmoudhas and others added 7 commits April 6, 2026 05:51
Let actor callers skip replaying the parent __main__ during child startup so downstream integrations can avoid inheriting incompatible bootstrap state without changing the default spawn behavior.
Keep trio child bootstrap data in the spawn handshake instead of stashing it on Actor state so the replay opt-out stays explicit and avoids stale-looking runtime fields.
Keep actor-owned parent-main capture and let `_mp_figure_out_main()` decide whether to return `__main__` bootstrap data, avoiding the extra SpawnSpec plumbing while preserving the per-actor flag.
Use `inherit_parent_main` across the actor APIs and helper to better describe the behavior, and restore the reviewer note at child bootstrap where the inherited `__main__` data is copied from `SpawnSpec`.
Clean up mutable defaults, give parent-main bootstrap data a named type, and add direct start_actor coverage so the opt-out change is clearer to review.
Move the subprocess probe into dedicated spawn test support files so the inheritance tests cover the real __main__ replay path without monkeypatching or inline script strings.
Replace the subproc-based test harness with inline
`tractor.open_nursery()` calls that directly check
`actor._parent_main_data` instead of comparing
`__main__.__name__` across a process boundary
(which is a no-op under pytest bc the parent
`__main__` is `pytest.__main__`).

Deats,
- delete `tests/spawn_test_support/` pkg (3 files)
- add `check_parent_main_inheritance()` helper fn
  that asserts on `_parent_main_data` emptiness
- rewrite both `run_in_actor` and `start_actor`
  parent-main tests as inline async fns
- drop `tmp_path` fixture and unused imports

Review: PR #434 (goodboy, Copilot)
#434

(this patch was generated in some part by [`claude-code`][claude-code-gh])
[claude-code-gh]: https://github.com/anthropics/claude-code
Copilot AI review requested due to automatic review settings April 9, 2026 23:19
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a per-actor opt-out for inheriting the parent process’ __main__ bootstrap data when using Tractor’s trio spawn backend, aligning behavior with multiprocessing-style main fixup while allowing isolation when needed.

Changes:

  • Introduce inherit_parent_main flag on ActorNursery.start_actor() and ActorNursery.run_in_actor(), threaded into Actor construction.
  • Localize main-module inheritance behavior in _mp_fixup_main._mp_figure_out_main(inherit_parent_main=...) and type it via a TypedDict.
  • Add trio-backend regression tests covering inherited vs. non-inherited parent __main__ bootstrap data.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tractor/spawn/_mp_fixup_main.py Adds ParentMainData typing and supports skipping parent __main__ capture when requested.
tractor/runtime/_supervise.py Exposes inherit_parent_main through the public nursery spawn APIs and avoids mutable default args.
tractor/runtime/_runtime.py Threads the new flag into parent main data capture and refines typing around _parent_main_data/enable_modules.
tests/test_spawning.py Adds trio-only tests validating correct propagation of the opt-out flag to child actors.

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

Comment on lines +211 to +212
bootstrap module instead of re-running the parent's ``__main__``
during startup.
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

The docstring implies inherit_parent_main=False always prevents re-running the parent’s __main__, but the implementation only changes Tractor’s trio spawn fixup (Actor.load_modules() guards on self._spawn_method == 'trio'). For multiprocessing spawn/forkserver, the stdlib bootstrap will still reconstruct __main__. Please clarify in the docstring that this flag only affects the trio spawn backend (or explicitly document it as a no-op for mp backends).

Suggested change
bootstrap module instead of re-running the parent's ``__main__``
during startup.
bootstrap module for Tractor's trio spawn backend instead of
applying Tractor's parent ``__main__`` re-exec fixup during
startup. This does not prevent ``multiprocessing`` ``spawn`` or
``forkserver`` from reconstructing the parent's ``__main__`` as
part of their normal stdlib bootstrap.

Copilot uses AI. Check for mistakes.
@goodboy
Copy link
Copy Markdown
Owner Author

goodboy commented Apr 10, 2026

gd, why is the double CI thing happening..

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.

3 participants