Repository navigation
fix(cuda-ep): gate the Celu GPU test behind gpu-tests like its neighbours - #1941
Merged
Merged
Conversation
…ours `celu_matches_cpu_including_nan_and_alpha_variants` was added in #1909 with a bare `#[test]`. The other four tests in `activations_gpu.rs` all carry `#[cfg_attr(not(feature = "gpu-tests"), ignore = ...)]`, so without the feature this one alone attempted to construct a CUDA EP and **failed** on a CPU-only runner instead of reporting ignored. `verify_cuda_test_honesty.py` caught it exactly as designed: ``` - activations_gpu: 1 tests failed while checking ignored status (celu_matches_cpu_including_nan_and_alpha_variants) - activations_gpu: Cargo inventory has 5 tests but libtest reported 4 ignored ``` This reached `main` because #1909 was merged with `--admin`, which bypasses the very check that reports this class of mistake. Verified on a CUDA host: - without the feature: `0 passed; 0 failed; 5 ignored` (was 4 ignored plus one live test) - with `--features gpu-tests --test-threads=1`: `5 passed; 0 failed; 0 ignored` -- the test still genuinely runs, it is not ignored everywhere Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
justinchuby
added a commit
that referenced
this pull request
Aug 24, 2026
…with a source scan (#1920) ## What Two things, one cause. 1. **The fix:** `celu_matches_cpu_including_nan_and_alpha_variants` (landed in #1909, `d6e542509`) has no `#[cfg_attr(not(feature = "gpu-tests"), ignore = "...")]`, so it *runs* on the CUDA lane's no-GPU builder and reds it. 2. **The guard:** a `--source-scan` mode in `verify_cuda_test_honesty.py`, wired into the fast `rust-quality` lane, that catches this class **in ~75 ms** instead of after a 20-minute CUDA build. ## Why a guard and not just the fix This is the **sixth** instance of the class in four days, and the fifth of this exact sub-class: | # | test | landed | fixed by | |---|---|---|---| | 1-3 | `content_preserving_transition_gpu` (+2 more) | | #1881 `e42fa9470` | | 4 | `causal_conv_with_state_gpu` | | #1881 | | 5 | `activations_gpu::mish` | | #1911 `3f8103478` | | 6 | `activations_gpu::celu` | #1909 | **this PR** | The shared cause isn't carelessness. The authoritative check only runs on `CUDA compile (Linux x86_64)`, which takes ~20 minutes and has itself been red for unrelated reasons for much of this week — so an author gets no usable signal at the time they can act on it. Another one-off fix leaves instance seven to the same trap. The scan runs on a lane that is fast, always-on for any `.rs` change, and green. ## Scope of the guard, stated honestly The scan detects **exactly one** sub-class: a `#[test]` in a policed CUDA target with no effective ignore. It is **blind** to the target-level `#![cfg(feature = "gpu-tests")]` inventory-drift sub-class — which I found while validating this PR, and which is fixed separately in **#1927**. Extending the scan to that sub-class is deliberately deferred to a follow-up so a guard and a green tree land together. It reuses `is_cuda_test_target()` rather than re-deriving "policed target", so there remains one definition. ## Validation **Positive controls** (would a real defect be caught?) — reverting the ignore flags **exactly one** test, by correct name and line, in each case: - Celu, on this branch - `mish` at `3f8103478^` — a genuine historical instance - `causal_conv_with_state_gpu` at `e42fa9470^` — another **Negative controls** (does correct code pass?) — clean on all 79 CUDA targets in 75 ms, including `qmoe_gpu` (10 macro-generated tests) and `coarse_residency_plan_gpu` (2536 lines / 13 tests, written by someone else, landed mid-branch — a negative control I did not construct). **Combined with #1927**, the authoritative script is green: `554 tests / 81 targets` in both feature configurations, `exit 0`. Each branch alone is red only on what the other fixes. `--self-test`: **30 fixtures**. `cargo fmt --all -- --check` clean. ## Two rounds of independent Opus review, and what they found I reproduced every reported defect myself before fixing it, and mutation-tested every defense afterwards. **Round 1 — three real defects, one of them serious:** - **Fail-open (major):** bracket counting did not ignore string literals, so the upward attribute walk could escape an item and adopt the *previous* test's ignore — silently clearing a genuinely un-ignored test. - That same escape **resurrected the exact vacuity this PR exists to fix**: v1 of the scan searched the item head for the substring `"ignore"`, and Celu's doc comment reads "a kernel that **ignored** the attribute entirely". The guard passed on English prose. It failed its own positive control, which is the only reason I know. - `#[cfg_attr(not(feature = "something-else"), ignore)]` was accepted as a valid ignore. Fixed by rewriting the internals: length-preserving, column-exact masking of literals and comments; hard item boundaries; predicate validation by `fullmatch`. While fixing, I **found a fourth defect myself** — a false positive on `vmm_kv_layout_residency_gpu.rs:355`, caused by `.strip()`ing lines when building the masked and raw blobs, which broke the column alignment the span mapping depends on. My own fail-closed length guard caught it. **Round 2 — four residual holes:** - A blank line inside an attribute list broke the walk → **false positive on correct code**, which would red a green lane. Worse than a miss, in a guard whose whole value is being trusted. - The `cfg_attr` predicate was matched as a substring, so a nested `cfg_attr` could smuggle a different predicate through. - Non-canonical `#[test]` spellings (e.g. `#[ test ]`) were never inspected at all. - `pub(crate) fn` was not matched by the `pub\s` item boundary. All four fixed, each with a fixture. ## Mutation matrix Every defense is falsifiable — I broke each one and confirmed a **distinct** failure: | mutation | caught by | |---|---| | ungate the blank-line break | blank-line-in-attribute fixture | | match the predicate by substring | nested-`cfg_attr` fixture | | permit a nested `cfg_attr` | predicate fixture | | match only canonical `#[test]` | spacing fixture | | always walk downward | one-line `#[test] fn a() {}` fixture | | remove the `fn` item boundary | `pub(crate)` fixture | | relax `IGNORE_ATTR` to a substring | doc-prose fixture | | disable masking | literal-bracket fixture | One finding worth passing on: `FN_DECLARATION` and an enumerated `pub[\s(]` boundary turned out to be **redundant**, and the symptom was that *neither could be mutation-caught* — each was covered by the other. An unfalsifiable guard is dead weight regardless of whether it is correct. I removed the enumeration, kept the token anchor, and confirmed the fixture then fires. ## Risk The scan is additive and read-only. Its failure mode if wrong is a false positive on the fast lane — visible immediately, fixable in one line, and specifically fixture-guarded above. The Celu change is a single attribute. Closes part of #1875. --- ## Maintainer follow-up: rebased, fix half already landed, and one number corrected **The fix half is gone from this PR.** I hit the same Celu failure independently and — not having checked the open PR list first, which was my mistake — landed the one-line ignore separately as #1941. Rebasing this branch onto `main` dropped that hunk cleanly. What remains is **only the guard**, which was always the valuable half: ``` .github/scripts/verify_cuda_test_honesty.py | 456 +++++++++++++++++++++++++++- .github/workflows/ci.yml | 17 ++ ``` **Independently re-verified the guard on a Windows CUDA host**, since a guard I have not personally seen fire is not a guard I should merge: | Check | Result | |---|---| | Scan on current `main` | `CUDA test source scan passed: every test in a CUDA target carries an ignore`, exit 0 | | **Falsified** — Celu's `cfg_attr` removed again | exit 1, `activations_gpu.rs:336: celu_matches_cpu_including_nan_and_alpha_variants has no ignore attribute` — correct file, correct line, correct test, with the fix spelled out | **One number is wrong and I am correcting it rather than repeating it.** The body claims the scan is clean *"in 75 ms"*. On this Windows host the median of three steady-state runs is **~3.4 s wall** (3383 / 3546 / 4438 ms), of which ~0.6 s is interpreter startup alone. I could not reproduce 75 ms here; it may hold on a Linux runner with a warm cache, but as written it reads as a cross-platform property and is not one. This changes nothing about the argument. The comparison that matters is 3.4 s on an always-on fast lane against ~20 minutes on a CUDA lane that has been red for unrelated reasons much of this week — still three orders of magnitude, and still the difference between signal an author can act on and signal they cannot. But a number that is off by 160x reads as evidence, so it should not stand. --------- 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 #1941 +/- ##
==========================================
+ Coverage 80.18% 80.27% +0.09%
==========================================
Files 399 421 +22
Lines 186130 206129 +19999
Branches 186130 206129 +19999
==========================================
+ Hits 149243 165466 +16223
- Misses 31527 35038 +3511
- Partials 5360 5625 +265
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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
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.
What broke
celu_matches_cpu_including_nan_and_alpha_variants(added in #1909) has abare
#[test]. The other four tests inactivations_gpu.rsall carry:So without the feature, this one test alone tried to construct a CUDA EP and
failed on a CPU-only runner instead of reporting ignored.
verify_cuda_test_honesty.pyflagged it precisely:Why it reached main
#1909 was merged with
gh pr merge --squash --admin, which bypasses the checkthat reports this. The check was working; nothing was listening. Worth noting
plainly rather than filing under "flake".
Verification
On a CUDA host, in a registry-rebuilt child process:
0 passed; 0 failed; 5 ignored— was 4 ignored plus one live test--features gpu-tests --test-threads=15 passed; 0 failed; 0 ignoredThe second row matters as much as the first: the point is to make the test
ignored where it cannot run, not ignored everywhere. A one-line
#[ignore]would have satisfied the honesty check while silently deleting the coverage.
Note on a second failure in the same CI run
The same job also reported 13
coarse_residency_plan_gpuinventory failures.Those are not addressed here and need no action: they are the pre-existing
issue #1927 fixed, and the branch that surfaced them predates that merge.