[AMD][AgentX] benchmark_lib: add wait_for_amd_gpu_clean GPU-drain gate

[seungrokj]

Summary

• Add wait_for_amd_gpu_clean to benchmarks/benchmark_lib.sh: a pre-run gate that polls rocm-smi --showmemuse VRAM% every 10s and blocks until the busiest GPU is at ≤10% VRAM, up to a 15-minute timeout.
• Returns non-zero on timeout (library-friendly), so a caller running set -euo pipefail aborts rather than starting a benchmark on GPUs still draining a prior job's memory.

Test plan

• bash -n benchmarks/benchmark_lib.sh passes
• A recipe sourcing benchmark_lib.sh can call wait_for_amd_gpu_clean before launching its server

🤖 Generated with Claude Code

[github-actions]

Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs

---

感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have。

如需进行 PR 验证，请为此 PR 添加 full-sweep-fail-fast 标签（强烈推荐）— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled。

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

[claude]

Approving — this is a small, isolated addition (one new function, no existing callers wired up yet) with only minor nits found.

What was reviewed: the polling/timeout loop logic, the rocm-smi parsing pipeline, and the success/failure return-code contract. The candidate concern about a missing per-call timeout was checked and ruled out — the outer for i in $(seq 1 90) loop with 10s sleeps already bounds total runtime to ~15 minutes.

Extended reasoning...

Overview

The PR adds a single new function, wait_for_amd_gpu_clean, to benchmarks/benchmark_lib.sh. It polls rocm-smi --showmemuse every 10s (up to 90 iterations / 15 minutes) and returns once the busiest GPU's VRAM% is at or below 10%, or returns 1 on timeout. It is a pure addition — no existing function or call site was modified, and grepping the repo confirms nothing yet calls this new function, so it cannot regress any currently-running recipe.

Security risks

None. This is a local shell utility that shells out to rocm-smi and parses its own stdout with grep/awk; there is no untrusted input, no network call, and no privilege escalation involved.

Level of scrutiny

Low-to-moderate is appropriate here. It's an isolated, additive utility function in a benchmarking shell library, not a change to a production-critical or security-sensitive path, and it currently has zero callers in the codebase, so its blast radius today is zero. The two issues found (cosmetic off-by-one in the success log's elapsed-time calculation, and a fail-open edge case if rocm-smi output can't be parsed) are both nits: the first is purely a misleading log string, and the second only matters once a real caller starts depending on this gate for correctness — which hasn't happened in this PR.

Other factors

A candidate concern about the loop having no per-call timeout was raised and ruled out on inspection — the fixed 90-iteration loop with 10s sleeps already caps worst-case runtime at 15 minutes, matching the function's documented contract. The PR's own test plan is limited to bash -n syntax checking and manual integration by a future recipe, which is proportionate given no caller exists yet.

[claude]

🟡 The success log in wait_for_amd_gpu_clean() uses $((i * 10)) for elapsed seconds, but the VRAM check runs before the sleep, so it's overstated by one 10s poll interval (e.g. printing "after 10s" when GPUs were already clean at 0s). Purely cosmetic — use $(((i - 1) * 10)) instead.

Extended reasoning...

The bug: In wait_for_amd_gpu_clean() (benchmarks/benchmark_lib.sh:167-190), the loop body checks the VRAM usage first, then only sleeps 10s at the bottom if the check fails. The success-path log line, however, computes elapsed time as $((i * 10)), which implicitly assumes a sleep has already happened for every completed iteration including the current one.

Code path:

for i in $(seq 1 90); do
    vram_max=$(rocm-smi --showmemuse ...)
    if [ "${vram_max:-0}" -le 10 ]; then
        echo "GPUs clean (vram%max=$vram_max after $((i * 10))s)"   # <-- overstated
        gpu_clean=true
        break
    fi
    echo "waiting for prior-job GPU memory reclaim: vram%max=$vram_max"
    sleep 10
done

By the time iteration i runs its VRAM check, exactly i - 1 sleeps have elapsed (zero sleeps before the first check, one sleep before the second check, and so on). So the true wait time at success is (i - 1) * 10 seconds, not i * 10.

Concrete proof:

• i=1, GPUs already clean: 0 sleeps have occurred → true elapsed = 0s. Logged value: $((1 * 10)) = 10s. Off by 10s.
• i=2, GPUs became clean after one 10s sleep: true elapsed = 10s. Logged value: $((2 * 10)) = 20s. Off by 10s.
• In general, every success message overstates the actual wait by exactly one poll interval (10s).

Why nothing catches this: The function has no test coverage (it's new in this PR), and the value is only used in an informational echo — it isn't captured, compared, or asserted on anywhere, so nothing would fail even though the message is wrong.

Impact: This is purely a misleading log/telemetry value. The gate's actual behavior — waiting for VRAM to drop and returning 0 on success / 1 on timeout — is unaffected; only the human-readable timestamp in the success message is wrong. Anyone reading benchmark logs to gauge how long GPU-drain waits actually took would see numbers consistently 10s too high, which is a minor but avoidable inaccuracy in operational logs used for tuning timeouts.

Fix: Change the success log to echo \"GPUs clean (vram%max=$vram_max after $(((i - 1) * 10))s)\" so it reflects the number of completed sleeps rather than the loop counter.

[claude]

🟡 wait_for_amd_gpu_clean (benchmarks/benchmark_lib.sh:166-189) treats "no VRAM data" the same as "0% VRAM": if rocm-smi is missing, errors out, or its output format doesn't match the fixed regex, grep returns zero matches and the awk pipeline still prints m+0 = 0, so the gate reports "GPUs clean" and returns success on the very first 10s poll. This silently defeats the drain-gate's purpose instead of blocking or erroring as intended. Consider treating empty/unparsed rocm-smi output as not-clean (or a hard error) rather than 0%.

Extended reasoning...

wait_for_amd_gpu_clean computes the busiest GPU's VRAM% with:

vram_max=$(rocm-smi --showmemuse 2>/dev/null \
    | grep -oE "GPU Memory Allocated \(VRAM%\): [0-9]+" \
    | awk '{if ($NF > m) m = $NF} END {print m+0}')

The awk END block runs unconditionally, even when the main pattern block never fired because grep produced zero matching lines. When there's no input, m is never assigned, and print m+0 evaluates the unset variable as 0, printing 0. I verified this directly: echo "" | awk '{if ($NF > m) m = $NF} END {print m+0}' prints 0.

This means the "no data" case and the "genuinely 0% VRAM, GPU is clean" case are indistinguishable to the caller. Three realistic conditions collapse grep's output to zero matches:

1. rocm-smi isn't installed or isn't on PATH (stderr is suppressed by 2>/dev/null, so this fails silently).
2. rocm-smi runs but exits non-zero for some other reason (driver issue, permissions, etc.) — again silenced by 2>/dev/null.
3. rocm-smi's output format doesn't match the hardcoded regex GPU Memory Allocated \(VRAM%\): [0-9]+ — a real risk given that ROCm/rocm-smi output format and wording has changed across versions and can vary with locale.

In any of these cases, vram_max becomes 0, the guard [ "${vram_max:-0}" -le 10 ] is true, and the function immediately echoes "GPUs clean (vram%max=0 after 10s)" and returns success on the very first poll — after only 10 seconds, not the intended 15-minute wait. This is the exact inverse of the function's own documented contract (per its comment: "returns 1 so the caller aborts rather than starting a benchmark on GPUs still draining the previous run's memory"). A caller running under set -euo pipefail would proceed straight into launching a benchmark on GPUs that are still draining a prior job's VRAM, producing contaminated/noisy benchmark results without any visible error — the failure mode is silent, which makes it harder to catch than an outright crash.

Step-by-step proof:

1. Assume rocm-smi is not installed on a given runner (or its CLI output format has drifted from the hardcoded string).
2. rocm-smi --showmemuse 2>/dev/null produces no stdout (command-not-found error is redirected to /dev/null) or exits with unmatched-format text.
3. grep -oE "GPU Memory Allocated \(VRAM%\): [0-9]+" receives that (possibly empty) input and matches zero lines.
4. awk's main block ({if ($NF > m) m = $NF}) never executes since there are no input lines, so m remains unset.
5. awk's END { print m+0 } still runs (END blocks always run) and prints m+0, i.e. 0+0 = 0.
6. vram_max=0, so [ "${vram_max:-0}" -le 10 ] is true.
7. The function echoes "GPUs clean (vram%max=0 after 10s)", sets gpu_clean=true, and returns 0 immediately — instead of the intended 15-minute polling/blocking behavior.

Why existing code doesn't prevent it: there's no check on rocm-smi's exit status, no check that grep actually matched anything (e.g. via grep -c or PIPESTATUS), and no distinction in the awk logic between "ran but found 0%" and "found nothing to measure."

Suggested fix: distinguish "no data" from "actually 0%", e.g. check rocm-smi's exit code and/or that grep matched at least one line before treating vram_max as valid, and treat the "no data" case as not-clean (continue polling) or as a hard error rather than silently declaring success.

Severity: this is being filed as a nit rather than blocking. The happy path (a correctly provisioned AMD host with a rocm-smi version that matches the hardcoded regex) works exactly as intended, and this new function currently has no caller wired into any recipe in this PR — it's being added as a utility for future adoption per the test plan. The fail-open behavior is a real robustness gap worth hardening before recipes start relying on it for correctness, but it doesn't break anything that already works today.