fix(bootstrap): warm the resolver cache before adding top-level nodes - #1336
andre-motta wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe bootstrapper now pre-warms resolver results for all top-level requirements before graph insertion. It passes the top-level request type and multiple-version setting to Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The top-level resolution fix maintains consistent graph state regardless of the pinned and unpinned requirement order. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
c2b5d6a to
66e7e76
Compare
Top-level requirements were resolved and recorded in the dependency graph one rule at a time. A later rule for the same package, such as an exact pin that bypasses the release-age cooldown, widens the resolver's shared per-package cache, so the Resolve phase then returned a different version than the one already written to the graph. Because Start skips graph insertion for top-level requirements, the stale node stayed in graph.json with no build-system edges and build-parallel scheduled it in round 1 before its build backend was available. Resolve every top-level rule once before any of them is added to the graph, so the recorded version matches what the Resolve phase uses. Closes python-wheel-build#1334 Co-Authored-By: Claude Opus <noreply@anthropic.com> Signed-off-by: Andre Lustosa <alustosa@redhat.com>
66e7e76 to
64ea41d
Compare
andre-motta
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
This looks mostly good. I have left a comment about exception handling.
Thank you for the fix!
| # exact pin that bypasses the release-age cooldown) changes what an | ||
| # earlier rule resolves to. Errors are reported by | ||
| # _resolve_and_add_top_level below. | ||
| for req in requirements: |
There was a problem hiding this comment.
The warm-up loop resolves every top-level requirement with req_type=TOP_LEVEL, which triggers cooldown bypass logic for == pins (via resolve_package_cooldown). However, the warm-up swallows all
exceptions with a bare except Exception: pass, including ValueError from PEP 508 direct-reference URLs (req.url). While _resolve_and_add_top_level handles these errors properly on the second call, the silent swallow means the warm-up could mask unexpected errors during debugging.
More importantly, the warm-up currently doesn't set the requirement_ctxvar context, so any
log messages emitted during warm-up resolution (e.g. 'cooldown bypassed as the top-level requirement uses == pin') will lack the package name context that the main loop provides via requirement_ctxvar.set(req). This inconsistency means duplicate log lines: once without context during warm-up, once with context during _resolve_and_add_top_level
Suggestion:
Set requirement_ctxvar around each warm-up call to maintain consistent log context, and
consider logging a debug message when catching the exception so silent failures are traceable:
2 participants
Fixes #1334.
Top-level requirements were resolved and written to the dependency graph one rule at a time. A later rule for the same package (an exact
==pin that bypasses the release-age cooldown) widens the resolver's shared per-package cache, so theResolvephase settled on a different version than the one already in the graph, andStartskips graph insertion for top-level requirements. The stale node stayed in graph.json withedges: []andbuild-parallelscheduled it in round 1.Fix: resolve every top-level rule once before any of them is added to the graph, so the recorded version matches what the
Resolvephase uses. The extra pass only hits the resolver cache on the second lookup, and errors are still reported by the existing code path.Tests: regression for the unpinned-then-pinned case (fails on main with
{'pkg==0.39.0', 'pkg==0.39.1'} == {'pkg==0.39.1'}) and a multiple-versions guard.pytest: 952 passed, 3 skipped.ruff,mypyclean.🤖 Generated with Claude Code