[PowerX AMD] preserve telemetry through the measurement end / 保留完整窗口遥测 - #3041
[PowerX AMD] preserve telemetry through the measurement end / 保留完整窗口遥测#3041edwingao28 wants to merge 2 commits into
Conversation
保留 AMD 功耗测量边界及诊断信息。
d6dc50d to
e31d7f6
Compare
|
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 For PR verification, add the 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 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
等待延迟到达的首条 AMD 有效遥测，并对不支持的采样流执行同一停止超时，避免提前结束采样或超时等待。
|
Claude finished @edwingao28's task in 4m 1s —— View job Review of PR #3041
SummaryOne 🟡 WARNING posted inline on No blocking issues found. The FIFO reader/writer ordering, the trap re-entrancy during the coverage wait, the awk column detection (mirrors Not verified: the sandbox denied running |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| GPU_MONITOR_PIPE="${output}.pipe.$$" | ||
| mkfifo "$GPU_MONITOR_PIPE" || return 1 |
There was a problem hiding this comment.
🟡 WARNING: mkfifo ... || return 1 turns a monitor-setup failure into a benchmark abort.
Why it matters: Every AMD recipe that calls start_gpu_monitor runs under set -eo pipefail (e.g. dsv4_fp8_mi300x.sh, dsv4_fp4_mi355x_vllm.sh), and the AgentX path at line 3454 is inside a set -e subshell. Before this PR, starting the monitor could never fail the run. Now mkfifo failing kills the benchmark before the server even launches, in two realistic cases:
- A stale FIFO at the same path. The default output is
gpu_metrics.csvin the reused runner workspace, and the suffix is$$, which is easily reused across container runs. A prior run killed by a job cancel/timeout never reachesstop_gpu_monitor, so the FIFO stays behind andmkfiforeturnsEEXIST. - The result dir living on a filesystem that rejects
mknod(some network shares). The MI355X runner workspaces sit under/it-share/gharunners*.
Also, on this failure path GPU_MONITOR_VENDOR="amd" and GPU_MONITOR_PIPE stay set with no PIDs, so a later stop_gpu_monitor would still run the AMD teardown branch.
Fix: remove any stale FIFO first and degrade to "no monitoring" instead of failing, matching the pre-PR contract. Consider mktemp -u under ${TMPDIR:-/tmp} for the FIFO path if the result dir may be on a network share.
| GPU_MONITOR_PIPE="${output}.pipe.$$" | |
| mkfifo "$GPU_MONITOR_PIPE" || return 1 | |
| GPU_MONITOR_PIPE="${output}.pipe.$$" | |
| rm -f "$GPU_MONITOR_PIPE" | |
| if ! mkfifo "$GPU_MONITOR_PIPE" 2>/dev/null; then | |
| echo "[GPU Monitor] Warning: mkfifo failed at $GPU_MONITOR_PIPE, skipping AMD monitoring" >&2 | |
| GPU_MONITOR_PIPE="" | |
| GPU_MONITOR_VENDOR="" | |
| return 0 | |
| fi |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 942c058. Configure here.
| # Track both processes: killing only awk can leave amd-smi alive until | ||
| # its next write. Keep the FIFO beside this run's raw CSV, never shared. | ||
| GPU_MONITOR_PIPE="${output}.pipe.$$" | ||
| mkfifo "$GPU_MONITOR_PIPE" || return 1 |
There was a problem hiding this comment.
mkfifo failure aborts AMD monitoring
Medium Severity
start_gpu_monitor now creates a named pipe beside the metrics CSV and return 1s if mkfifo fails. AgentX calls it without checking status, so under set -e a FIFO setup failure aborts the whole replay instead of skipping power. That is a new hard failure: the old anonymous pipe could not fail this way, and a missing SMI tool still returns 0.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 942c058. Configure here.
| # power > 0), or nothing when the stream holds no usable epoch-stamped row | ||
| # (e.g. an amd-smi build emitting ISO timestamps). Column detection mirrors | ||
| # _POWER_COL_RE/_POWER_EXCLUDE_RE/_GPU_INDEX_COL_RE in utils/aggregate_power.py. | ||
| # POSIX awk only: the ROCm container images ship mawk/busybox awk. | ||
| _amd_monitor_min_covered_tick() { | ||
| [[ -f "$GPU_METRICS_CSV" ]] || return 0 | ||
| awk -F, ' | ||
| NR == 1 { | ||
| for (i = 1; i <= NF; i++) { | ||
| name = tolower($i) | ||
| gsub(/^ +| +$/, "", name) | ||
| sub(/\r$/, "", name) | ||
| if (!power_col && name ~ /power/ && name !~ /limit|cap|max|min/) | ||
| power_col = i | ||
| if (!gpu_col && name ~ /^(index|gpu|gpu_id|gpu_index|card|device)$/) | ||
| gpu_col = i | ||
| } | ||
| next | ||
| } | ||
| !power_col || !gpu_col { next } | ||
| { | ||
| # amd-smi quotes list-valued cells that embed commas; neutralize | ||
| # them so the power cell keeps its header-relative position. | ||
| line = $0 | ||
| sub(/\r$/, "", line) | ||
| if (line ~ /"/) { | ||
| n = split(line, seg, /"/) | ||
| line = "" | ||
| for (i = 1; i <= n; i++) { | ||
| if (i % 2 == 0) gsub(/,/, ";", seg[i]) | ||
| line = line seg[i] | ||
| } | ||
| } | ||
| count = split(line, cell, /,/) | ||
| if (count < power_col || count < gpu_col) next | ||
| if (cell[1] !~ /^[0-9]+(\.[0-9]+)?$/) next |
There was a problem hiding this comment.
🔴 _amd_monitor_min_covered_tick only treats column 1 as usable when it matches a plain numeric epoch (cell[1] !~ /^[0-9]+(\.[0-9]+)?$/ { next }). single_node.py's _parse_timestamp explicitly documents amd-smi emitting ISO-8601 timestamps 'as observed in practice', which this regex rejects on every row. For such builds the poll never finds a covered tick, so _wait_for_amd_stop_coverage silently blocks the full AMD_MONITOR_STOP_TIMEOUT_S (30s default) on every stop instead of the old ~3-5s fixed sleep, adding ~25s teardown latency per benchmark run. Fix: parse ISO-8601 (and other formats single_node.py tolerates) in the coverage awk too, or detect an all-non-numeric stream quickly and stop waiting instead of running to the full timeout.
Extended reasoning...
amd-smi (a version/config producing ISO timestamps, per single_node.py's own comment 'amd-smi: ISO 8601 ... or epoch seconds') writes rows whose first column is e.g. '2025-01-15T12:34:56.789'. In _amd_monitor_min_covered_tick, cell[1] !~ /^[0-9]+(\.[0-9]+)?$/ is true for every such row, so next skips it; newest[] stays empty and the function prints nothing. _wait_for_amd_stop_coverage's covered is always empty, so the -ge target check never passes; the loop runs sleep 1 until date +%s -ge deadline (30s after entry by default), then logs 'never covered the stop request' and returns. Every AMD stop_gpu_monitor call on such a build now pays ~30s versus the previous unconditional sleep $((interval+2)) (~3-5s), a real per-run latency regression the comment anticipates but does not size.
Verification: nit — acknowledged in diff (note's claims hold). Mechanism is real: benchmark_lib.sh:331 if (cell[1] !~ /^[0-9]+(\.[0-9]+)?$/) next rejects ISO-8601 first columns (2025-01-15T12:34:56.789) on every row, so newest[] stays empty and _amd_monitor_min_covered_tick prints nothing. In _wait_for_amd_stop_coverage, covered is then always empty, the `[[ -n "$covered" && "$covered" -ge…
1 participant
Description
Superseded by #3026: AMD monitor fixes and validation travel together.
Keep AMD telemetry through the measurement end, handle delayed samples, bound cleanup, and retain invalid-boundary diagnostics.
Testing: 202 focused tests passed; delayed-sample regression failed before the fix. Bash/changelog checks passed.
Pending: MI355X hardware validation and the FIFO-startup review fix continue in #3026. No separate sweep is needed for this duplicate.
中文
中文说明
由 #3026 接替： AMD 监控修复与验证统一在该 PR 推进。
保留 AMD 遥测直到测量结束，处理延迟采样，限制清理等待，并保留无效边界行诊断。
测试： 202 项针对性测试通过；延迟采样回归测试在修复前失败。Bash/changelog 检查通过。
待完成： MI355X 硬件验证与 FIFO 启动问题的 review 修复继续在 #3026 完成。本重复范围无需单独运行 sweep。
Related Issue
Scope index and shared policy / 范围索引与共同规则: #3030.
Type of Change
Checklist
perf-changelog.yamland have not edited historical entriesOWNER/MEMBER/COLLABORATOR) has commented/reuse-sweep-runon this PR. Do this only once there is a final full sweep that is all green with evals passing, since after this comment the sweep label will no longer automatically kick off new sweeps. Remove and re-add the label to force one.Note
Medium Risk
Changes benchmark teardown timing and power validity rules for AMD AgentX runs; mis-tuned timeouts could delay stops or mark windows invalid, but cancellation remains bounded and failures stay explicit in audits.
Overview
Improves AMD GPU power telemetry so benchmark windows can be bracketed reliably and teardown noise does not corrupt validity.
Monitor lifecycle (
benchmark_lib.sh): AMDamd-smiwatch output now flows through a per-run FIFO with separate PIDs for the producer andawkconsumer, so killing the parser does not leaveamd-smirunning or drop buffered rows. On stop, the fixed tail sleep is replaced by pollinggpu_metrics.csvuntil every GPU has a usable tick (numeric epoch, power > 0) at or after the next whole second, bounded byAMD_MONITOR_STOP_TIMEOUT_S(default 30s). AgentX signal/EXIT cleanup sets that timeout to 0 so cancellation stays fast while normal completion still waits for coverage.Aggregation (
infx/results/power/single_node.py): Rows in the ingest band but outside the formal window with missing, non-finite, or ≤0 W power are skipped for bracketing and integration and counted in auditboundary_degenerate_rows(e.g. N/A or 0 W teardown tails no longer fake end bracketing). In-window invalid samples are unchanged.Docs,
perf-changelog.yaml, and shell/Python tests cover the new stop behavior, abort path, and degenerate-boundary semantics.Reviewed by Cursor Bugbot for commit 942c058. Bugbot is set up for automated code reviews on this repo. Configure here.