Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cquil11 merged 1 commit into from
Aug 4, 2026
+24 −0
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions benchmarks/benchmark_lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,30 @@
GPU_MONITOR_PID=""
}

# Block until the GPUs have released a prior job's memory before starting a run.
# Polls rocm-smi VRAM% every 10s for up to 15 minutes; succeeds once the busiest
# GPU is at <=10% VRAM, otherwise returns 1 so the caller aborts rather than
# starting a benchmark on GPUs still draining the previous run's memory.
wait_for_amd_gpu_clean() {
local gpu_clean=false vram_max i
for i in $(seq 1 90); do
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}')
if [ "${vram_max:-0}" -le 10 ]; then
echo "GPUs clean (vram%max=$vram_max after $((i * 10))s)"
gpu_clean=true
break

Check warning on line 179 in benchmarks/benchmark_lib.sh

View check run for this annotation

Claude / Claude Code Review

wait_for_amd_gpu_clean logs incorrect elapsed time on success 

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.
Comment on lines +172 to +179

@claude claude Bot Aug 4, 2026

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.

🟡 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.

fi
echo "waiting for prior-job GPU memory reclaim: vram%max=$vram_max"
sleep 10
done
if [ "$gpu_clean" != "true" ]; then
echo "Error: GPUs still draining prior job's memory after 15min" >&2
return 1
fi
}

Check warning on line 189 in benchmarks/benchmark_lib.sh

View check run for this annotation

Claude / Claude Code Review

wait_for_amd_gpu_clean fails open when rocm-smi output cannot be parsed 

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
Comment on lines +166 to +189

@claude claude Bot Aug 4, 2026

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.

🟡 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.

# Return success only while a PID exists and is not a zombie waiting to be
# reaped. `kill -0` alone treats zombies as live processes.
_background_process_is_running() {
Expand Down