Skip to content

Deduplicate embedded cause suffix in _format_exception#1262

Open
raravind007 wants to merge 1 commit into
python-wheel-build:mainfrom
raravind007:fix-1243-format-exception-dedup
Open

Deduplicate embedded cause suffix in _format_exception#1262
raravind007 wants to merge 1 commit into
python-wheel-build:mainfrom
raravind007:fix-1243-format-exception-dedup

Conversation

@raravind007

Copy link
Copy Markdown
Contributor

Summary

Fixes #1243.

_format_exception() always appends " because {cause}" for any
chained exception. resolver.py:285-288 wraps resolution failures as
f"Unable to resolve requirement specifier ... using {provider}: {original_msg}"
where original_msg is str(err), then chains from err - so the
cause's own message is already embedded at the end of the outer
message before _format_exception() ever sees it, producing a
duplicated, hard-to-read error:

Unable to resolve requirement specifier flashinfer-python==0.6.8.post1
with constraint flashinfer-python==0.6.11.post2 using PyPI resolver:
found no match for flashinfer-python==0.6.8.post1 because found no
match for flashinfer-python==0.6.8.post1

While reproducing the issue against current resolver.py, I found that
the cause text is appended as f"...: {original_msg}", so the concrete
relationship is a suffix rather than a prefix. This uses
endswith(f": {cause_str}") to match that exact construction and avoid
coincidental suffix matches - a bare endswith(cause_str) would
incorrectly dedupe something like RuntimeError("failed to match")
chained from ValueError("match").

A naive suffix-dedupe would also silently truncate a deeper cause
chain: if the immediate cause itself has its own cause, deduping
against its raw message alone and returning early loses that deeper
"because ..." information. The fix formats the cause recursively first
and preserves anything beyond the raw immediate-cause text, so a 3+
level chain isn't truncated.

Test plan

  • pytest tests/test_external_commands.py — covers the resolver-style
    duplication case, the unrelated-cause case (unaffected), the
    nested-cause preservation case, and the coincidental-suffix guard
    case, all with exact-string assertions.
  • ruff check / ruff format --check
  • mypy -p fromager — no new errors

…build#1243)

resolver.py wraps failures as f"...: {original_msg}" where
original_msg is str(err) before chaining `from err` - the cause's raw
message ends up embedded at the end of the outer message, anchored on
a ": " separator, not the start.

_format_exception() currently follows __cause__ unconditionally and
always appends " because {cause}", producing a duplicated message for
this pattern.

Skip the "because" clause when the outer message ends with
": {cause}" (the exact resolver.py construction) - a bare
endswith(cause_str) would risk a coincidental match on a short cause
(e.g. RuntimeError("failed to match") from ValueError("match")), so
the check is anchored on the separator instead.

Recursively format the cause first so a deeper cause (the immediate
cause's own cause) isn't silently dropped: deduping the immediate
cause's raw text but returning early would also discard anything the
recursive formatting adds beyond it. If the recursively formatted
cause is exactly the raw immediate-cause text, dedup fully; if it's
"{cause} because {deeper}", keep the "because {deeper}" suffix.

Signed-off-by: Reshmi Aravind <raravind@redhat.com>
@raravind007
raravind007 requested a review from a team as a code owner July 23, 2026 10:54
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

_format_exception() now formats chained exceptions using anchored cause matching. It removes an immediately duplicated embedded cause, preserves deeper nested causes, and retains the because connector for unrelated messages. Four regression tests cover embedded causes, unrelated causes, nested causes, and coincidental suffix matches.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes implement #1243 by skipping redundant chained cause text while preserving deeper cause details.
Out of Scope Changes check ✅ Passed The PR stays focused on _format_exception() and regression tests, with no unrelated code changes.
Title check ✅ Passed The title clearly summarizes the main change: deduplicating embedded cause suffixes in _format_exception.
Description check ✅ Passed The description matches the code changes and explains the bug, fix, and tests in detail.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mergify mergify Bot added the ci label Jul 23, 2026
@dhellmann

Copy link
Copy Markdown
Member

What does the error output look like if we stop trying to do the formatting and appending of the "why" chain ourselves and we just let python raise the exception normally and then show the whole traceback? And what would it look like if we use raise from to build an exception chain using simple messages?

@raravind007

Copy link
Copy Markdown
Contributor Author

Good question — I tried the concrete alternatives against the exception shapes we have today.

A. Current main after #1241

For a transitive resolution failure, _enrich_resolution_error() creates a new exception whose message includes the dependency path, and _handle_phase_error() raises it without an explicit from clause. Python keeps the original exception in __context__, so the full traceback is still available to exc_info=True; it just is not exposed to our single-line _format_exception(), which only follows __cause__.

The user-facing line is roughly:

Unable to resolve requirement specifier flashinfer-python==0.6.8.post1
with constraint flashinfer-python==0.6.11.post2 using PyPI resolver:
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)

B. Simple messages + normal raise ... from err + native traceback

If each exception adds only new information instead of embedding str(err), the native chain is much cleaner:

ResolverException: found no match for flashinfer-python==0.6.8.post1

The above exception was the direct cause of the following exception:

ResolverException: could not resolve flashinfer-python==0.6.8.post1
(constraint flashinfer-python==0.6.11.post2)

The above exception was the direct cause of the following exception:

ResolverException: could not resolve flashinfer-python==0.6.8.post1,
required by vllm==3.0 <- reward-hub==2.0 <- its-hub==1.0

That gives full file/line information at each level, but it is much more verbose for normal CLI output.

C. Simple messages + raise ... from err + the existing compact formatter

This is the interesting result:

could not resolve flashinfer-python==0.6.8.post1, required by
vllm==3.0 <- reward-hub==2.0 <- its-hub==1.0 because
could not resolve flashinfer-python==0.6.8.post1
(constraint flashinfer-python==0.6.11.post2) because
found no match for flashinfer-python==0.6.8.post1

In that shape, the original unconditional _format_exception() works without any deduplication.

That suggests the underlying duplication in #1243 is not really caused by _format_exception() itself. resolver.py currently does both of these:

original_msg = str(err)
raise ResolverException(f"...: {original_msg}") from err

so the same information exists both in the outer message and in __cause__.

A cleaner fix may therefore be to stop embedding str(err) in wrapper messages and let exception chaining carry the cause. We would still need to decide how we want to present the dependency why_snapshot — as one compact outer message, as #1241 does today, or as additional exception-chain levels.

Given that, I'm inclined to pause this _format_exception() PR rather than add dedup logic at the presentation layer. If you prefer, I can prototype the simpler-message / raise from approach and compare the final CLI output before changing the PR.

@dhellmann

Copy link
Copy Markdown
Member

Yes, I would like to see what the code changes and output look like if we stop trying to be clever and rely on more standard python implementation. Maybe we want something in the command line tool that takes a chain of exceptions and formats those as a single error message as well as showing the full set of tracebacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_format_exception() duplicates message when cause text is already embedded

2 participants