Point the Gemma benchmark routines at a capability that exists - #369
Merged
Conversation
benchmarks/routines/rocm/support.py mapped gemma_rmsnorm and gemma_fused_add_rmsnorm to the capability op "layernorm", which arch_caps.py no longer declares -- it was removed once it turned out to have no ROCm kernel behind it. capability_available() therefore answered False and both routines filtered to no backends, so #364 shipped 21 of the 23 routines it claimed. Measured on gfx942 at c333f20: capability_available(d, "layernorm", "hip") -> False capability_available(d, "gemma_rmsnorm", "hip") -> True rocm_supported_backends("gemma_rmsnorm", d) -> [] and through the runner, "[ERROR] No backends to test. Exiting." against a [PERF] row after the fix. Neither change was wrong alone and git merged both cleanly, because they touch different files. What is missing is not a test: test_filter_only_offers_backends_the_cli_accepts already fails on this, for both routines -- verified by restoring the key. It is a *lane*. tests/rocm/test_benchmark_harness.py is skipif(not IS_HIP) and no workflow references it, so the suite that catches this runs only on a maintainer's ROCm box, which is how the removal merged. So the guard goes in tests/rocm/test_arch_caps.py, which arch-caps-conformance.yml runs on every pull request with no GPU. That lane has no torch, and support.py reaches flashinfer, so the registry is parsed with ast rather than imported. The regex scan already in that file cannot cover it either: support.py passes `op` as a variable, so the strings sit in a dict literal and never appear at a call site. Two smaller review items from the same PR: bench_block_sparse_attention.py recorded kind, seq_len, head counts, density and num_blocks per row but not --seed. Recording it alone would have overstated what it pins: --seed reached only _block_mask's local generator, while every q/k/v came from the unseeded global RNG, so --accuracy could not be reproduced from a row. main() now seeds the global RNG too. testlist_rocm.txt's norm section was headed "bf16 and fp16" while all 7 of its commands pass --input_dtype bfloat16. Corrected the heading rather than adding fp16 cases, and recorded why in the file: vec_size is gcd(16/sizeof(T), d) and both are 2-byte, so fp16 would re-run the same path for numerical noise alone. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The capability fix, regression guard, seed handling, and documentation update are consistent and complete.
Pull request overview
Fixes ROCm Gemma benchmark capability routing and improves benchmark validation and reproducibility.
Changes:
- Maps Gemma routines to the declared capability.
- Adds a GPU-free registry conformance test.
- Records and applies benchmark seeds; corrects norm documentation.
File summaries
| File | Description |
|---|---|
tests/rocm/test_arch_caps.py |
Validates registry capability references. |
benchmarks/routines/rocm/support.py |
Corrects Gemma capability mappings. |
benchmarks/rocm/testlist_rocm.txt |
Clarifies BF16-only coverage. |
benchmarks/rocm/bench_block_sparse_attention.py |
Seeds inputs and records the seed. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
11 tasks
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.
Follow-up to #364, addressing the three review comments that were still open when it merged.
Summary
#364 claimed to make 23 benchmark routines runnable on ROCm. It made 21: both Gemma routines were mapped to a capability op that
arch_caps.pyno longer declares, so they filtered to no backends and produced no rows. This fixes that, adds the guard that would have caught it, and clears two smaller items from the same review.What changed
benchmarks/routines/rocm/support.py—gemma_rmsnormandgemma_fused_add_rmsnormnow reference the capability opgemma_rmsnorm; they referencedlayernorm, which was removed fromarch_caps.pyonce it turned out to have no ROCm kernel behind it.tests/rocm/test_arch_caps.py—test_benchmark_registry_ops_are_declared, asserting every op the registry references has ahiprow. It lives here becausearch-caps-conformance.ymlruns this file on every PR with no GPU; the registry is parsed withastbecause that lane has no torch.benchmarks/rocm/bench_block_sparse_attention.py— record--seedwith the other case parameters, and actually seed the global RNG:--seedreached only the block mask, while every q/k/v came from the unseeded generator, so recording it alone would have overstated what a row pins.benchmarks/rocm/testlist_rocm.txt— the norm heading claimed "bf16 and fp16"; all 7 of its commands passbfloat16.Architecture / design notes
The missing thing was a CI lane, not a test.
test_filter_only_offers_backends_the_cli_acceptsalready fails on this defect for both Gemma routines — verified by restoring the key. Buttests/rocm/test_benchmark_harness.pyisskipif(not IS_HIP)and no workflow references it, so the suite that catches this runs only on a maintainer's ROCm box. That is how thelayernormremoval merged. The new guard therefore goes in the one lane that runs everywhere, and the on-hardware test keeps its stronger check (a row can exist and still be gatedUNSUPPORTEDon an arch, which a declaration scan cannot see).Why
astand not the regex scan already in that file.support.pypassesopas a variable tocapability_available, so the op strings live in a dict literal and never appear at a call site fortest_every_op_the_library_asks_for_is_declaredto match.Heading corrected rather than adding fp16 cases.
vec_sizeisgcd(16/sizeof(T), d)and fp16 and bf16 are both 2-byte, so fp16 cases would exercise the same code path and differ only in numerical noise — coverage in name only, paid for on every testlist run.Benchmark results
No performance change: this makes two routines run that previously exited before timing anything, and adds a column to a CSV. For the record, on gfx942 at
c333f2003both now measure (batch 989, hidden 4096, bf16):gemma_rmsnormgemma_fused_add_rmsnormTest plan
pytest --noconftest tests/rocm/test_arch_caps.py): with thelayernormkey restored it fails naminglayernorm; with the fix the file's 85 cases pass.[ERROR] No backends to test. Exiting.; with the fix, a[PERF]row for both Gemma routines.tests/rocm/test_benchmark_harness.py— exit 0 on gfx942 after removing the duplicate guard. With the bug restored it fails ontest_filter_only_offers_backends_the_cli_accepts[gemma_rmsnorm]and[gemma_fused_add_rmsnorm], which is the evidence that the pre-existing test already covered this.--csvoutput carries aseedcolumn, verified by parsing the written file.pre-commit runon the changelist — clean, nothing reformatted./code-review xhigh(the levelreview-level.shcomputes for this changelist).Measured on gfx942 only. The fix is a capability-table lookup and a CSV field, neither of which is architecture-dependent, and the guard test does not touch a GPU.