fix: run of circular-dependencies with python3.15#7396
Conversation
|
✅ Review complete (commit 7cb0c29) |
|
![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 (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
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.
|
pls consider adeb8dd |
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`
…ess dep
get_context("fork") raises ValueError on platforms without fork (Windows),
so guard it and run handle_module2 serially there. Also remove the now-unused
multiprocess pip package, whose only consumer was this script.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
knst
force-pushed
the
fix-multiprocess-3.15
branch
from
fa97d7a to
7cb0c29
Compare
There was a problem hiding this comment.
Code Review
Small, well-scoped dev-tool fix replacing the third-party multiprocess package with stdlib multiprocessing to unblock Python 3.15 (dill's 3.15 support is broken due to co_lnotab removal). Second commit adds a serial-map fallback for platforms without fork (Windows) and drops the now-unused pip package from the slim CI image. Both changes are correct: handle_module2 and its deps/files globals are hoisted to module scope so fork workers inherit the populated dependency graph, and the serial path runs in the same process where the globals are already populated. No carried-forward findings from the prior 0fe8e05 review (which was clean) and no new blocking or suggestion-level issues in the latest delta.
Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5[high] (failed: initialize stall) for general + dash-core-commit-history; verifier opus/opus.
|
Thanks. The current PR head |
3 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: