Skip to content

docs(cuda): name DsaIndexSelect in the coverage matrix, which main is red without - #2271

Draft
justinchuby wants to merge 2 commits into
mainfrom
gaff1-cuda-coverage-dsaindexselect
Draft

justinchuby wants to merge 2 commits into
mainfrom
gaff1-cuda-coverage-dsaindexselect

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

origin/main is currently red. Not a flake, not a merge artifact — reproduced on a pristine detached checkout of 4def2238b with zero tracked modifications:

$ git status --porcelain --untracked-files=no    # empty
$ cargo test --locked -p onnx-runtime-ep-cuda --lib
test result: FAILED. 665 passed; 1 failed; 36 ignored

---- kernels::tests::every_cuda_covered_op_is_named_in_current_coverage_matrix ----
panicked at crates/onnx-runtime-ep-cuda/src/kernels/mod.rs:1884:5:
assertion `left == right` failed: CUDA_COVERED_OPS names and current coverage-matrix names diverged

The set difference is exactly one name. #2076 registered pkg.nxrt::DsaIndexSelect and added it to CUDA_COVERED_OPS, but added no row to the matrix in docs/execution/CUDA_COVERAGE.md, which is include_str!'d at kernels/mod.rs:1686 and parsed between the CUDA_COVERAGE_MATRIX_START/END markers. grep -c DsaIndexSelect docs/execution/CUDA_COVERAGE.md → 0.

The fix

One matrix row, in the pkg.nxrt sparse/KV block next to IndexShare, which is the indexer it serves. The Notes cell is taken from the kernel's own module documentation rather than paraphrased: device-resident head-weighted ReLU'd scaled dot-product scoring, additive f32 causal bias, the bias > -1e30 allow mask, top min(#allowed, top_k) by (score desc, index asc), emitted ascending with -1 right padding, and NaN canonicalization to positive quiet NaN on both backends giving bit-parity with the frozen CPU oracle.

After:

$ cargo test --locked -p onnx-runtime-ep-cuda
82 targets: 732 passed, 0 failed, 656 ignored, exit 0

Why it merged green, which is the part worth reviewing

No CI lane runs this crate's tests. Asked the real resolver rather than reading lists:

$ for lane in offline-linux lint ort-backed offline-cross-platform; do
    python3 .github/scripts/workspace_test_packages.py cargo-args "$lane" | grep -cx onnx-runtime-ep-cuda; done
0
0
0
0

workspace_test_packages.py:219 denies it: "onnx-runtime-ep-cuda": "runtime tests require a CUDA GPU". The two CUDA lanes only cargo check and cargo clippy --all-targets. So this assertion compiles in CI on every PR and has never executed there.

The reason is true of the 656 ignored tests. The mechanism it triggers discards the 732 that need no GPU, no toolkit and no runner change — the crate already gates the GPU ones itself via #[ignore] and the gpu-tests feature. A per-package switch is carrying a per-test property, so a doc/code consistency guard that works perfectly is wired to nothing.

Worth noting CUDA_COVERAGE.md is one of the two files #2081 taught the change-scope classifier to treat as code, so this PR correctly runs full CI rather than classifying as docs-only. That half is working; what it gates does not run the test.

Not bundled here. Removing the denylist entry changes required-lane composition and trips the resolver's own unenforced enforcement, so it needs its own review and its own validation. Filing separately. This PR is one markdown row that turns main green.

… red without

`kernels::tests::every_cuda_covered_op_is_named_in_current_coverage_matrix`
asserts CUDA_COVERED_OPS equals the op set parsed out of the current
CUDA_COVERAGE.md matrix. #2076 registered `pkg.nxrt::DsaIndexSelect` and added
it to CUDA_COVERED_OPS without a matrix row, so the two sets diverged and the
assertion has been failing on main ever since:

    cargo test --locked -p onnx-runtime-ep-cuda --lib
    test result: FAILED. 665 passed; 1 failed; 36 ignored

reproduced on a pristine checkout of origin/main (4def223) with zero tracked
modifications. With the row added the whole package is green on a box with no
GPU and no CUDA toolkit:

    cargo test --locked -p onnx-runtime-ep-cuda
    82 targets: 732 passed, 0 failed, 656 ignored, exit 0

The row states what the kernel actually does, taken from its module docs rather
than paraphrased: device-resident scoring, the `bias > -1e30` allow mask, top
`min(#allowed, top_k)` by `(score desc, index asc)`, ascending emission with
`-1` padding, and NaN canonicalization giving bit-parity with the frozen CPU
oracle.

Why it reached main green is a separate defect, reported alongside this: no CI
lane runs this crate's tests. `workspace_test_packages.py` denies
`onnx-runtime-ep-cuda` with the reason "runtime tests require a CUDA GPU",
which is true of the 656 ignored tests and discards the 732 that need nothing.
The CUDA lanes only `cargo check`/`clippy --all-targets`, so this assertion
compiles in CI and has never once executed there. That fix needs its own
review and is not bundled here.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby marked this pull request as draft August 27, 2026 08:55
@codecov

codecov Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.32%. Comparing base (743bd7c) to head (385602a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2271      +/-   ##
==========================================
+ Coverage   81.19%   81.32%   +0.13%     
==========================================
  Files         434      434              
  Lines      222250   222250              
  Branches   222250   222250              
==========================================
+ Hits       180452   180748     +296     
+ Misses      35869    35574     -295     
+ Partials     5929     5928       -1     
Flag Coverage Δ
cli-ort-linux 72.51% <ø> (ø)
cli-ort-windows 72.10% <ø> (ø)
mlas 86.02% <ø> (+0.12%) ⬆️
offline 81.46% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justinchuby

Copy link
Copy Markdown
Owner Author

Re-measured at today's main, because the last transcript was three merges old

main has moved four times since I opened this (4def2238b → 8395917c0 → … → 47a35b82e). A reviewer could fairly discount a red from three commits ago, so here it is again, on a pristine detached worktree at 47a35b82e with zero tracked modifications:

test kernels::tests::every_cuda_covered_op_is_named_in_current_coverage_matrix ... FAILED
panicked at crates/onnx-runtime-ep-cuda/src/kernels/mod.rs:1884:9:
assertion `left == right` failed: CUDA_COVERED_OPS names and current coverage-matrix names diverged
test result: FAILED. 0 passed; 1 failed; 0 ignored; 701 filtered out

The two sets are 168 and 169 names. They differ by exactly one element, DsaIndexSelect — present in CUDA_COVERED_OPS, absent from the matrix in docs/execution/CUDA_COVERAGE.md. This PR is that one row.

main is red right now, and has been since #2076. It is invisible because onnx-runtime-ep-cuda runs in no test lane — that is #2274's problem, not this one. This PR is the whole fix for the red, and it is one line of markdown.

A note on the transcript above, because my first attempt at it was vacuous

My first run of this used the bare test name as an --exact filter:

FILTER-SELECTS=0
FILTER-DRIFT: selected 0, expected 1

The test lives in mod tests, so its full path is kernels::tests::every_cuda_..., and a bare name matches nothing. Without the count guard I would have read 0 passed; 0 failed, exit 0, and concluded main was green — and the conclusion I would have drawn from a broken instrument was to withdraw a correct PR.

That is the trap from #2070 and the per-binary row in #2269, firing on the person who wrote the row, on the first real use, in the direction that costs the most. I am reporting it because the guard is only worth anything if people see it catch something, and because it is the exact shape I have been asking others to watch for: an instrument that finds nothing and an instrument that cannot find anything read identically.

Checks here: 18 unique names, all SUCCESS, 0 skipped, both required lanes SUCCESS by name.

Refresh the base: the prior green was measured against a tree 11 commits old.
CUDA_COVERAGE.md is embedded via include_str!, so this must be re-tested
against current main rather than assumed docs-only.
justinchuby added a commit that referenced this pull request Aug 27, 2026
Base PR #2271 was re-synced to main 743bd7c; carry that forward so this
PR's evidence is measured against the tree that would actually land.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant