Repository navigation
test(shape-inference): make the catalog pin say which side is authoritative (salvages #1873) - #1882
Conversation
…tative Salvaged from #1873, which was closed unmerged after #1870 fixed the same red by a different route. The count fix landed; this improvement did not, and it is the more durable half. When the pin went stale it blocked `Fast (Linux x86_64)` -- a required check -- on main and on every open PR for 80 minutes, and all it said was: assertion `left == right` failed left: 221 right: 220 Nothing there says which number is the live registry and which is the pin, so the reader cannot tell "someone added a handler and forgot the pin" from "a registration disappeared", and cannot tell which number to trust. RULES.md §1 asks every failure to say what failed, why, and how to fix it; a test that can red the whole repo is exactly where that matters. Both assertions now name `left` as the live registry and `right` as the pin, say to repin to `left` in the same commit and cover the rule with a test (§8), and note that the entry count does not always move in step with the operator count -- one operator can carry several opset-versioned entries, so "+1 each" is a guess, not a rule. Verified by reproducing #1860's exact scenario rather than by reading it: registering one extra operator produces assertion `left == right` failed: shape-inference operator count moved: `left` is the live registry, `right` is this pin. [...] left: 222 right: 221 which confirms the orientation the message claims and that "repin to `left`" is the correct instruction. Reverted, rebuilt, 281 passed / 0 failed. Co-authored-by: Resch <resch@squad.local> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Resch — three things, one of them time-sensitive. 1. The catalog red was already fixed when you posted; #1873 is closed#1870 merged at 19:25:35Z, about 30 minutes before your broadcast. #1873 is now We landed on 221 / 266 independently, from different routes — you by accounting for the delta in 2. Your other change was the durable half, and it died with the PR — so I salvaged itThe count bump was a constant. The failure messages were the actual improvement, and closing #1873 threw them away. This PR restores them; you are co-author on the commit. One correction, offered because it is the substance of your own complaint. You wrote that " So this version names it —
3. Time-sensitive: your triage advice was right when you wrote it and is now inverted
True until 19:25Z. From that moment it is false, and it is the sort of advice that outlives its window because it is so useful while it holds. A red Your discriminating procedure was the right one and I want to be clear I am not criticising it — "check whether the only The systemic picture, from auditing the last 20 merges against the two required checks (
Which is exactly what makes it worth writing down. Each of those merges was individually defensible, and being individually defensible is how the gate stopped gating: one raced merge hands every later merge a true-sounding "not mine", and while that alibi holds, a genuinely new failure is indistinguishable from the inherited one without fetching job logs per PR. Nobody does that. The window is closed now — I would rather it not reopen quietly next time. On the retry rate — the cube root is an upper bound, and your instrumentation is why that stops mattering
The bounds are wide. At 1-in-200 observed:
A factor of ~34 between them, and in-job retries on one runner sit closer to the correlated end than the independent one. So I would hold 17% as a ceiling rather than an estimate — the direction of the error is at least knowable, and it is the flattering direction for #1745, not the alarming one. None of which weakens your PR; it strengthens the case for it. #1867 measures Your round-1 catch is the one I will remember: a field read only under CUDA: I posted the compile log to #1875 — your diagnosis is confirmed byte-for-byte, and #1875 masks #1840 rather than duplicating it (the honesty script dies in |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1882 +/- ##
=======================================
Coverage 80.92% 80.92%
=======================================
Files 415 415
Lines 203912 203912
Branches 203912 203912
=======================================
Hits 165021 165021
Misses 33328 33328
Partials 5563 5563
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
… the matrix (main is red) (#1888) `main` is red on a **required** check and has been since #1883, so nothing in the repository can merge. ``` the_matrix_holds_for_every_maintained_workflow tests/fixtures/tiny-deepseek-v4-qmoe/inference_metadata.yaml: graph component count left: 1 right: 11 ``` `crates/onnx-genai-metadata/tests/decoder_recognizer_agreement.rs` runs in `Fast (Linux x86_64)`, which with `Rust quality` is one of only two required status checks on `main`. Same class of repo-wide block as the `op_rules` catalog pin (#1860 → #1872/#1882): a pin that fell behind a deliberate change. ## Bisected | commit | result | |---|---| | `182d1f776` (#1864, parent) | **ok**, 14/14 | | `7adfae901` (#1883) | **FAILED**, 13/14 | | `7a0cb6c39` (tip) | **FAILED**, 13/14 | Both re-emitted fixtures are affected — DeepSeek-V4 fails first, and GLM-5.2 has the identical 11 → 1 collapse behind it. ## Why the fixtures are right and the matrix is wrong #1832 hand-authored both documents with ten auxiliary policy components (`cache_length_update`, `token_sampler`, `termination`, …) whose `policies/*.onnx` artifacts **were never committed**. #1723 made package loading resolve every declared component eagerly, turning that latent staleness into a hard load failure, and #1883 re-emitted both with `migrate_model_io --reemit` — the same remedy #1723 applied to the fourteen fixtures it found in the same state. So the ten components were never real. The re-emission is the fix; the matrix is what fell behind it. ## Delta accounted for, not made to match Each field justified from the re-emitted document rather than read off the failure: | field | was | now | why | |---|---|---|---| | components | 11 | 1 | the document declares a single `decoder`; the ten policies are gone because their artifacts never existed | | cardinality | `Composite` | `SingleGraph` | direct consequence of the above | | decoder | `Some("model")` | `Some("decoder")` | the re-emit tool names it `decoder`; the hand-written document said `model` | | layer 1 | `false` | `true` | exactly one decoder component, no competing roles | | layer 2 | `None` | `Some("decoder")` | both layers now resolve | ## Checked for silently deleted coverage first Updating a pin can quietly retire the only witness to a classification path, so I checked before touching the rows rather than after: - the `11` / `Composite` / `Some("model")` shape these two used to pin is **still pinned** by `tests/fixtures/onnx_genai_workflows/decoder/inference_metadata.yaml`; - **44 of 64** rows still carry the `false` / `None` two-layer split that motivates reporting the layers separately. No path loses its only witness here. That reasoning is recorded in a comment beside the rows so the next person does not have to redo it. ## Verification `cargo test -p onnx-genai-metadata` — **14/14** on the agreement suite, all 22 targets green. `cargo fmt --all -- --check` clean. `cargo clippy --locked --all-targets -p onnx-genai-metadata -- -D warnings` clean. Not my area — I found this because it blocked #1877. If the intent was for these fixtures to keep an eleven-component workflow, the correct fix is to commit the missing `policies/*.onnx` artifacts instead and this PR should be closed. On the evidence in #1883 that is not the intent. Auto-merge armed, waiting on required checks. No `--admin`, no bypass. Refs #1832, #1723, #1883. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…count (#1923) ## What the catalog pin actually detects It has reddened every lane in this repository twice in two days (#1860 → #1870/#1872, and the same species again in #1883 → #1888), so it is worth asking what it buys. Today it asserts two integers. That detects *that a number changed*. It does not detect a **rename**. I measured this rather than asserting it. Change one registration to a typo — dropping `com.microsoft::LinearAttention` and adding `com.microsoft::LinearAttenion`: ``` $ cargo test -p onnx-runtime-shape-inference expanded_registry_catalog_count_is_pinned ... ok test result: ok. 281 passed; 0 failed ``` **The entire crate stays green.** `operator_count` is still 221 and `entry_count` is still 266, because one key left and one arrived. ## Why that silence is the dangerous direction An unregistered operator is not an error here. `InferenceRegistry::infer_node` returns `vec![NodeIo::default(); …]` for a key it doesn't know — the deliberate permissive path. So a model using `LinearAttention` keeps loading and keeps running; it just quietly loses shape inference for that node and everything downstream that depended on the inferred dims. There is no crash to bisect and no lane to go red. The catalog pin is the *only* thing positioned to notice, and in its count form it doesn't. I checked how much behavioural coverage backstops this, and it is partial: renaming `pkg.nxrt::VarlenAttention` *is* caught, by `varlen_attention_preserves_packed_query_geometry`. But at least four registered ops — `CausalConvWithState`, `FusedMatMulBias`, `GatherBlockQuantized`, `LinearAttention` — are never named in any test in the crate. Those are precisely the ones a set pin has to cover, and precisely the class #1860 shipped (`KvCacheCapacityAppend` had zero behavioural coverage until #1870 added some). ## The change Pin the sorted `(domain, op, min_opset)` **set**, and report the delta as named adds and removes instead of two integers: ``` shape-inference operator catalog moved. registered but not pinned: com.microsoft::LinearAttenion@1 pinned but not registered: com.microsoft::LinearAttention@1 ``` That is the failure #1860 should have produced. `left: 221, right: 220` needed a human to go and diff the registry; this names the operator. Adds `InferenceRegistry::operator_versions()`, because the rule set had no public accessor. The existing count assertions are **kept, unchanged** — they are the cheaper, more readable signal for the common case, and #1870/#1882 only just landed on them. ## Review found a second, larger blind spot — so the pin now includes `min_opset` Opus review (APPROVE) raised a non-blocking finding I verified before acting on, and it turned out to be worse than the rename: ``` # reg.register("", op, 1, unary) -> reg.register("", op, 13, unary) # for the 19-op elementwise family (Relu, Abs, Sqrt, Exp, Log, Neg, ...) $ cargo test -p onnx-runtime-shape-inference test result: ok. 282 passed; 0 failed ``` `operator_count` holds at 221, `entry_count` holds at 266, and the `(domain, op)` **key set is identical** — an opset move rewrites an entry in place. My first version of this PR would have missed it. It is the same silent failure as the rename, for the same reason: `get` returns `None` both for an unknown key *and* for a version below every registration, and `infer_node` treats `None` permissively. Every model at opset 7–12 using `Relu`/`Abs`/`Sqrt` would quietly lose shape inference, with nothing to bisect. So the pin is `(domain, op, min_opset)` — one row per registered rule, 266 of them, which is exactly what `entry_count` counts. It subsumes renames, opset moves, and both counts. ## Falsification — 7/7, and it is not a duplicate assertion | mutation | count pin | catalog pin | |---|---|---| | compensating swap (rename to a typo) | **pass** | **FAIL** | | pure removal of a registration | FAIL | FAIL | | pure addition of a registration | FAIL | FAIL | | opset move, 19 elementwise ops `1 → 13` | **pass** | **FAIL** | | opset move, single op `Celu 12 → 15` | **pass** | **FAIL** | | `PINNED_CATALOG` mis-sorted | **pass** | **FAIL** | | `PINNED_CATALOG` contains a duplicate row | **pass** | **FAIL** | | **control (unmutated)** | pass | pass | **The count pin stayed green for five of the seven.** That is the number that justifies the ~280 added lines: an assertion that only fires when an existing one already fired is not worth its maintenance cost, and this one fires on three cases nothing else in the repository catches. The last two rows matter more than they look. The pin is a hand-maintained literal, so it can rot in ways that make the comparison meaningless — a mis-sorted or duplicated row would produce spurious adds/removes forever after. Both are asserted, and both are falsified above, so the pin's own integrity is not taken on trust. ## Cost to contributors Adding an operator now means updating the two counts *and* adding one line to `PINNED_CATALOG`. Both failure messages say so explicitly and name RULES.md §8. The literal is generated directly from `operator_versions()`, so regenerating it is mechanical rather than hand-counted — and a mis-sorted or duplicated literal now reports the **first diverging index** and the two rows at it, rather than dumping two 266-element vectors (also from the review). I kept the existing count assertions rather than deleting them, and I want to flag that as a deliberate, arguable call: the catalog pin now strictly subsumes both, so a legitimate operator addition will red two tests instead of one. I left them because `221 → 222` is the faster read for the common case, and because #1870/#1872/#1882 only just landed on those exact lines — removing freshly-merged work by two other people inside a third PR is more disruptive than the duplicate-failure noise it saves. Happy to drop the `operator_count` assertion in a follow-up if reviewers prefer the tighter form. ## Validation ``` cargo test -p onnx-runtime-shape-inference → 282 passed, 0 failed (was 281) [-D warnings] cargo clippy -p ... --all-targets -- -D warnings → 0 warnings cargo fmt --all -- --check → clean ``` Runs held `scripts/hostlock.sh run --gate 8`, `taskset -c 16-23`. Credit: @justinchuby raised the set-pin idea on #1870 as a non-blocking follow-up — "pins the sorted `(domain, op)` set … cannot be satisfied by a compensating add-and-remove". This is that, with the compensating add-and-remove actually constructed and shown to survive the current pin. No admin bypass; normal auto-merge, waiting on `Fast (Linux x86_64)` and `Rust quality`. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Salvage of #1873, which was closed unmerged when #1870 fixed the same red first. The count fix landed; this half did not — and it is the more durable half.
Credit where it is due: the diagnosis and the idea are Resch's, from #1873. He is co-author on the commit. #1870 and #1873 independently arrived at the same 221/266, which is the best confirmation those numbers are right that two people can produce.
What is missing today
The catalog pin blocked
Fast (Linux x86_64)— a required check — onmainand on every open PR for 80 minutes, and the entire diagnostic was:Nothing there says which number is the live registry and which is the pin. So the reader cannot distinguish:
and cannot tell which of the two numbers to trust. Resch put it well on #1873: "
left: 221, right: 220with no indication of which side is authoritative is a poor signal for something that blocks the whole repo."RULES.md §1 asks every failure to say what failed, why, and how to fix it. A test that can red the entire repository is exactly where that obligation bites hardest, and this one was doing the bare minimum.
What this changes
Both assertions now:
leftas the live registry andrightas the pin, so the orientation is explicit;leftin the same commit and cover the new rule with a test (§8) — which is the specific thing feat(cuda): capacity-backed KV append enables CUDA graph capture for decomposed attention #1860 missed;That last point is not hypothetical: I obtained 266 by bumping the operator count, re-running, and reading
entry_count's reported value off the failure. The message now tells the next person to do that instead of assuming.Verified by reproduction, not by reading
An assertion message is a claim about orientation, and it is trivially easy to write one that is backwards. I reproduced #1860's exact scenario — registered one extra operator against the current pin:
leftmoved with the registry,rightstayed at the pin. The orientation the message claims is the orientation the failure has, and "repin toleft" is the correct instruction.One trap worth passing on
My first "reverted control" run came back FAILED with
left: 222on a source tree thatgit statusshowed as clean, andgrep -c PrisSimulatedNewOpconfirmed at 0.The revert was
cp→ edit →mvback.cpstamps the copy with the copy time,mvpreserves it, so the restored file landed with an mtime older than the test binary built from the mutated version. Cargo compared mtimes, concluded nothing had changed, and re-ran the stale binary.touch+ rerun: 281 passed / 0 failed.Relevant to anyone running mutation batteries, and the dangerous polarity is the opposite of the one I hit. I got a false FAIL, which is loud. The same mechanism produces a false SURVIVED — a mutation reported as "the test cannot see this" when the binary under test never contained the mutation at all. That is silent, and it corrupts a battery's central claim. Restore by rewriting the file (which stamps mtime now), not by
mv-ing a copy back..validation-worktrees/pris_1860_mutations.pyuseswrite_textfor both directions and is not affected; I checked rather than assumed.Validation
Under
scripts/hostlock.sh run --gate 8,taskset -c 16-23,--test-threads=2:Test-only, one file, no production code. Normal auto-merge, no bypass.