fix: run of circular-dependencies with python3.15#7396
Conversation
Multiprocess uses dill to pickle the nested handle_module2 closure and dill's Python 3.15 support is broken (co_lnotab was removed from code objects). Review hint: use `git show -w --color-moved=dimmed-zebra`
|
✅ Review complete (commit 0fe8e05) |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fe8e0553b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| with Pool(8) as pool: | ||
| # Use fork so workers inherit the populated `deps` global without | ||
| # having to pickle it for every task. | ||
| with multiprocessing.get_context("fork").Pool(8) as pool: |
There was a problem hiding this comment.
When this devtool is run with a Windows Python or any other platform that lacks the fork start method, this line raises before any dependency checking runs, which also breaks test/lint/lint-circular-dependencies.py for those environments. The previous multiprocess.Pool path could serialize the nested closure under spawn, so this Python 3.15 fix regresses non-fork platforms; consider passing/initializing deps for workers instead of hard-coding fork.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe script contrib/devtools/circular-dependencies.py was refactored to use Python's multiprocessing module instead of multiprocess, explicitly creating the worker pool with a "fork" context. The files and deps variables, along with the handle_module2 function, were moved from the main block to module scope so that forked worker processes inherit the populated deps mapping rather than requiring it to be pickled per task. The main block now only handles argument iteration and invocation of handle_module. Estimated code review effort: 2 (Simple) | ~10 minutes Related PRs: None identified. Suggested labels: contrib, devtools, refactor Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small dev-tool fix replacing third-party multiprocess (broken on Python 3.15 due to dill/co_lnotab) with stdlib multiprocessing using an explicit fork context, hoisting handle_module2/files/deps to module scope. Behavior is preserved — the pool is recreated inside the outer loop, so each forked worker set inherits an up-to-date deps. Affects only contrib/devtools/circular-dependencies.py; no consensus or runtime code touched.
2 participants
Issue being fixed or feature implemented
Multiprocess uses dill to pickle the nested handle_module2 closure and dill's Python 3.15 support is broken (co_lnotab was removed from code objects).
What was done?
Moved some functions and variables to global namespace.
How Has This Been Tested?
Run
test/lint/lint-circular-dependencies.pywith python3.15Review hint: use
git show -w --color-moved=dimmed-zebraBreaking Changes
Please describe any breaking changes your code introduces
Checklist: