Repository navigation
fix(ort): give the non-dense loader tests a fixture they own (CLI ORT lane) - #1877
justinchuby wants to merge 3 commits into
Conversation
`CLI ORT` has been red on main on both Linux and Windows:
loader::tests::selected_non_dense_candidate_fails_explicitly
a selected non-dense candidate must fail: GraphIoMetadata {
outputs: [TensorInfo { name: "logits", dtype: Float32, ... }] }
This is not the `op_rules` catalog pin (#1873) and is not fixed by it.
Root cause: both non-dense tests borrowed `tiny-glm52-qmoe-indexshare`,
which is a real end-to-end decode fixture. #1832 regenerated it and
`logits` went from a non-dense type to a dense f32 tensor. The fixture
change is correct for its own purpose -- a real model has dense logits --
but two loader tests silently depended on it being non-dense.
The red test is the lesser half. The two tests encode the two halves of
one contract in `graph_io_from_model_path_for_names`: a *selected*
non-dense port must fail, and an *unrelated* non-dense port must never be
parsed. With `logits` dense:
* `selected_non_dense_candidate_fails_explicitly` fails loudly, and
* `selected_dense_kv_ignores_unrelated_non_dense_logits` keeps passing
while there is no longer any non-dense port for it to ignore.
The second is the dangerous one: it is green and guards nothing, so
fixing only the red test would leave the "unrelated non-dense" half of
the contract untested with no signal that it had been lost.
Fixed by giving these tests a dedicated `tests/fixtures/non-dense-logits`
fixture instead of borrowing a decode model, so regenerating a decode
fixture cannot reach them. It declares only graph inputs and outputs --
`graph_io_from_model_path_filtered` reads nothing else -- so there are no
nodes for a future regeneration to churn.
Also added `the_non_dense_fixture_is_actually_non_dense`, which asserts
the premise both siblings rest on. Neither sibling can detect the vacuity
itself, so without this the same silent failure recurs the next time
something touches the fixture.
Verified by mutation. Making the new fixture's `logits` dense (i.e.
replaying #1832):
the_non_dense_fixture_is_actually_non_dense .............. FAILED
selected_dense_kv_ignores_unrelated_non_dense_logits ...... ok
The sibling passing under the mutation is the evidence that the vacuity
is real and that the new guard is the only thing that catches it.
`cargo test -p onnx-genai-ort --lib` 168 passed / 0 failed, against
166 passed / 1 failed on main. fmt and clippy clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Independently re-run in a separate worktree, plus a second controlRan this branch's head against current 168/0 on the whole crate, not just A second control, complementary to the mutation already in the descriptionThe description mutates the fixture (makes Byte-identical to the CI failure, same tree, only this diff removed. The two controls answer different questions and both are worth having: the fixture mutation shows the new guard is load-bearing; the code revert shows the fix is load-bearing. A change can pass one and fail the other. CI triage
So on current |
Duplicate: #1877 and #1879 are the same fix, and they will conflictDisclosure first: #1877 was opened by a session of mine, so I am not neutral here and I am not the one who should pick. I am posting the comparison, not the verdict. Both PRs fix #1834 the same way, and reached the same design independently:
Both repoint They edit the same helper in the same file, so whichever merges second gets a conflict. This is the third time today the same defect has been fixed in parallel by people who could not see each other — #1870/#1872/#1873 on the catalog pin was the same shape, and there two of the three merged eight minutes apart. The one non-cosmetic differenceThe fixture name. The helper is So this should be decided on whichever the maintainer prefers to read, not on correctness. Both are correct. What each has that the other's reviewer may wantBetween the two threads there are now three independent controls on this fix, and they answer different questions:
A change can pass any one of these and fail another. Whichever PR survives, it is worth carrying all three into it. Note for whoever lands it
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #1877 +/- ##
===========================================
- Coverage 80.69% 72.56% -8.13%
===========================================
Files 415 12 -403
Lines 203912 5231 -198681
Branches 203912 5231 -198681
===========================================
- Hits 164551 3796 -160755
+ Misses 33796 1307 -32489
+ Partials 5565 128 -5437
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
The fixture guard in onnx-std enumerates every committed textproto via `git ls-files` with no allowlist, and requires exactly one default ai.onnx opset >= 24. This fixture inherited opset 21 from the decode fixture its content was modelled on, so `Fast (Linux x86_64)` -- a required check -- went red on a file that `cargo test -p onnx-genai-ort` never looks at. Bump to 24 and record in the header that this fixture is hand-authored: the guard's suggested remedy is "regenerate via its generator", which has no meaning here. Having no nodes keeps the node-schema half of the floor vacuous, so only the version fields need maintaining. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… 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>
|
Closing — superseded by #1879 ( I verified main rather than assuming: #1879's version is better placed than mine, and worth saying why. We both diagnosed the same root cause — #1832 regenerated
Theirs is the better call. This is the third duplicate fix I've seen on this repo today (#1870/#1872/#1873 all repinned the same Nothing from this branch is being carried forward. The one incidental artifact worth keeping is already in #1888 (merged, Branch deleted. |
|
Addendum to the close — two things I measured afterwards that the note above asserts but didn't test. 1. The design argument, confirmed by mutation rather than reasoningThe close says #1879's placement is better because Both. So the reasoning holds exactly as stated: the explicit test fails on its own, the sibling fails via the new helper, and there is no arrangement of the fixture in which one silently passes while the premise is gone. Under this PR's layout that mutation would have failed the explicit test and the standalone guard, while 2. My own validation section overclaimed, and I should name itI reported 168 passed / 0 failed from that command structurally could not have caught it. My fixture happens to declare That is the same defect I've spent the day flagging in other people's guards (#1817): a check whose claim is broader than its evidence. Mine was in the validation section of my own PR, one day after filing the issue about it. 3. The predicted conflict did landThe duplicate notice said these two edit the same helper and whichever merged second would conflict. For the record: Aborted rather than resolved — nothing here that #1879 doesn't already have, and better placed. |
1 participant
CLI ORT (Linux x86_64)andCLI ORT (Windows x86_64)have been red onmain. Resch flagged this as one of two failures not fixed by theop_rulescatalog pin (#1873) and not yet diagnosed. This is the diagnosis and the fix.Symptom
Root cause
Both non-dense tests borrowed
tests/fixtures/tiny-glm52-qmoe-indexshare, which is a real end-to-end decode fixture. #1832 (8ddaed5c7, "end-to-end GLM-5.2 + DeepSeek-V4 native decode fixtures") regenerated it, andlogitswent from a non-dense type to a dense f32 tensor. There are now zero non-dense types anywhere in that fixture.The fixture change is correct for its own purpose — a real model has dense logits. Nothing about #1832 is wrong. The defect is that two loader tests silently depended on a property of a fixture owned by someone else, for a different reason.
The red test is the lesser half
The two tests encode the two halves of one contract in
graph_io_from_model_path_for_names— "a selected port is validated strictly; unrelated non-dense ports are never parsed":logitsdenseselected_non_dense_candidate_fails_explicitlyselected_dense_kv_ignores_unrelated_non_dense_logitsThe second is the one worth worrying about. It is green while there is no longer any non-dense port for it to ignore — a fixture with no non-dense ports trivially fails to be blocked by one. Fixing only the red test would leave the "unrelated non-dense" half of the contract untested, with nothing to indicate it had been lost.
Fix
A dedicated
tests/fixtures/non-dense-logits/model.onnx.textprotothat these tests own, so regenerating a decode fixture cannot reach them. It declares only graph inputs and outputs —graph_io_from_model_path_filteredreads nothing butgraph.input,graph.outputandgraph.initializer— so there are deliberately no nodes for a future regeneration to churn.Plus
the_non_dense_fixture_is_actually_non_dense, asserting the premise both siblings rest on. Neither sibling can detect its own vacuity, so without this the identical silent failure recurs the next time anything touches the fixture.Verified by mutation
Making the new fixture's
logitsdense — i.e. replaying #1832 against it:The sibling passing under the mutation is the point. It is the evidence that the vacuity is real, and that the new guard is the only thing that catches it. Restored afterwards; 13/13
loader::testsgreen.cargo test -p onnx-genai-ort --lib: 168 passed / 0 failed, against 166 passed / 1 failed onmain.cargo fmt --checkandcargo clippy --all-targetsclean.What required CI caught that local testing could not
The first push of this branch turned
Fast (Linux x86_64)— one of only two required checks — red, and it was my defect, not an inherited one:crates/onnx-std/tests/fixture_ir_opset_guard.rsenumerates every committed*.textproto/*.onnxviagit ls-filesand applies a floor with no allowlist. The new fixture inheritedopset 21from the decode fixture its content was modelled on. Fixed by raising it to 24 (the file has no nodes, so the node-schema half of the floor is vacuous for it), and the header now records that it is hand-authored — the guard's suggested remedy, "regenerate the fixture via its generator", has no meaning here.Why local validation could not have caught it, and the part worth generalising:
Fastdoes not build or testonnx-genai-ort. Its package list stops atonnx-std. So the required check set does not cover the crate this PR fixes — it covers the fixture, because a fixture is a repo-wide artifact and the crate it serves is irrelevant to which lane sees it. "I ran the tests for the crate I changed" is structurally insufficient the moment a change adds a file undertests/fixtures/, and no amount of care within the changed crate closes that gap.The uncomfortable corollary for this specific fix:
CLI ORTis not a required check, so required CI verifies this fixture's hygiene but never its purpose. The contract these tests guard is confirmed by the mutation evidence above and by theCLI ORTlane going green, neither of which gates the merge. That is also why the lane was allowed to rot for as long as it did — a red advisory lane produces no forcing function, and once permanently red it stops reporting new failures too.CI state
Rebased onto
5318c3825. Theop_rulespin is fixed onmain(via #1872/#1870; #1873 was closed as one of three duplicate fixes), so that failure is gone from this branch.Fast (Linux x86_64)— requiredRust quality— requiredCUDA compile (Linux x86_64)— not required, red onmain, inherited (main: CUDA compile lane red — a #[cfg(test)] item is imported by an integration test, which is a separate crate #1875)Local, on the merged tree:
onnx-genai-ort --lib168/0;onnx-std190/0 including the fixture guard;op_rules281/0;cargo fmt --all --checkclean.Auto-merge is armed and waits for required checks. No
--admin, no ruleset bypass.Refs #1832, #1872, #1875.