Repository navigation
fix(metadata): repin classification MATRIX rows re-emitted by #1883 - #1886
justinchuby wants to merge 1 commit into
Conversation
#1883 re-emitted tiny-deepseek-v4-qmoe and tiny-glm52-full-attention, correctly dropping ten policy components whose `policies/*.onnx` artifacts were never committed. The classification MATRIX in decoder_recognizer_agreement.rs still pinned the old 11-component composite shape, so `the_matrix_holds_for_every_maintained_workflow` has failed on `main` at its own head since 7adfae9 -- reddening a required check on every open PR. Measured, not guessed: both fixtures now classify as graph_components=1, SingleGraph, decoder=Some("decoder"), is_single_decoder=true, contracted=Some("decoder"), which is exactly the shape their sibling fixtures already carry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1886 +/- ##
===========================================
+ Coverage 72.56% 80.33% +7.76%
===========================================
Files 12 415 +403
Lines 5231 204344 +199113
Branches 5231 204344 +199113
===========================================
+ Hits 3796 164164 +160368
- Misses 1307 34606 +33299
- Partials 128 5574 +5446
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Closing as superseded by #1888, which landed the identical repair while this was in Opus review. No disagreement whatsoever: #1888's two rows are field-for-field identical to these, and a third independent derivation (Gaff's local Verified on merged Nothing here needed to merge. Leaving the branch |
1 participant
mainis red at its own head; this is the second half of itFast (Linux x86_64)is a required check, and it currently fails onmainand therefore on every open PR branched from it:This is not the
op_rulescatalog-pin failure fixed in #1870 — that one is gone. This is a second, independent inherited failure underneath it.Bisect
the_matrix_holds_for_every_maintained_workflow182d1f7767adfae901git log 182d1f776..7adfae901 -- tests/fixtures/is #1883 alone.#1883 was right; only the pin lagged
#1883 (
fix(fixtures): re-emit stale DeepSeek-V4/GLM-5.2 workflow metadata) is a correct change and nothing here reverts it. Those two fixtures were hand-authored as 11-component composites whose tenpolicies/*.onnxartifacts were never committed, so #1723's eager artifact resolution hard-failed on them. Re-emission dropped the unresolvable components. What it did not do was update the classification MATRIX rows that pin the old shape — RULES.md §8, "update fixtures and expected counts in the same commit".That makes this the second merged defect of the same species today, after #1860/#1870: a deliberate data change plus a pinned expectation elsewhere that did not move with it.
The new values are measured, not arithmetic
I read them off the live classifier for both fixtures rather than inferring them from the diff:
graph_component_countcardinalityCompositeSingleGraphdecoder_componentSome("model")Some("decoder")is_single_decoderfalsetruecontracted_single_decoderNoneSome("decoder")Both fixtures land on identical values, and that tuple is exactly what their immediate neighbours in the MATRIX (
tiny-gemma4-assistant,tiny-glm52-qmoe-indexshare, …) already carry — independent corroboration that this is the shape a re-emitted single-decoder workflow is supposed to have, not a number chosen to make a test pass.No coverage is lost. ~19 other rows remain
Composite, so the composite branch of the classifier is still exercised.Falsification — a repinned row must not be silently unpinnable
A repin is worth very little on its own: it proves only that somebody changed a number. So I mutated each field of each repaired row and required the suite to fail.
11SingleGraph→CompositeSome("model")is_single_decodertrue → falseSome("decoder")→None11SingleGraph→Composite8/8 caught, control passes. Row 2 is the one that matters most: restoring the exact pre-#1883 value is caught, so the old row was genuinely wrong and the new one is genuinely load-bearing.
Battery restores by rewriting the file rather than
cp/mv, because a restored-with-older-mtime file makes cargo skip the rebuild and rerun the stale binary — whose dangerous polarity is a false SURVIVED.Local validation
All three MATRIX tests pass, not just the failing one:
the_matrix_holds_for_every_maintained_workflow,the_free_functions_are_the_classification,the_matrix_covers_every_maintained_workflow.Test runs held
scripts/hostlock.sh run --gate 8and weretaskset-bounded to cores 16–23.No admin bypass. Normal auto-merge, waiting on
Fast (Linux x86_64)+Rust quality.cc @justinchuby — no blame intended toward #1883's author; the fixture change was correct and only the pinned expectation lagged behind it.
Post-review addendum
Opus review returned APPROVE with three non-blocking findings. Two are worth recording, and one of them I verified rather than accepted — and it came back partly different from the claim.
What this test can and cannot prove
The reviewer's sharpest point: this MATRIX test pins the classifier's output against the fixture. It therefore cannot distinguish a correct single-decoder re-emission from a lossy component deletion — a fixture-degradation mutation would sail straight through
cargo test -p onnx-genai-metadata. So a green matrix test is not end-to-end proof that #1883 was right, and nobody should read it as one.The actual correctness anchor is the engine e2e suite. I ran it:
The re-emitted fixtures still decode to their locked anchor IDs and still load under stock ORT. #1883 was correct and this PR is the right fix.
Two caveats on that anchor, which the "4/4" hides
The CUDA arm did not actually run. Both
native_cuda_matches_cputests printskipping … CUDA is unavailable: CUDA_ERROR_NO_DEVICEand then returnOk(()). They are reported asok. The genuine evidence here is the CPU eager-decode and stock-ORT arms, which do execute. I'm stating that explicitly because "4/4 across CPU/CUDA/stock-ORT" would overclaim it.None of these eight tests runs in a required lane. Both files are
#![cfg(feature = "native-cuda")]in their entirety, so underFast (Linux x86_64)andRust qualitythey are not compiled in at all —cargo test -p onnx-genai-enginewithout the feature reports0 passed. They also self-skip toOk(())on missing fixtures.So the only gate that can tell a correct re-emission from a lossy one is (a) behind a non-default feature, (b) silent-skipping on two independent conditions, and (c) not required. That is how #1883 could remove ten components and have nothing anywhere object except a pinned count in a different crate. Filed separately; it is not a blocker for this PR.
Convergence
Gaff independently produced the identical repin on the same two rows off the same base (
6aa95e226, local branch, never pushed — no competing PR exists). Two authors arriving at the same five values from the same evidence is corroboration; I've left a note so the duplicate can be dropped rather than conflict.