ROCm: fix rocprofv3 region scoping (marker column) by demandal25 · Pull Request #12 · AMD-Ecosystem/flashinfer-bench

demandal25 · 2026-07-31T03:31:34Z

Summary

rocprofv3 region scoping has never worked. _region_window read a Name column that rocprofv3's marker_api_trace.csv does not have — the roctx message is written to Function — so the region was never matched and every profile silently widened to every kernel in the process while its header still claimed to be scoped to the region.

Found by running the profiler for real in the docker/rocm container (ROCm 7.2, gfx942) rather than reading the code. Confirmed on the host's ROCm 6.4.1 too: both releases use Function, neither emits Name.

Impact

On a minimal probe (torch.randn outside the range, one matmul inside), the report listed 3 dispatches and named the region. Two of them — a torch.randn RNG kernel and a setup buffer fill — ran outside the marked range, and the slowest entry, printed first, was setup work rather than the profiled kernel. After the fix the same probe reports 2, correctly excluding the RNG kernel. Since the unscoped report is textually indistinguishable from a good one, an agent optimizes a kernel the solution never ran, with no signal anything is wrong.

What changed

flashinfer_bench/agents/rocprof.py — look the region up in Function, falling back to Name. The lookup deliberately does not filter on Domain: it is MARKER_CORE_API on 6.4.1 but MARKER_CORE_RANGE_API on 7.2, so matching it would re-break scoping on one of the two.
rocprof.py header — stop asserting scoping that did not happen. When the marker is absent, or matches no dispatch, the header now reads ALL kernels — region '…' not found in the marker trace.
rocprof.py launch geometry — Grid_Size_* is a work-item count under HSA, not CUDA gridDim. Printed as grid beside block it overstated the launch by the workgroup size (262144 work-items reads as 262144 workgroups when it is 1024×256). Relabelled items/wg.
docker/rocm/validate_rocprof.py — it printed PASS on the broken report because it only asserted "us |" appeared. It now fails unless the header reports real region scoping, which is the property the script exists to prove.
tests/agent/test_rocprof.py — the fixtures hand-wrote a Name header, so they encoded the very bug they were meant to guard. They now use the real ROCm 7.2 layout captured from the container, plus regression tests for the unscoped-header path and the geometry labels.
scripts/linting.sh — ran ruff check . --fix, which in CI repairs what it can, exits 0, and discards the repairs with the runner, so every auto-fixable violation reached the branch unnoticed. Measured: --fix exits 0 ("1 fixed, 0 remaining"), --no-fix exits 1. Switched to --no-fix; the tree passes today so this lands green and only bites on future violations. Included here rather than split out because it is the same defect as the validate_rocprof.py change above — a check that could not fail.

Why seven rounds of review missed it

Every test and every reviewer read the code against fixtures that shared the code's wrong assumption. Nothing had ever been run against real rocprofv3 output. The failure mode is silent — no error, no crash, just a quietly over-broad report — so a green suite proved nothing.

Test plan

Run inside the docker/rocm container (ROCm 7.2.0, torch 2.9.1+rocm7.2, gfx942):

python docker/rocm/validate_rocprof.py → PASS — rocprofv3 profiled kernels, scoped to the region (fails on the pre-fix code with the strengthened check)
python docker/rocm/validate_p0.py → 8/8 checks passed
pytest tests/agent -q → 23 passed, 11 skipped
End-to-end probe under real rocprofv3: _region_window returns a real window instead of None; out-of-region kernel correctly excluded
pre-commit run -a

Note: pytest -n auto -m "not slow" on amd-integration has 18 pre-existing failures in the container (tests/bench/test_evaluator.py, test_dsa_*, tests/integration/flashinfer/test_mla_paged.py, test_isolated_runner, test_benchmark). They are untouched by this PR and unrelated to agents/ — CI only runs CPU-only on ubuntu, so they are invisible there. Worth triaging separately.

Copilot

Pull request overview

Fixes rocprofv3 region scoping by matching the roctx marker message in the correct marker_api_trace.csv column (Function, with fallback to Name), and makes the generated report header/geometry labels accurately reflect whether scoping occurred and what the launch dimensions represent. This improves correctness of profiling output consumed by agent tooling and strengthens validation/tests to prevent silent unscoped profiles.

Changes:

Fix _region_window to locate the profiled region via Function (fallback Name) and update report header to explicitly indicate when scoping fails.
Relabel launch geometry in the report from grid/block to items/wg to match HSA semantics for Grid_Size_*.
Update validation script and unit tests to use real ROCm 7.2 CSV schema and to assert true region scoping.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`flashinfer_bench/agents/rocprof.py`	Correct marker column lookup for region correlation; improve report header truthfulness; relabel geometry to `items/wg`.
`tests/agent/test_rocprof.py`	Update fixtures to real `marker_api_trace.csv` schema and add regression tests for scoping/header/geometry.
`docker/rocm/validate_rocprof.py`	Strengthen validation to fail when reports are unscoped and require the header to indicate true region scoping.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

flashinfer_bench/agents/rocprof.py:120

_region_window() is intended to fall back from the rocprofv3 "Function" column to the legacy "Name" column, but the current implementation only inspects the first truthy column. If a rocprofv3 build ever emits both columns (or a wrapper script adds one) and the roctx message is in "Name" while "Function" is non-empty but unrelated, region scoping will still silently fail.

To make the fallback robust, check all candidate columns for PROFILE_REGION instead of selecting the first populated one.

    for r in rows:
        name = next((r[c] for c in _MARKER_NAME_COLUMNS if r.get(c)), "")
        if PROFILE_REGION in name:

tests/agent/test_rocprof.py:143

There is no test case for the intended "Function"→"Name" fallback when both columns are present but only "Name" contains the roctx region string. Given the original regression was caused by a schema mismatch, adding this synthetic schema test would guard against future rocprofv3 variants and also catches the current fallback-selection bug.

demandal25 · 2026-07-31T04:21:48Z

Addressed the two suppressed findings from the 03:53 review in 70d9051. Both were right, and the first was a real bug in this PR's own fix.

Fixed — _region_window took the first populated column, not the first column containing the region. next((r[c] for c in _MARKER_NAME_COLUMNS if r.get(c)), "") returns Function's value whenever it is non-empty, so a build that fills Function with the API name (roctxRangePushA) and puts the roctx message in Name would match the wrong column and silently lose scoping — reintroducing exactly the defect this PR exists to remove, one layer down. Now every candidate column is searched. Added the regression test the second finding asked for (test_region_found_when_another_column_is_populated_but_unrelated), and confirmed it fails against the previous implementation before keeping it.

Fixed — validate_rocprof.py searched the whole report body for "ALL kernels" and the region string. Reported inline as well; the checks now read the header line only.

Re-validated in the docker/rocm container (ROCm 7.2, gfx942) after rebasing onto ed36ab6: validate_rocprof.py → PASS — scoped to the region; validate_p0.py → 8/8; pytest tests/agent → 24 passed, 11 skipped; scripts/linting.sh → clean under the new --no-fix.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/linting.sh:15

ruff check runs in non-fixing mode by default, so passing --no-fix is redundant and can reduce portability if the flag isn’t supported in some ruff versions. You can keep the intended “fail CI, don’t auto-rewrite” behavior by simply omitting --fix.

# --no-fix, NOT --fix. With --fix ruff repairs what it can and exits 0, so in CI the violations
# are silently fixed in the ephemeral checkout, the job passes, and the repairs are discarded with
# the runner — every auto-fixable violation lands on the branch unnoticed. Fail instead; run
# `ruff check . --fix` locally to repair.
ruff check . --no-fix

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

flashinfer_bench/agents/rocprof.py:143

_region_window collapses multiple matching marker spans into a single window by taking min(start), max(end). If rocprofv3 produces multiple disjoint ranges (e.g., multi-process output where each process has its own roctx range), this “bounding box” can incorrectly include kernels that ran between the ranges while the header still claims region scoping succeeded. Consider merging spans and only returning a single window when the union is contiguous; otherwise return None so the report is explicitly marked unscoped (or handle a span union explicitly).

    # Span the widest extent rather than trusting the first match. _read_csv merges every marker
    # CSV, so several rows can carry the message: a second process's range, or an instantaneous
    # roctxMark-style record whose start == end. Taking the first would hand back a window that
    # contains no dispatch, and the report would silently fall back to every kernel again.
    return min(s for s, _ in spans), max(e for _, e in spans)

flashinfer_bench/agents/rocprof.py:228

The occupancy-column NOTE detection only checks the first selected row’s dict keys. Since _read_csv can merge rows from multiple CSVs (potentially with different headers/column counts due to truncation or per-process differences), this can incorrectly print “no occupancy columns” even when other rows do contain VGPR/LDS fields. It’s safer to scan all selected rows for the presence of at least one occupancy key.

    # Say once why the occupancy fields are blank, instead of leaving N lines of bare "?" that read
    # as "this kernel has no registers" rather than "this profiler build does not report them".
    if selected and not any(k in selected[0][0] for k in ("VGPR_Count", "LDS_Block_Size")):
        lines += [

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (4)

flashinfer_bench/agents/rocprof.py:174

After changing _region_window to return multiple spans (union semantics), the in-region selection should match against any span, and the “why” string should treat “no spans found” vs “spans found but matched no dispatch” separately. As written today (single min/max window), multi-process runs can include out-of-region kernels that happen between two disjoint spans.

    window = _region_window(out_dir)
    all_kernels = [(r, end - start) for r, start, end in parsed]
    in_region = (
        [(r, end - start) for r, start, end in parsed if window[0] <= start and end <= window[1]]
        if window is not None

flashinfer_bench/agents/rocprof.py:143

_region_window() collapses all matching marker spans into a single (min_start, max_end) window. In multi-process runs this can incorrectly include warmup/setup kernels that occur between disjoint per-process regions (because they still fall between the global min/max), partially reintroducing the “scoped but actually unscoped” failure mode.

Consider returning all matching spans and treating the effective region as the union of spans (kernel is “in region” if it falls within any span), rather than bridging gaps via min/max.

This issue also appears on line 170 of the same file.

    # Span the widest extent rather than trusting the first match. _read_csv merges every marker
    # CSV, so several rows can carry the message: a second process's range, or an instantaneous
    # roctxMark-style record whose start == end. Taking the first would hand back a window that
    # contains no dispatch, and the report would silently fall back to every kernel again.
    return min(s for s, _ in spans), max(e for _, e in spans)

flashinfer_bench/agents/rocprof.py:229

The occupancy-note condition only inspects the first selected row's dict keys. Since _read_csv() merges per-process CSVs, and different files could theoretically have different headers/column sets, this can emit (or suppress) the NOTE incorrectly depending on which row sorts first. Checking across all selected rows avoids misleading output.

    # Say once why the occupancy fields are blank, instead of leaving N lines of bare "?" that read
    # as "this kernel has no registers" rather than "this profiler build does not report them".
    if selected and not any(k in selected[0][0] for k in ("VGPR_Count", "LDS_Block_Size")):
        lines += [

scripts/linting.sh:15

ruff check already defaults to not applying fixes. Using --no-fix is redundant and can make this script depend on a specific Ruff CLI version/flag set unnecessarily. Dropping the flag keeps the intended behavior (fail CI on violations) while maximizing compatibility.

# --no-fix, NOT --fix. With --fix ruff repairs what it can and exits 0, so in CI the violations
# are silently fixed in the ephemeral checkout, the job passes, and the repairs are discarded with
# the runner — every auto-fixable violation lands on the branch unnoticed. Fail instead; run
# `ruff check . --fix` locally to repair.
ruff check . --no-fix

Region correlation never worked. _region_window looked up a "Name" column that rocprofv3's marker_api_trace.csv does not have — the roctx message is written to "Function". r.get("Name") was therefore always None, the region was never matched, and every report silently fell back to listing every kernel in the process while its header still claimed to be scoped. Verified in the docker/rocm container (ROCm 7.2, gfx942) against real rocprofv3 output, and on the host (ROCm 6.4.1): both write the roctx message to "Function" and neither emits a "Name" column. On a minimal probe the profile went from 3 dispatches to 2 once fixed, correctly dropping the torch.randn RNG kernel that runs outside the marked range. Note the "Domain" value differs across releases (MARKER_CORE_API on 6.4.1, MARKER_CORE_RANGE_API on 7.2), so the lookup deliberately matches on the message column only; "Name" is kept as a fallback in case a build differs. Also: - The header no longer claims scoping it does not have. When the marker is not found, or matches no dispatch, it now says "ALL kernels — region ... not found in the marker trace" instead of naming the region. Silent widening is what makes this class of bug undiagnosable: the report reads identical either way, and the extra dispatches (the runner's warmup, JIT and setup work) sort to the top by duration, so an agent optimizes a kernel the solution never ran. - Grid_Size_* is a work-item count under HSA, not CUDA gridDim. Printing it as "grid" beside "block" overstated the launch by the workgroup size (262144 work items reads as 262144 workgroups when it is 1024). Relabelled "items"/"wg". Why no test caught this: the fixtures hand-wrote a "Name" header, so they encoded the bug they were meant to guard. They now use the real ROCm 7.2 layout captured from the container, plus regression tests for the unscoped header and the launch-geometry labels. docker/rocm/validate_rocprof.py passed on the broken report because it only asserted "us |" was present. It now fails unless the header reports real region scoping — which is the property the script exists to prove.

scripts/linting.sh ran `ruff check . --fix`. In CI that repairs whatever it can in the ephemeral checkout and exits 0, so the job goes green and the repairs are thrown away with the runner — every auto-fixable violation reaches the branch unnoticed. Measured: ruff check --fix -> exit 0, "Found 1 error (1 fixed, 0 remaining)" ruff check --no-fix -> exit 1, "Found 1 error" Same defect class as the validate_rocprof.py change in this branch: a check that could not fail. Locally `ruff check . --fix` still repairs. The tree passes with --no-fix today, so this lands green and only bites on future violations.

- _region_window took the first *populated* candidate column, not the first column containing the region. A build that fills "Function" with the API name (roctxRangePushA) and the message into "Name" would match the wrong column and silently lose scoping — reintroducing this PR's own bug one layer down. Now every candidate column is searched. Regression test added; verified failing against the previous implementation. - validate_rocprof.py searched the whole report body for "ALL kernels" and the region string. Kernel names appear verbatim there, so a mangled symbol could flip the verdict either way. The checks now read the header line only.

A /code-review pass over this branch (the step I skipped before opening it) turned up defects in the fix itself, several of the same class it was written to remove. Correctness: - _format_kernel_report raised TypeError instead of returning an "ERROR:" string when csv.DictReader padded a short row with None: f"{None:>4}" is a hard error, and Kernel_Name was the only guarded field. Reproduced, then routed all eight fields through a field() coercion. Regression test added. - _region_window returned the FIRST matching marker row. A zero-width roctxMark-style record, or a row from another process (_read_csv merges every marker CSV), yields a window containing no dispatch — silently unscoping the report exactly as before. It now spans min(start)/max(end) over all matches. - scope and selected were two pieces of one decision joined by statement order: the label was computed, then the selection was overwritten beneath it. Correct as written, but hoisting the fallback three lines up would have made the header lie again. They are now assigned together in one branch. Honesty of the report and its checks: - The header now carries "(N of M)" when scoped, so filtering is visible in band. Nothing previously distinguished a correctly scoped report from a degenerate window that happened to contain everything. - validate_rocprof.py had an unreachable branch: every scope string contains "region '<X>'" (the failure ones read "ALL kernels — region '<X>' not found..."), so the `elif ... not in header` could never fire for a real report. On the ERROR path it fired anyway and printed three FAIL reasons for one cause. Scoping checks are now skipped when the tool errored. - Dropped a comment I added claiming "kernel names appear verbatim in the body"; rocprof.py truncates them to name[:77]. Third comment in this file's history asserting behaviour the code does not have. - The module docstring still promised "grid/block" after the items/wg relabel and advertised occupancy unconditionally. It now states the ROCm version split, and the report emits one NOTE when the trace carries no occupancy columns instead of N lines of bare "?". Coverage and gates: - Kernel-trace fixtures used an invented three-column header no rocprofv3 emits — the marker-CSV lesson applied to only half the file. They now use the real ROCm 7.2 layout via a _kernel_row helper. - Added the untested "window matched no dispatch" branch, reachable on hardware when a kernel's End_Timestamp lands after the host-side roctx pop. - UNSCOPED_MARKER joins PROFILE_REGION in _profiling.py. The header wording was re-derived in the validator and three tests, so rewording it would have silently disarmed the check that detects unscoped reports. - docker/rocm/validate_rocprof.py was referenced nowhere: not the README, not the rocm-setup skill, not CI. The only check that sees real rocprofv3 output ran only if a human remembered it. Wired into rocm_gpu_test.yaml. - ruff's include covered only flashinfer_bench/ and tests/, so the --no-fix hardening in this PR could not see docker/rocm/validate_rocprof.py, a file this PR edits. docker/ is now linted (it is clean). scripts/ stays excluded: 14 pre-existing violations, its own cleanup. Verified in the docker/rocm container (ROCm 7.2, gfx942): validate_rocprof reports "region '...' (9 of 20)" and PASSes, validate_p0 8/8, tests/agent 26 passed, scripts/linting.sh clean.

…idator CLAUDE.md requires updating .claude/skills/*.md when a skill's procedure changes, and this PR changed the profiler's output format. rocm-benchmark was teaching agents the format the fix just removed. - It said the rocprofv3 agent was unported ("Porting the profiling agent (agents/ncu.py) ... is tracked in ROCM_PORT_PLAN.md §3.10"). It has shipped; an agent reading that would hand-roll rocprofv3 invocations instead of calling flashinfer_bench_run_rocprof, and would not know the roctx region name. - It described the per-kernel line as "VGPR/SGPR/LDS/grid". The report now emits items/wg, and "grid" was precisely the misreading the relabel removed — Grid_Size_* is work-items, so reading it as gridDim overstates the launch by the workgroup size. - Added how to read the header: "(N of M)" proves filtering happened, and a header starting "ALL kernels" means correlation failed and the report covers the whole process, so the top entry is probably warmup or setup rather than the kernel under test. - Recorded that occupancy columns exist on ROCm 7.x but not 6.4.x, and that the marker message lives in "Function" rather than "Name". rocm-setup now lists validate_rocprof.py alongside validate_p0.py, with why it cannot be replaced by unit tests.

docs/api/rst/agents.md autodoc'd the NCU pair and omitted the rocprof tools entirely, so on a ROCm-only fork the public API page advertised a profiler that cannot run here and hid the one that does. It also described the package as running "NVIDIA Nsight Compute, Compute Sanitizer". Adds the rocprof pair first, keeps the NCU entries (still importable for upstream parity) with a note that get_all_tool_schemas exposes rocprof rather than NCU on this fork and that the NCU tools report a missing executable on AMD. Verified every autodoc'd name is exported from flashinfer_bench.agents.

Copilot caught a regression I introduced. The previous commit fixed _region_window taking the *first* matching marker row by collapsing all matches to (min start, max end) — but that is a bounding box, and _read_csv merges every marker CSV, so a multi-process run has one span per process. Two disjoint regions bridged into one window silently readmit every kernel that ran between them, while the header reports success: the same "scoped but actually unscoped" failure this PR exists to remove, reintroduced one layer down for the second time. _region_window becomes _region_spans and returns every matching span; a dispatch is in-region if it falls inside ANY span. Regression test added and verified failing against the bounding-box version ("between_spans" was included). Also scan every selected row for the occupancy columns rather than only the first: merged per-process CSVs can carry different headers, so keying off one row emitted or suppressed the NOTE by luck of sort order. Container-verified (ROCm 7.2, gfx942): validate_rocprof PASS "(9 of 20)", validate_p0 8/8, tests/agent 27 passed, lint clean.

demandal25 · 2026-07-31T19:48:33Z

Addressed the suppressed findings from the 05:13 and 19:08 reviews in 9500815. The first was a regression I introduced while fixing an earlier finding, and it is the second time in this PR that a fix reintroduced the original defect one layer down.

Fixed — the min/max window was a bounding box. The previous commit replaced "take the first matching marker row" with (min start, max end) over all matches. But _read_csv merges every marker CSV, so a multi-process run has one span per process, and collapsing two disjoint regions into one window silently readmits every kernel that ran between them — while the header reports scoping succeeded. That is exactly the "scoped but actually unscoped" failure this PR exists to remove.

_region_window is now _region_spans and returns every matching span; a dispatch is in-region if it falls inside any span. Regression test added and confirmed failing against the bounding-box version (a between_spans kernel was wrongly included, now correctly excluded, (2 of 3)).

Fixed — the occupancy NOTE inspected only selected[0]. Merged per-process CSVs can carry different headers, so keying off one row emitted or suppressed the note by luck of sort order. It now scans every selected row.

Also in this push: rebased onto e2bc07e (#13), resolving two conflicts in favour of that PR's improvements — the hardened pytest --reruns 2 -m "not slow" invocation in the GPU workflow, and the python3 spelling in the rocm-setup skill — while keeping the validate_rocprof.py line this PR adds. Also documents the rocprof tools in docs/api/rst/agents.md, which still autodoc'd only the NCU pair, and corrects the rocm-benchmark skill, which described the report as VGPR/SGPR/LDS/grid and claimed the rocprof agent was unported.

Container-verified (ROCm 7.2, gfx942): validate_rocprof → PASS — scoped to the region, header region 'flashinfer_bench_profile' (9 of 20); validate_p0 8/8; tests/agent 27 passed; scripts/linting.sh clean.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

flashinfer_bench/agents/rocprof.py:144

_region_spans promises spans are returned "newest last", but it currently returns spans in _read_csv order (lexicographic filename order, then file row order), which isn’t guaranteed to be chronological. Sorting by Start_Timestamp before returning makes the docstring true and avoids misleading future callers.

            # than silently giving up on region scoping.
            continue

    return spans

scripts/linting.sh:15

--no-fix isn’t needed here (this repo’s pyproject.toml doesn’t set tool.ruff.fix = true), and it can reduce compatibility if the environment already has an older ruff on PATH (this script only installs ruff when it’s missing). ruff check . already fails on violations without attempting fixes.

# --no-fix, NOT --fix. With --fix ruff repairs what it can and exits 0, so in CI the violations
# are silently fixed in the ephemeral checkout, the job passes, and the repairs are discarded with
# the runner — every auto-fixable violation lands on the branch unnoticed. Fail instead; run
# `ruff check . --fix` locally to repair.
ruff check . --no-fix

The docstring said spans are returned "newest last". They are returned in _read_csv order — lexicographic filename, then row order — which is not chronological. Fourth comment in this file's history asserting behaviour the code does not have. Copilot suggested sorting by Start_Timestamp to make the claim true. Deleting the claim is better: both consumers are order-independent (an any() membership test and an emptiness check), so sorting would be work no caller asks for. The docstring now says order is unspecified and why. Also records why scripts/linting.sh passes --no-fix explicitly rather than relying on ruff's default: pyproject sets no [tool.ruff] fix, so a bare `ruff check .` does not fix today — but adding `fix = true` for local convenience would silently restore the masking the flag exists to stop.

demandal25 · 2026-07-31T20:40:31Z

Addressed the two suppressed findings from the 19:52 review in dd57000.

Fixed — _region_spans docstring claimed "newest last". It returns _read_csv order (lexicographic filename, then row order), which is not chronological. That is the fourth comment in this file's history asserting behaviour the code does not have.

I took a different remedy than suggested: rather than sorting by Start_Timestamp to make the claim true, I deleted the claim. Both consumers are order-independent — an any(...) membership test and an emptiness check — so sorting would be work no caller asks for. The docstring now states that order is unspecified, and why.

Won't fix — --no-fix in scripts/linting.sh stays. The premise is correct: pyproject.toml sets no [tool.ruff] fix, so a bare ruff check . would not auto-fix today. But explicitness is the point — if anyone later adds fix = true for local convenience, a bare ruff check . silently returns to the CI-masking behaviour this change exists to stop, and the failure mode is invisible (green CI, violations on the branch). The compatibility concern does not apply either: --no-fix is available at the repo's own declared floor of ruff>=0.1.0, so any ruff satisfying the project's constraint supports it. I have added a comment recording this reasoning next to the flag.

Verified: ruff check . --no-fix clean, bash -n scripts/linting.sh OK, tests/agent/test_rocprof.py 16 passed.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

flashinfer_bench/agents/rocprof.py:237

The occupancy-note gate only checks for key presence (e.g. "VGPR_Count" in the row), but csv.DictReader can populate missing trailing fields as None. In that case the key exists, the report prints ? for every kernel, but the NOTE is suppressed even though the data is effectively absent. Consider gating on whether any selected row has a non-placeholder value for the occupancy fields (using the existing field() helper), so partially-flushed/short rows still get the explanatory NOTE.

    # Say once why the occupancy fields are blank, instead of leaving N lines of bare "?" that read
    # as "this kernel has no registers" rather than "this profiler build does not report them".
    # Scan every selected row, not just the first: _read_csv merges per-process CSVs that can
    # carry different headers, so keying off one row emits or suppresses the note by luck of sort
    # order.
    if selected and not any(k in r for r, _ in selected for k in ("VGPR_Count", "LDS_Block_Size")):

The note fired only when the occupancy keys were absent from the row dict. But csv.DictReader creates the key with None for a short row, so a CSV whose header merely declares VGPR_Count suppressed the explanation while every line still printed "?" — the reader gets bare placeholders and no reason for them. Reproduced with the fixture from test_short_rows_do_not_raise, which declares VGPR_Count and supplies a short row: "VGPR ?" on the line, no NOTE. The gate now asks field() whether anything rendered as a real value, so it matches exactly what the reader sees. Both directions are tested: the note appears for placeholder-only output, and stays away when ROCm 7.x populates the columns. Container-verified (ROCm 7.2): validate_rocprof still PASSes "(9 of 20)" with no spurious NOTE; tests/agent 28 passed.

demandal25 · 2026-07-31T20:55:20Z

Addressed the suppressed finding from the 20:50 review in 90f59ab.

Fixed — the occupancy NOTE gated on key presence rather than a rendered value. csv.DictReader creates the key with None for a short row, so a CSV whose header merely declares VGPR_Count suppressed the explanation while every kernel line still printed ?. The reader is left with bare placeholders and no reason for them — which is precisely what the note exists to prevent.

Reproduced with this PR's own test_short_rows_do_not_raise fixture (header declares VGPR_Count, row is short): VGPR ? on the line, no NOTE. The gate now asks the existing field() helper whether anything rendered as a real value, so the condition matches exactly what the reader sees rather than what the dict happens to contain.

Both directions are now tested — the note appears for placeholder-only output, and stays away when ROCm 7.x populates the columns.

Verified in the docker/rocm container (ROCm 7.2, gfx942): validate_rocprof still reports region 'flashinfer_bench_profile' (9 of 20) and PASS, with no spurious NOTE; tests/agent 28 passed.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/agent/test_rocprof.py:128

The new regression-test docstring still references _region_window, but that helper no longer exists (it was replaced by _region_spans). This can confuse future readers/searches and makes the explanation look stale.

Consider rewording to describe “region correlation” or “marker span matching” rather than naming the removed function.

    Captured verbatim from ROCm 7.2 in the docker/rocm container: the roctx message is in the
    "Function" column and there is no "Name" column at all. Reading "Name" made _region_window
    return None on every real run, so the report silently widened to every kernel on the device
    while still advertising the region.

_region_window became _region_spans when membership moved from a single window to a union of spans, but the test docstring still named the removed helper and one test name still said "region_window" — dead symbols that break grep and read as though the code still works that way. The docstring now describes the behaviour ("region correlation match nothing") rather than naming a function, so it survives the next rename. Test renamed to test_region_spans_skip_malformed_marker_rows. No references remain repo-wide.

demandal25 · 2026-07-31T21:01:46Z

Addressed the suppressed finding from the 20:58 review in e29977a.

Fixed — stale _region_window references after the rename. The helper became _region_spans when membership moved from a single window to a union of spans, but the regression-test docstring still named the removed function, and a test name still said region_window. Dead symbols break grep and read as though the code still works that way.

Went slightly further than the suggestion: the docstring now describes the behaviour ("region correlation match nothing") rather than naming any function, so it survives the next rename too — naming an internal helper in prose is what made it go stale in the first place. Test renamed to test_region_spans_skip_malformed_marker_rows. Verified no _region_window references remain repo-wide.

ruff clean, tests/agent/test_rocprof.py 17 passed.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

flashinfer_bench/agents/rocprof.py:120

The return type List[tuple] uses an unparameterized generic (tuple), which defeats type checking (and can be flagged under mypy strict/disallow-any-generics). Use a fully-typed tuple for the span pair.

def _region_spans(out_dir: Path) -> List[tuple]:

_region_spans returned List[tuple] — an unparameterized generic that tells a type checker nothing and trips mypy's disallow-any-generics. It returns (start_ns, end_ns) pairs, so List[Tuple[int, int]] says that. Scoped to the annotation this PR introduced. The remaining List[dict] uses in rocprof._read_csv and schema.get_all_tool_schemas are pre-existing and left alone; note csv.DictReader can yield None values, so annotating those honestly would be Dict[str, Optional[str]] rather than Dict[str, str] — worth doing, but not in this PR.

demandal25 · 2026-07-31T21:23:43Z

Addressed the suppressed finding from the 21:05 review in 81296d0.

Fixed — _region_spans returned List[tuple]. An unparameterized generic tells a type checker nothing and trips mypy's disallow-any-generics. It returns (start_ns, end_ns) pairs, so it is now List[Tuple[int, int]].

Scoped deliberately to the annotation this PR introduced. The remaining List[dict] uses in rocprof._read_csv and schema.get_all_tool_schemas are pre-existing and left alone — worth noting that annotating those honestly would be Dict[str, Optional[str]] rather than Dict[str, str], since csv.DictReader yields None for missing trailing fields (the same behaviour that caused the TypeError fixed earlier in this PR). That is a real improvement, but for its own change.

ruff clean, tests/agent/test_rocprof.py 17 passed.

Copilot