Skip to content

[CI] run_benchmark: treat runtime pytest.skip as skipped, not failed#842

Merged
coderfeli merged 1 commit into
ROCm:mainfrom
jhinpan:fix/bench-skip-not-fail
Jul 14, 2026
Merged

[CI] run_benchmark: treat runtime pytest.skip as skipped, not failed#842
coderfeli merged 1 commit into
ROCm:mainfrom
jhinpan:fix/bench-skip-not-fail

Conversation

@jhinpan

@jhinpan jhinpan commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Problem

test (linux-flydsl-*) has been red on main and on multiple open PRs (#785, #801, …). The test suites actually pass — the failure is entirely in the benchmark step.

scripts/run_benchmark.sh runs each kernel test as a plain script (python3 tests/kernels/test_*.py), not through pytest. When one of those files reaches a pytest.skip() / pytest.importorskip() at runtime, the skip raises an uncaught Skipped exception, the process exits non‑zero, and the op was counted as failed → the whole job (and CI) goes red.

Two independent triggers of the same flaw:

  • mla (CDNA runners, and main itself): upstream aiter dropped/renamed hk_mla_decode_fwdhk_mla_v32_decode_fwd. test_mla_decode.py guards the import with pytest.skip(allow_module_level=True) (added in [Fix]: mla_decode test aborts whole collection on aiter API drift #840), which correctly skips under pytest but hard‑fails when run as a script.
  • layernorm (all single‑GPU runners incl. navi): test_layernorm.py's multi‑GPU test calls pytest.skip("needs >=2 GPUs").

So main and several PRs were failing for purely environmental reasons, not because of their own changes.

Fix

Add a small _fail_or_skip helper that downgrades a non‑zero benchmark exit to skipped when the log contains a Skipped: marker — mirroring the grep -q "Skipped:" handling this script already uses for hgemm/fp8. Route the plain failure branches through it: softmax, layernorm, rmsnorm, flash_attn, mla, gemm, gemm_async, hgemm, moe, moe_w4a16, RDNA WMMA.

  • Genuine failures still count and still dump their log tail (unchanged behavior).
  • The summary now reports a Skipped: count; the job still fails iff FAIL_COUNT > 0.
  • The arch‑specific blocks (fp8_8wave_rowscale, gemm_fp4, moe_fp4, moe_a8w4) keep their existing bespoke skip detection.

Scope / non‑goals

Test plan

  • bash -n and sh -n pass (POSIX‑sh clean; script is #!/bin/sh).
  • Unit‑tested the helper: a log with Skipped:SKIP_COUNT, a log with AssertionErrorFAIL_COUNT.
  • CI: mla/layernorm should now show skip and the benchmark summary should end Failed: 0 → job green.

Made with Cursor

run_benchmark.sh invokes each kernel test as a plain script (python3
tests/kernels/test_*.py), not through pytest. When such a file reaches a
pytest.skip()/pytest.importorskip() at runtime -- e.g. a single-GPU runner
hitting the layernorm multi-GPU test ("needs >=2 GPUs"), an arch guard, or the
mla_decode test hitting an aiter API drift (hk_mla_decode_fwd removed) -- the
skip raises an uncaught Skipped exception, the script exits non-zero, and the
op was counted as "failed", turning the whole benchmark job (and thus CI) red.

The pytest suite already tolerates these (they show up as skips), so only the
benchmark step was going red; this made main and several unrelated PRs fail for
purely environmental reasons.

Add a _fail_or_skip helper that downgrades a non-zero benchmark exit to
"skipped" when its log contains a "Skipped:" marker (mirroring the existing
grep -q "Skipped:" handling already used for hgemm/fp8), and route the plain
failure branches (softmax, layernorm, rmsnorm, flash_attn, mla, gemm,
gemm_async, hgemm, moe, moe_w4a16, RDNA WMMA) through it. Genuine failures
still count and dump their log tail. Report a Skipped count in the summary;
the job still fails iff FAIL_COUNT > 0.

The arch-specific blocks (fp8_8wave_rowscale, gemm_fp4, moe_fp4, moe_a8w4)
keep their existing bespoke skip detection.
Copilot AI review requested due to automatic review settings July 13, 2026 23:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderfeli
coderfeli merged commit a855951 into ROCm:main Jul 14, 2026
8 checks passed
@jhinpan
jhinpan deleted the fix/bench-skip-not-fail branch July 14, 2026 20:16
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.

3 participants