Skip to content

fix(serve): delete two lib tests that assert nothing and take 19 minutes; scale four more - #2533

Open
noahgift wants to merge 1 commit into
mainfrom
fix/serve-slow-lib-benchmarks
Open

fix(serve): delete two lib tests that assert nothing and take 19 minutes; scale four more#2533
noahgift wants to merge 1 commit into
mainfrom
fix/serve-slow-lib-benchmarks

Conversation

@noahgift

Copy link
Copy Markdown
Contributor

These were the top cause of workspace-test timing out at its 75-minute step limit — which blocks every PR in the repo. They were invisible until #2517 fixed nextest's suppressed SLOW reporting; before that the job died anonymously.

From CI job 95324516796. "Max threshold" is the highest SLOW [> Ns] line seen, so real durations are higher — and several appeared as TRY 2 SLOW, i.e. being retried (retries = 2), tripling the cost:

max threshold test asserts
>1140s (19 min) test_parity017e_end_to_end_batch_throughput 0
>300s test_parity017a_gpu_batch_ffn_implementation 1
>300s test_parity018b_batch_ffn_gpu_method 1
>120s test_parity015d_batch_forward_timing 0

017e allocated batch 32 × hidden 2560 × intermediate 10240 × 4 layers of f32 — gigabytes — measured wall-clock with Instant, printed a throughput number, and asserted nothing. On a CPU-only runner the "GPU" path falls back to CPU. A 19-minute test that cannot fail is the purest form of the assertions-must-exclude-an-outcome defect.

Deleted (0 asserts), with tombstones pointing throughput work at benches/

Coverage lost, stated precisely rather than as "none":

  • 015d: nothing. It did let _ = scheduler.matmul(...) — result discarded. It could not fail even if matmul returned Err.
  • 017e: not literally nothing. Zero asserts but two .expect() calls, so it did check Err-freedom for a 4-layer FFN at phi-2 sizes. That same Err-freedom is still covered by 017a/018b on the same code path; what's genuinely gone is Err-freedom at phi-2 tensor sizes specifically, plus the 4-sequential-layer loop. A fair trade, but not zero.

Scaled (assertion kept)

017a, 018b, plus 015a and 016c found by enumerating the family programmatically. batch 32→8, hidden 2560→128, intermediate 10240→512 — 1600× less arithmetic. Dimensions chosen so m*k*n = 524,288 stays above HybridScheduler's gpu_threshold (262,144), so the GPU-vs-CPU dispatch decision is unchanged (verified: Should use GPU: true still prints). Meaningless Instant/GFLOPS prints removed; no timing assertions present or added.

A real defect found while mutation-testing — worth more than the speedup

Mutating HybridScheduler::matmul to drop one output element left 017a green. Its Err arm printed "SKIP - GPU path failed" and passed. Same hole in 015a and 016c:

-  println!("  Error: {} (expected if no GPU)", e);
-  println!("  Status: SKIP - Matmul failed (may be CPU fallback)");
-  println!("  Status: SKIP - GPU path failed");

Three tests passing when the operation they test failed — and the reassurance was false: cpu_matmul returns Ok unconditionally, so "may be CPU fallback" could never explain an Err.

Proven pre-existing rather than caused by the scaling, by re-running the identical mutation at the original [32×2560] dimensions — also test result: ok. Those arms are now panic!, after which all 8 combinations (4 tests × GPU and CPU-forced paths) go RED, rc=101.

Measurements

CPU-only path forced with VK_ICD_FILENAMES=/nonexistent and proven engaged (GPU available: false, GFLOPS 9.99 → 0.25) rather than assumed:

test before after
017e 55.3s deleted
015d 6.5s deleted
017a 13.9s 0.03s
018b 13.9s 0.03s
015a 7.0s 0.02s
016c 7.0s 0.02s
total 103.6s 0.10s

Absolute seconds here run ~20× below CI's for both 017a and 017e — a consistent ratio, being runner concurrency contention across 15,721 tests. Treat the ~1000× reduction as the reliable figure, not the absolute times.

Verification

cargo test -p aprender-serve --lib rc=0, 15,660 passed, 0 failed, 59 ignored
test count 15721 → 15719, exactly the two deletions
cargo fmt --check rc=0
cargo clippy --all-targets rc=101 — pre-existing, proven by materialising origin/main's version of these 5 files via git show and getting the identical rc and same 4 errors in files never touched here. No lint config changed.

CI cannot currently verify this: the fleet is down again (#2528, reopened — rustup deleted a second time). Everything above is local.

Refs #2503, #2517

…tes; scale four more

These were the top cause of `workspace-test` timing out at its 75-minute step
limit, which blocks every PR in the repo. They were invisible until #2517 fixed
nextest's suppressed SLOW reporting -- the job simply died anonymously before.

From CI job 95324516796, "max threshold" being the highest `SLOW [> Ns]` line
seen, so real durations are higher. Several appeared as `TRY 2 SLOW`, i.e. they
were being RETRIED (nextest.toml sets retries = 2), tripling the cost:

    >1140s (19 min)  test_parity017e_end_to_end_batch_throughput   0 asserts
     >300s           test_parity017a_gpu_batch_ffn_implementation   1 assert
     >300s           test_parity018b_batch_ffn_gpu_method           1 assert
     >120s           test_parity015d_batch_forward_timing           0 asserts

017e allocated batch 32 x hidden 2560 x intermediate 10240 x 4 layers of f32 --
gigabytes -- measured wall-clock with `Instant`, printed a throughput number, and
asserted NOTHING. On a CPU-only runner the "GPU" path falls back to CPU. A
19-minute test that cannot fail is the purest form of the
assertions-must-exclude-an-outcome defect.

DELETED (0 asserts), with a tombstone comment pointing throughput work at benches/:
  test_parity017e_end_to_end_batch_throughput
  test_parity015d_batch_forward_timing

Coverage lost, stated precisely rather than as "none":
  * 015d: nothing. It did `let _ = scheduler.matmul(...)` -- result discarded.
    It could not fail even if matmul returned Err.
  * 017e: NOT literally nothing. 0 asserts but 2 `.expect()` calls, so it did
    check Err-freedom for a 4-layer FFN at phi-2 sizes. The same Err-freedom is
    still covered by 017a/018b on that code path; what is genuinely gone is
    Err-freedom at phi-2 tensor sizes specifically, and the 4-sequential-layer
    loop. A fair trade, but not zero.

SCALED (assertion kept): 017a, 018b, plus 015a and 016c found by enumerating the
family programmatically. batch 32->8, hidden 2560->128, intermediate 10240->512
-- 1600x less arithmetic. Dimensions chosen so m*k*n = 524,288 stays above
HybridScheduler's gpu_threshold (262,144), so the GPU-vs-CPU dispatch decision is
UNCHANGED (verified: "Should use GPU: true" still prints). Meaningless
Instant/GFLOPS prints removed; no timing assertions present or added.

A REAL DEFECT FOUND WHILE MUTATION-TESTING, worth more than the speedup

Mutating HybridScheduler::matmul to drop one output element left 017a GREEN. Its
Err arm printed "SKIP - GPU path failed" and PASSED. Same hole in 015a and 016c:

    -  println!("  Error: {} (expected if no GPU)", e);
    -  println!("  Status: SKIP - Matmul failed (may be CPU fallback)");
    -  println!("  Status: SKIP - GPU path failed");

Three tests PASSING when the operation they test FAILED -- and the reassurance
was false: `cpu_matmul` returns Ok unconditionally, so "may be CPU fallback"
could never explain an Err.

Proven PRE-EXISTING rather than caused by the scaling, by re-running the
identical mutation at the original [32x2560] dimensions: also `test result: ok`.
Those arms are now `panic!`, after which all 8 combinations (4 tests x GPU and
CPU-forced paths) go RED, rc=101.

MEASUREMENTS, CPU-only path forced with VK_ICD_FILENAMES=/nonexistent and PROVEN
engaged (GPU available: false, GFLOPS 9.99 -> 0.25) rather than assumed:

    017e 55.3s -> deleted     015d 6.5s -> deleted
    017a 13.9s -> 0.03s       018b 13.9s -> 0.03s
    015a  7.0s -> 0.02s       016c  7.0s -> 0.02s
    total 103.6s -> 0.10s

Absolute seconds here run ~20x below CI's for both 017a and 017e -- a consistent
ratio, being runner concurrency contention across 15,721 tests. Treat the ~1000x
REDUCTION as the reliable figure, not the absolute times.

VERIFICATION
  cargo test -p aprender-serve --lib   rc=0  15,660 passed, 0 failed, 59 ignored
  test count 15721 -> 15719, exactly the two deletions
  cargo fmt -p aprender-serve -- --check   rc=0
  cargo clippy -p aprender-serve --all-targets  rc=101 PRE-EXISTING -- proven by
    materialising origin/main's version of these 5 files via `git show` and
    getting the identical rc and the same 4 errors in files never touched here
    (examples/bench_manual_threads.rs, src/format_detect.rs,
     src/api/tests/cancel_scope_2376.rs, src/memory.rs). No lint config changed.

Refs #2503, #2517
@noahgift
noahgift added this pull request to the merge queue Aug 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 17, 2026
noahgift added a commit that referenced this pull request Aug 18, 2026
The sovereign-ci image bakes cargo-nextest but not cargo-mutants, so
"cargo mutants" exited 101 (no such command) on every run. The pre-#2514
script treated a missing outcomes.json as "0 mutants in diff. Pass."
without consulting the exit code, so the blocking mutation gate passed
every PR precisely because the tool did not exist.

Visible on #2533, which went green yesterday with this in its log:

  error: no such command: mutants
  cargo-mutants exit: 101
  No mutants.out/outcomes.json - 0 mutants in diff. Pass.

#2514 (in this batch) closed that hole, which is why the gate now fails
on #2534 rather than passing: it is correctly refusing to report a result
it cannot measure. The gate is right; the environment is wrong.

Install cargo-mutants in the job as a stopgap, pinned to 27.1.0, and then
assert it RUNS -- an install that half-succeeds must not reach the gate
looking like a clean diff, which is the same failure mode #2514 closed.
The tool belongs baked into the image; tracked separately against infra.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@noahgift
noahgift added this pull request to the merge queue Aug 18, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 18, 2026
@noahgift
noahgift added this pull request to the merge queue Aug 18, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant