Include dependency chain in resolution error messages#1241
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBootstrapper now appends dependency-chain context to Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 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 |
…uild#1214) Resolution failures were logged using whatever top-level package happened to be active in the requirement_ctxvar-based logging context, which can make an unrelated top-level package look like the culprit when the actual conflict is several levels down the dependency tree. _phase_resolve now appends the requirement chain (from WorkItem's existing why_snapshot) to ResolverException messages, e.g.: found no match for flashinfer-python==0.6.8.post1 ... (dependency chain: its-hub==1.0 -> reward-hub==2.0 -> vllm==3.0 -> flashinfer-python==0.6.8.post1) The wrapped exception suppresses its cause (`from None`) since the chain suffix already embeds the original message, and top-level failures with no chain propagate unchanged. Signed-off-by: Reshmi Aravind <raravind@redhat.com>
Self-review follow-up: _phase_resolve was suppressing the cause (from None) to avoid __main__._format_exception() printing the resolver message twice via "... because ...". That violated the project's documented rule to chain exceptions, and silently discarded the original traceback that debug/test-mode logging relies on. Restore `from err` and instead teach _format_exception() to skip the "because" clause when the cause's message is already a substring of the outer message - the actual duplication this PR needs to avoid. Also drop the redundant empty-string-as-boolean round trip between _dependency_chain_suffix() and its caller (check item.why_snapshot directly instead). Signed-off-by: Reshmi Aravind <raravind@redhat.com>
raravind007
force-pushed
the
fix-1214-resolver-error-dependency-chain
branch
from
6311c78 to
b8c5955
Compare
|
Ran an AI-assisted code review (self-review) against this diff before/after opening the PR, which caught a couple of real issues, now fixed in the second commit:
Happy to share the fuller review notes if useful. |
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
I don't think this fully solves #1214
There are two separate issues that need to be addressed:
- The error message body doesn't show the dependency path (PR fixes this)
The PR appends (dependency chain: its-hub==1.0 -> reward-hub==2.0 -> vllm==3.0 -> flashinfer-python==0.6.8.post1) to the ResolverException message. That helps a user trace the real path. - The its-hub: log prefix is still wrong (PR does NOT fix this)
The misleading prefix comes fromrequirement_ctxvar.
I have also added a suggestion for current PR. Can you take a look at this and wither send a second PR or change the original issue description and create a follow up card? cc @LalatenduMohanty for awareness
| # embedded in the outer exception's own message (e.g. an exception | ||
| # re-raised with extra context appended to the original text) — | ||
| # otherwise the cause would be printed a second time verbatim. | ||
| if cause and cause in exc_str: |
There was a problem hiding this comment.
This looks very fragile. A safer check would be to verify the outer message starts with the cause text, since the pattern is always f"{err}{suffix}":
if cause and exc_str.startswith(cause):
return exc_str
Or even more precise: we could check that the outer message equals cause + some suffix. The startswith variant is still general enough for any append context to original message pattern while avoiding false positives from coincidental substring matches
There was a problem hiding this comment.
Thanks for the startswith suggestion, Rohan! I applied that, but while adding a regression test using the actual nested resolver-style exception shape, I found one more issue. :)
The check should compare against the immediate cause's raw message, str(exc.__cause__), rather than the recursively formatted cause. When the cause itself has a cause, _format_exception(exc.__cause__) can already include its own " because ..." suffix, which no longer matches the prefix of the outer f"{err}{suffix}" message.
I updated the formatter to use str(exc.__cause__) for the dedupe check, while still using the recursively formatted cause when the "because" clause is actually emitted. I also added a regression test for the real resolver.py → _phase_resolve() wrapping shape. Pushed this as a separate commit so the correction is clear in the PR history.
Per review feedback from Rohan (PR python-wheel-build#1241): the dedup pattern is always f"{err}{suffix}", so `cause in exc_str` is unnecessarily loose and could false-positive if the cause's text coincidentally appears elsewhere in an unrelated outer message. `exc_str.startswith(cause)` matches the actual invariant instead. Adds a regression test proving the old substring check would have incorrectly suppressed the "because" clause for a coincidental match. Signed-off-by: Reshmi Aravind <raravind@redhat.com>
The previous `startswith` fix compared the outer exception message
against the recursively formatted cause string. That breaks when the
immediate cause has a cause of its own, because the recursively
formatted cause may already contain a `" because ..."` suffix.
That matters for the real resolver failure shape: `resolver.py` can
raise a `ResolverException` from an inner error while embedding the
inner message in its own text, and `_phase_resolve()` can then wrap
that `ResolverException` again with dependency-chain context. In that
nested case, comparing against `_format_exception(exc.__cause__)`
fails to recognize the append-context pattern and can still produce a
duplicated, hard-to-read message.
Compare against `str(exc.__cause__)` instead: the immediate cause's
raw message. That matches the actual `f"{err}{suffix}"` pattern while
still using the recursively formatted cause when the `"because"`
clause really needs to be printed.
Add a regression test using the real nested resolver-style exception
shape rather than a flat mock.
Signed-off-by: Reshmi Aravind <raravind@redhat.com>
2 participants
Summary
Fixes #1214.
Resolution failures are logged with a package-name prefix derived from
requirement_ctxvar, which reflects whatever package happens to be activein the logging context when the exception is finally reported — not
necessarily the package whose requirement actually conflicted. When a
transitive dependency several levels deep fails to resolve (e.g.
its-hub -> reward-hub -> vllm -> flashinfer-python), the error messagecorrectly names the failing package (
flashinfer-python) but the logprefix can show an unrelated top-level package (
its-hub), making it looklike
its-hubis the culprit when the real conflict originates withvllm's pin onflashinfer-python.Bootstrapperalready tracks the full dependency chain for eachWorkItemviawhy_snapshot. This adds_dependency_chain_suffix(),which turns that chain into a
(dependency chain: its-hub==1.0 -> reward-hub==2.0 -> vllm==3.0 -> flashinfer-python==0.6.8.post1)suffix, appended to anyResolverExceptionraised during dependency resolution in_phase_resolve()— regardless of what the log prefix happens to show.resolution failures (no chain) propagate unchanged (verified by
exception identity in tests).
from None) since thechain suffix already embeds the original message — chaining it would
make
__main__._format_exception()print the resolver message twicevia
"... because ...".Test plan
pytest tests/test_bootstrapper_iterative.py— added threeregression tests: chain-suffix message content, no-duplicate-cause
behavior (verified through the real
__main__._format_exception),and identity-preservation for the no-chain path.
pytest tests/test_resolver.pyruff check/ruff format --checkmypy -p fromager— no new errors introduced