Re-target the slow marker at footprint, and replace the stale suite timings with measured ones - #343
Conversation
There was a problem hiding this comment.
Pull request overview
This PR redefines the slow pytest marker for the ROCm test suite to reflect memory footprint risk under xdist parallelism, and updates documentation/comments to match newly measured cold vs warm test-suite timing behavior (highlighting that cold runs are dominated by JIT compilation rather than test execution).
Changes:
- Removes
@pytest.mark.slowfrom several ROCm tests that are not footprint-heavy (including 1M-trial frequency tests and the LogitsPipe compilation class). - Keeps
slowonly on the speculative-sampling chain test, and updates its in-file rationale to be footprint-based. - Updates
pyproject.toml,CONTRIBUTING.md, andCLAUDE.mdto document the new marker intent and measured cold/warm timings.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rocm/test_sampling.py | Removes slow from non-footprint tests; documents why speculative chain sampling remains slow. |
| tests/rocm/test_logits_processor.py | Removes slow from the compilation-vs-non-compilation class; corrects the class docstring’s claims. |
| tests/rocm/test_customize_prefill_use_softmax.py | Removes slow so this regression coverage is no longer retired by -m "not slow". |
| pyproject.toml | Updates the slow marker description to be footprint-focused (but needs a small wording consistency fix). |
| CONTRIBUTING.md | Updates test-running guidance and records measured cold/warm timings (but contains one outdated “fast path” description). |
| CLAUDE.md | Updates the Essential Commands table and guidance to reflect footprint-based marker semantics and cold-cache behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Self-review round on Fixed. The slow lane was documented as Measured, not assumed. Unmarking 343 sampling cases moves them into a concurrency regime the Deferred, with the reason. |
…a box Measured every test in the default selection at a855402 on gfx950 (1x MI350X, ROCm 10.0 / HIP 7.15, torch 2.12, aiter 0.1.20, -n 1), cold and warm, with a per-test duration recorder on the xdist controller. Nothing in this repo runs `-m slow` -- no CI job, no script -- and `-m "not slow"` is also what scripts/amd_coverage.py documents. The marker therefore retires a test rather than deferring it, which makes runtime an inadmissible reason to apply it and leaves footprint as the only one. Six of the seven marks did not meet that bar: the four 1M-trial sampling-frequency tests 11.2 s warm, 90 cases TestLogitsPipeCompilationHIP 27.1 s warm, 252 cases test_use_softmax_false_ragged_prefill_... 0.3 s warm, 1 case The last is the one that mattered. It is the only use_softmax=false coverage in testpaths -- tests/utils/test_jit_example.py is outside it -- so the marker had retired the regression test for the uninitialized accumulator NaN fixed in #321, in exchange for 0.3 s. test_chain_speculative_sampling keeps its mark, with the reason corrected. The comment said ~4 GB, which is one tensor; the worst case (batch 989, vocab 128256, 7 speculate tokens) holds four live at once -- 2 x 989x7x128256x4 B plus 2 x 989x8x128256x4 B, about 15 GB -- and `-n auto` runs several such workers together. Deliberately not marked, though they are the slowest tests in the suite: test_aot.py's three compile-and-package cases (113.8 s), test_force_cta_tile_q.py (48.9 s), test_torch_compile.py (33.8 s). They are the only coverage of the AOT packaging path, FLASHINFER_ROCM_FORCE_CTA_TILE_Q and FLASHINFER_USE_TORCH_CUSTOM_OPS; 196 s off a 574 s suite does not buy retiring them. Case count is not the signal either: test_rope.py is the largest matrix at 13,080 cases and costs 1.2 ms each, 2.8% of warm runtime. Verified by collection, not arithmetic: 36,766 "not slow" + 72 "slow" = 36,838 total.
CONTRIBUTING.md claimed ~7 min for `-m "not slow"`, ~20 min full and ~13 min for `-m slow`. Those came in with #219 on 2026-05-01 and were copied forward by #307 without re-measuring; the four largest test files have grown 43% since (18,947 -> 27,102 cases) and the suite is now 36,838. They also omitted the effect that dominates everything else. Measured at a855402 on gfx950, 1x MI350X, -n 1: the full suite takes 118 min cold and 10 min warm. 92% of a cold run is compilation, not test execution -- which is what makes a first run look like a ~50 min suite, and no marker can move it, since the compile happens in whichever lane runs first. It also looks exactly like a hang: there is no pytest-timeout here, and one AITER CK build sat for 36 minutes with no output during this measurement, distinguishable from a hung kernel only by looking inside the container for clang processes. Records that nothing runs `-m slow`, so marking retires a test. All three places that describe the marker -- pyproject.toml, CONTRIBUTING.md and CLAUDE.md -- now give footprint as the sole criterion rather than three different ones. The Essential Commands table no longer advertises `-m "not slow"` as the fast lane: it is within ~10 s of the full run. Figures carry arch, card count and -n because none of it transfers. gfx942 is unmeasured -- no usable image on the 2-GPU node, and the 8-card box runs ROCm 6.4.1 against a rocm7.2 torch wheel.
CONTRIBUTING.md documents a manual 'pytest -m slow' invocation, so the unqualified claim contradicted a command three lines above it. Also drops 'bespoke-compile' from the fast-path comment: with the use_softmax mark gone, '-m "not slow"' skips only the speculative-sampling cases.
The slow lane's one test is marked precisely because its worst case is ~15 GB, and tests/conftest.py:25 states that the HIP_VISIBLE_DEVICES pinning "does not re-scope *this* process" -- torch is imported before the variable is set, so every xdist worker allocates on the same card. `pytest -n auto -m "slow"` therefore put four concurrent 15 GB allocations on one device, with --reruns 2 retrying the OOM twice. The command now says -n 1, which is what the marker means. Also re-anchors the "~10 s" fast-lane delta: it describes the split this change produces, not the a855402 measurement it was written next to, where the fast lane still excluded the 343 cases unmarked here. Unmarking 343 sampling cases moves them into a regime the -n 1 timings did not cover, and this file's header records that high ROCm sample counts once triggered fatal HSA aborts. Measured rather than assumed: the three affected files at -n 4 on a single MI350X -- harsher than -n auto, since the workers share a card either way -- exit 0 in 55 s with no aborts.
2dc664e to
a3bff1b
Compare
Summary
Measured per-test runtime across the whole default selection, then corrected the
slowmarker and the suite timings in the docs to match what was measured. The marker was aimed at tests that are not slow, and the documented figures had been copied forward since May without re-measuring.The headline is that there was no runtime problem to fix: the full suite is 10 minutes warm. A run that feels like ~50 minutes is paying for JIT compilation, which no marker can move.
What changed
Tests
tests/rocm/test_sampling.py— droppedslowfrom the four 1M-trial frequency tests (11.2 s warm between them); kept it ontest_chain_speculative_samplingand corrected the stated reason.tests/rocm/test_logits_processor.py— droppedslowfromTestLogitsPipeCompilationHIP(27.1 s warm, 252 cases), and fixed a class docstring that claimed every test runs a sampling kernel —test_temperature_softmaxdoes not.tests/rocm/test_customize_prefill_use_softmax.py— droppedslow. This is the one that mattered: it is the onlyuse_softmax=falsecoverage insidetestpaths, so the marker had retired the regression test for the uninitialised-accumulator NaN fixed in fix(rocm): zero the persistent attention-sum accumulator #321, in exchange for 0.3 s.pyproject.toml— marker description now gives footprint as the sole criterion.Docs
CONTRIBUTING.md— replaced the ~7 / ~20 / ~13 min figures with measured cold and warm numbers, and recorded that nothing runs-m slow.CLAUDE.md— same criterion, plus the cold-cache warning; the Essential Commands table no longer advertises-m "not slow"as the fast lane.Measurement
Every test in the default selection, at
a85540294on gfx950 (1× MI350X, ROCm 10.0 / HIP 7.15, torch 2.12, aiter 0.1.20,-n 1), run cold then warm inside one container so the warm pass saw the cold pass's AITER build. Durations came from a per-test recorder on the xdist controller. No--reruns, so retries of known failures could not distort per-test numbers.Warm cost is concentrated in small files that compile or subprocess, not in the large parametrised ones:
test_aot.pytest_force_cta_tile_q.pytest_torch_compile.pytest_rope.pyArchitecture / design notes
Why nothing was added to the
slowlane, despite those three files being the slowest in the suite. No CI job, workflow or script invokes-m slow, and-m "not slow"is also whatscripts/amd_coverage.pydocuments. Marking therefore retires a test rather than deferring it. Those three files are the only coverage of the AOT packaging path,FLASHINFER_ROCM_FORCE_CTA_TILE_QandFLASHINFER_USE_TORCH_CUSTOM_OPSrespectively; 196 s off a 574 s suite does not buy that. Footprint is left as the only admissible reason to mark, which is why exactly one mark survives.Why case count is the wrong signal.
test_rope.pyis the largest matrix in the suite at 13,080 cases and costs 1.2 ms each. Marking it would have saved 16 seconds while removing the broadest shape coverage in the repo.The one surviving mark.
test_chain_speculative_sampling's worst case (batch 989, vocab 128256, 7 speculate tokens) holds four probability tensors live at once — 2 × 989×7×128256×4 B plus 2 × 989×8×128256×4 B, about 15 GB. The previous comment said ~4 GB, which is one tensor.-n autoruns several such workers together, so this is a footprint judgement, not a runtime one.Test plan
not slow+ 72slow= 36,838 total — cases moved, none vanished.a85540294on gfx950; 32,750 passed / 3,943 skipped / 5 failed, the failures pre-existing and unrelated.pre-commit run -agfx942 is unmeasured. The 2-GPU gfx942 node has no usable flashinfer image, and the 8-card box runs ROCm 6.4.1 against a torch 2.9.1+rocm7.2 wheel. Every wall-clock figure here is single-GPU gfx950 and is labelled as such in the docs. The marker change itself is arch-independent — it moves 343 cases into the default lane and removes none from it, so no architecture loses coverage.