Fix 14 GPU-only test failures on gfx942 - #14
Merged
Conversation
The suite was 18-failed/302-passed on MI300X while CI was green, because every one of these tests is gated on `requires_torch_cuda` / `device_count() == 0` and this fork's CI is CPU-only. None of the failures are ROCm-specific — they would fail identically on NVIDIA. Two distinct causes, both in the tests rather than in production code: 1. 12 evaluator tests passed an unresolved `BenchmarkConfig` straight to `Evaluator.evaluate()`, which expects a `ResolvedEvalConfig`. On `BenchmarkConfig` the tolerances are `Optional[float] = None`, so `compute_error_stats` hit `TypeError: '>' not supported between instances of 'Tensor' and 'NoneType'` at utils.py:113. Production is unaffected: both runners call `cfg.resolve_eval_config(definition)` first. The tests now do the same, at all 25 call sites across the three files. 2. `test_benchmark_with_mixed_results` asserted the pre-flashinfer-ai#379 flat trace layout. `add_traces` has written `traces/{author}/{op_type}/{name}.jsonl` since d3f798c ("Adapt codebase to author-based trace directory layout"), and the loader `rglob`s recursively, so writer and reader agree — only the test was stale. Verified the round trip: the file lands at traces/tester/op/ and `TraceSet.from_path` rediscovers both traces. CLAUDE.md documented the same stale path and is corrected to match. Not addressed here, deliberately: the 4 `test_mla_paged` failures are an amd-flashinfer coverage gap (`module 'flashinfer' has no attribute 'mla'`) and are left failing rather than skipped. A `tests/serve` ordering flake that appeared in the original run has not recurred in two full runs since; the evaluator tests previously errored mid-suite, which is the likely source of the state it tripped on. Verified on MI300X / gfx942, ROCm 7.2: 18 failed/302 passed -> 4 failed/316 passed, stable across two runs. CPU unchanged at 226 passed. pre-commit clean.
Self-review of the previous commit surfaced three things it left undone. - tests/bench/test_isolated_runner.py: the sweep fixed the 25 evaluate() call sites but missed compute_error_stats(), still handed a raw BenchmarkConfig. It passes today only because that test sets both atol and rtol explicitly; drop either and it reproduces the exact TypeError the sweep set out to remove. Build a ResolvedEvalConfig directly (no definition is in scope there to resolve against). - evaluators/evaluator.py: fix the class of bug, not just its instances. The annotation says ResolvedEvalConfig but Python does not enforce it, so a BenchmarkConfig duck-types far enough to fail deep inside compute_error_stats with "'>' not supported between instances of 'Tensor' and 'NoneType'" and no hint at the cause. Reject it at the boundary with a message naming the fix. - CLAUDE.md: the previous commit corrected the traces/ layout line and left its sibling solutions/ line hardcoding "baseline", which is one author value and not a fixed segment (docs/flashinfer-trace/validate.mdx:59 documents solutions/<author>/<op_type>/<definition>/). Also ignore GPU run artifacts: every suite run on gfx942 dumps a ~900MB root owned `core` from the MLA crashes, plus `.rocprofv3/`. Being untracked and root-owned, they make `git add -A` fail outright. Verified: guard raises the intended TypeError for the old call shape; GPU suite unchanged at 4 failed (MLA only) / 316 passed; CPU 226 passed; pre-commit clean.
There was a problem hiding this comment.
Pull request overview
Fixes GPU-only test failures by ensuring evaluator paths consume fully-resolved eval configs (matching production runners), updates the benchmark trace-path assertion to the author-scoped layout, and adds a defensive boundary check in Evaluator.evaluate() to fail fast when an unresolved config is passed.
Changes:
- Update bench tests to call
BenchmarkConfig.resolve_eval_config(definition)(and useResolvedEvalConfigdirectly where appropriate). - Fix
test_benchmark_with_mixed_resultsto assert the author-scoped trace layout (traces/{author}/{op_type}/{definition}.jsonl). - Add a runtime type guard in
Evaluator.evaluate()and update docs/ignore patterns for ROCm/GPU-run artifacts.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/bench/test_isolated_runner.py | Use ResolvedEvalConfig when calling compute_error_stats (matches function contract). |
| tests/bench/test_evaluator.py | Resolve BenchmarkConfig into ResolvedEvalConfig before calling evaluator entrypoints. |
| tests/bench/test_dsa_topk_indexer_evaluator.py | Resolve eval config before evaluation to avoid None tolerances. |
| tests/bench/test_dsa_sparse_attention_evaluator.py | Resolve eval config before evaluation to avoid None tolerances. |
| tests/bench/test_benchmark.py | Update trace-file assertion to author-scoped trace directory layout. |
| flashinfer_bench/bench/evaluators/evaluator.py | Add fast-fail TypeError when cfg is not a ResolvedEvalConfig. |
| CLAUDE.md | Fix dataset path documentation to author-scoped solutions/ and traces/ layouts. |
| .gitignore | Ignore GPU/rocprof artifacts (needs scoping adjustment per review comment). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
Fixes 14 of the 18 test failures on MI300X / gfx942. The suite was 18-failed/302-passed on GPU while CI stayed green, because every failing test is gated on
requires_torch_cuda/device_count() == 0and this fork's CI is CPU-only.None of these failures are ROCm-specific. Both root causes are in the tests, not in production code, and would fail identically on NVIDIA.
Root cause 1 — tests skipped a config-resolution step (12 failures)
There are two config types:
BenchmarkConfig(what you write; every eval field isOptionaland defaults toNone) andResolvedEvalConfig(what evaluators consume; every field populated).cfg.resolve_eval_config(definition)converts one to the other.The tests passed the unresolved
BenchmarkConfigstraight toEvaluator.evaluate(), which is annotated forResolvedEvalConfig. Python does not enforce annotations at runtime, so it ran until the tolerances were actually used:Production is unaffected —
isolated_runner.py:49andpersistent_runner.py:253both resolve first. The tests now do the same, at all 25 call sites across three files.Root cause 2 — a stale trace-path assertion (1 failure)
test_benchmark_with_mixed_resultsassertedtraces/{op_type}/{name}.jsonl.add_traceshas writtentraces/{author}/{op_type}/{name}.jsonlsince d3f798c ("Adapt codebase to author-based trace directory layout", flashinfer-ai#379). The code is right and the test was never migrated — verified both directions: the file lands attraces/tester/op/simple_add.jsonl, andTraceSet.from_pathrediscovers both traces because the loaderrglobs recursively.CLAUDE.mddocumented the same stale layout (and hardcodedsolutions/baseline/, wherebaselineis one author value) and is corrected to match.Preventing recurrence
Evaluator.evaluate()now rejects a non-ResolvedEvalConfigat the boundary with a message naming the fix, rather than failing 100 lines deeper with aTensor > NoneTypeerror that says nothing about config resolution. This is the only production change in the PR; correct callers are unaffected.Also ignores GPU run artifacts — each suite run on gfx942 leaves a ~900MB root-owned
corefrom the MLA crashes plus.rocprofv3/, which makegit add -Afail outright.Not fixed here
test_mla_pagedfailures remain red. They are an amd-flashinfer coverage gap (module 'flashinfer' has no attribute 'mla'), not something this PR can address, and are deliberately left failing rather than skipped so the gap stays visible. This PR does not make the GPU suite green.tests/serveordering flake present in the original run has not recurred in three full runs since. The 12 evaluator tests previously errored mid-suite, which is the likely source of the state it tripped on.Follow-ups this work surfaced (separate PRs)
flashinfer_bench/cli/main.py:111,155usetrace_set.workload; the field isworkloads.flashinfer-bench report mergedies withAttributeErroron any input.scripts/collect_stream.py:350,363still use the pre-Adapt codebase to author-based trace directory layout flashinfer-ai/flashinfer-bench#379 flat trace path, sopush_tracenever finds the file, logs a warning, and the pipeline reports success having uploaded no traces.trace_set.py:711interpolates rawsolution.authorinto an on-disk path with no sanitization, whilecli/main.py:147guards the analogous solutions path with_safe_path_segment.tests/compile tests/agent tests/integration/test_aiter_generator.py— it excludestests/bench, where 13 of these 18 failures lived. Worth widening once a runner is attached; note that addingtests/integration/flashinferwould go red on the MLA gap above.Test plan
docker/rocmcontainer: 18 failed / 302 passed → 4 failed / 325 passed, stable across runs (325 vs 316 earlier reflects ROCm: fix rocprofv3 region scoping (marker column) #12's new tests, merged in during this work).TypeErrorfor the old call shape.pre-commit run -a