Repository navigation
ITF: route line-prefixed assertions to the mismatch guidance; name the runner key - #215
Merged
Merged
Conversation
…e runner key
The CI dataset's "(line N) AssertionError" traces leave terminal_exc "other", so
expectation groups got the crash block. routing_exc reads the exception from that
prefix for instruction_addendum and dispatch_category. _MISMATCH_GUIDANCE now names
("cuda", 8) as the only key to write.
tarekziade
added a commit
to huggingface/serge
that referenced
this pull request
Oct 11, 2026
serge's expectation updates mostly don't have the shape maintainers
accept. Of the serge PRs opened in Sept–Oct 2026, the 3 value updates
that touched only the CI runner's `("cuda", 8)` entry all merged cleanly
(huggingface/transformers#48515, #49068, #49076). The one that overwrote
`("cuda", None)` merged with a wrong value (#48580). The open queue
overwrote `(None, None)` (#47616, #49480), rewrote XPU values with no
XPU evidence (#48955), loosened tolerances (#49143) and edited plain
literals in place (#47840). The prompt already asked for a device key.
This PR enforces it.
**`reviewbot/expectation_keys.py`** parses each changed test file before
and after the patch (`ast`), maps every `Expectations({...})` block key
→ value, and reports:
- a change to, or removal of, any key other than the runner's `("cuda",
8)` / `("cuda", (8, 6))` (A10G, compute capability 8.6). Demotion is
allowed: an existing value moved unchanged to `(None, None)`, which is
what #48198 did;
- an added key other than the runner's, or a `(None, None)` value that
isn't an existing one;
- a runner value moving by more than 5% of the block's largest
magnitude. Accepted drift was 0.4% (#48515) and 3.6% (#49068, bf16 near
zero); the pvt groups that should have been refused moved 18% and 45%;
- a loosened `atol`/`rtol`, in any patch;
- in an expectation-only patch, an expected value or assertion changed
outside any `Expectations` block. Test-code fixes are left alone (#47697
renamed a config key).
**Wiring.** `_validate_patch` runs it after the patch applies, before
the brevity pass and the normalizer. A violation resets the worktree and
comes back as correction feedback (rule text included), using the same
`TASK_NORMALIZE_MAX_RETRIES` budget. If a violation survives the budget,
the PR still opens, with a "⚠️ Expectation change not in the maintainer
shape" section listing the problems. `TASK_EXPECTATION_KEY_CHECK=0`
turns the check off.
**Checked against real PRs.** Every serge PR since 2026-07-20, comparing
each test file at its base and head commits:
- all 4 PRs I'd call auto-merge-eligible pass (#48838, #48882, #48945,
#49040)
- 7 of 8 red-flag PRs are flagged (#47698 changes the assertion's
structure, not a key)
- all 3 "right change, wrong key" PRs are flagged, with a fix the model
can apply
- the rule is stricter than maintainers on 5 merged PRs (plain-literal
text edits, broad keys). serge can always meet it by converting to
`Expectations` with the runner key.
The prompt side (the guidance now names the runner key, and a routing
bug kept it from reaching most assertion groups) is in
huggingface/transformers-ci#215.
Tests: `tests/test_expectation_keys.py` (17 cases cut down from the PRs
above) and `ExpectationShapeValidationTests` in `tests/test_tasks.py`.
Full suite: 1390 passed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes so serge writes test expectations the way maintainers accept them.
1. Routing bug: most assertion groups never received
_MISMATCH_GUIDANCE. The CI dataset writes every failure as(line 130) AssertionError: …, andterminal_exception()doesn't parse that prefix. So in production every group'sterminal_excis"other": all 376 integration failures on 2026-10-10, 222 of them AssertionErrors.instruction_addendumalready routes anothergroup whoseterminal_excisAssertionErrorto the mismatch block, but that path never fired. Those groups got_CRASH_GUIDANCE("treat it as a library bug, do not edit the test") and serge edited the expectations anyway, with no rule about which key to write: #49336, #48955, #49480, #49142, all labelledother. Thebig-bird-expectationfixture inutils/prompt_ab.pyhard-codesterminal_exc: "AssertionError", which hid this.routing_exc(target)reads the exception from the(line N)prefix whenterminal_excwasn't parsed, and is used byinstruction_addendumanddispatch_category. Grouping and fingerprints are unchanged. Fixingterminal_exceptionitself would re-key every open group, so that's left as a separate change.utils/prompt_ab.py --group ministral3-line-prefixed-assertion --before main(a new fixture shaped the way the triage really builds it): addendum 1,956 → 5,492 chars, crash block → mismatch block.big-bird-expectationandflex-olmo-driftgo 5,031 → 5,492 (the new key rule).phimoe-conversionis unchanged.2.
_MISMATCH_GUIDANCEnames the runner key. "Add a device key" becomes: write only("cuda", 8)(or("cuda", (8, 6))where the block keys by (major, minor)), because the runners are A10G. Never overwrite(None, None),("cuda", None), another CUDA generation,"xpu"or"rocm". The #48198 demotion is the one allowed move, and a plain literal is converted with the new value under the runner key. This matches what maintainers merged: of serge's Sept–Oct PRs, the 3 value updates on("cuda", 8)merged cleanly, and the one that overwrote("cuda", None)merged with a wrong value (#48580). huggingface/serge#153 enforces the same rule mechanically.Merging is shipping (the nightly installs
@main). This doesn't depend on serge#153 being deployed. Without it the guidance's "serge checks this mechanically" just isn't true yet.Not done: a local replay with
run-local-task-prompt.py(it costs a full LLM run per arm). Tests: 4 newInstructionAddendumTestcases. Full suite: 1141 passed.🤖 Generated with Claude Code