[AMD][Power] fix: wait for AMD telemetry to cover the benchmark window end before stopping the monitor / 修复 AMD 遥测在基准窗口结束前被截断导致功耗校验失败的问题 - #2767
Conversation
…e killing the monitor / 修复：AMD 遥测覆盖停止请求后再终止功耗监控进程 The AMD stop path used an open-loop fixed sleep (interval+2) before killing the awk pipeline consumer, so rows still in the OS pipe or not yet emitted by a slow amd-smi iteration were lost and the last on-file tick could trail the aiperf window end by several seconds (run 32433563482 conc1: last tick 1787277605 vs end ...609.157), failing power validation with benchmark_window_not_bracketed. Replace the fixed tail with a bounded poll of the output CSV: wait until every observed GPU has a usable tick (numeric epoch timestamp, power > 0) stamped at the first whole second past stop entry — amd-smi stamps integer seconds, and the window end never exceeds the stop-entry wall clock, so that tick strictly covers any fractional window end. Bounded by AMD_MONITOR_STOP_TIMEOUT_S (default 30 s, 0 skips); non-epoch timestamps keep the legacy fixed tail; on timeout or early monitor death it warns and proceeds so aggregation attributes the failure (fail-safe, never fail-silent). AgentX INT/TERM/EXIT traps stop in abort mode (timeout 0) to keep signal teardown fast; the explicit post-replay stop performs the full wait. The NVIDIA branch is unchanged. AMD 停止路径原先在杀掉 awk 管道消费者前只做固定时长的 sleep，导致管道缓冲中的 采样行丢失、文件中最后一个时间戳可能落后于基准窗口结束数秒，功耗校验因 benchmark_window_not_bracketed 失败。本补丁改为有界轮询输出 CSV：等到每个 GPU 都有一条时间戳到达停止时刻下一整秒的可用采样（数值时间戳、功率 > 0）再终止监控， 由 AMD_MONITOR_STOP_TIMEOUT_S 限定（默认 30 秒，0 表示跳过）；非 epoch 时间戳 回退到旧的固定等待；超时或监控提前退出时告警并继续，由聚合端归因。AgentX 的 INT/TERM/EXIT trap 以 abort 模式停止（跳过等待），正常收尾仍执行完整覆盖等待。 NVIDIA 分支不变。
…oisoning with them / 修复：统计窗口外的异常功耗行，不再用其伪造窗口覆盖或污染校验 SMI teardown rows can carry N/A or 0 W power cells. Inside the +/-3 s ingest band but outside the formal window, an N/A row previously flipped validity via invalid_power_sample, and a 0 W row could silently satisfy end bracketing while corrupting the boundary interpolation. integrate_power now skips such rows (power missing, non-finite, or <= 0 outside [start, end]) and counts them per GPU in a new additive sidecar field boundary_degenerate_rows. In-window semantics are unchanged: in-window N/A still yields invalid_power_sample and in-window 0 W still integrates. No new reason codes; sidecar schema_version stays 1; aggregate rows are untouched. SMI 收尾阶段可能输出功率为 N/A 或 0 W 的行：在 ±3 秒摄取带内但位于正式窗口外时， N/A 行会误置 invalid_power_sample，0 W 行则可能伪造窗口末端覆盖并污染边界插值。 integrate_power 现在跳过此类行（窗口外且功率缺失、非有限或 <= 0），并按 GPU 计入 新增的 sidecar 字段 boundary_degenerate_rows。窗口内语义不变：窗口内 N/A 仍记 invalid_power_sample，窗口内 0 W 仍参与积分。不新增 reason 代码，sidecar schema_version 保持 1，聚合结果行结构不变。 Includes a regression fixture for run 32433563482 conc1 (MI355X integer-second ticks ending 4 s before the fractional aiperf window end) asserting the producer failure stays attributed as benchmark_window_not_bracketed.
…epochs / 加固 AMD 停止覆盖轮询：容错非法超时值并归一化毫秒时间戳 - Sanitize AMD_MONITOR_STOP_TIMEOUT_S: a non-integer value (e.g. "30s") previously aborted stop_gpu_monitor via a bash arithmetic error under set -e, leaking the monitor process and skipping tail repair and the energy sidecar; it now warns and falls back to the default 30. - Normalize millisecond epoch timestamps (>1e12) in the awk coverage helper, mirroring _parse_timestamp in utils/aggregate_power.py, so a ms-stamping amd-smi build cannot trivially satisfy the stop target. - Shell-contract tests pin both behaviors.
|
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 关于重新运行失败任务的文档 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked how _wait_for_amd_stop_coverage handles an empty result from _amd_monitor_min_covered_tick on the first poll (benchmarks/benchmark_lib.sh:313) — it falls back to the same fixed interval+2 sleep that existed before this PR, so that path is not a regression even though it forgoes retrying before treating the stream as unusable.
Extended reasoning...
Verified by reading _wait_for_amd_stop_coverage: when _amd_monitor_min_covered_tick returns empty on any poll iteration, the function immediately runs sleep $(( ${GPU_MONITOR_INTERVAL:-1} + 2 )) and returns — identical to the pre-PR unconditional sleep. Since this is byte-for-byte the previous behavior (not a new code path with new risk), it doesn't warrant a separate inline finding, but it is worth noting as something checked and ruled out given the two confirmed findings already surfaced in the same area of the diff.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
utils/aggregate_power.py— When every row in the CSV ends up in raw_samples empty (all in-band rows are boundary-degenerate outside the window, e.g. amd-smi emitting N/A/0W before it warms up and no valid sample ever falls inside the window), integrate_power hits theif not raw_samples:branch and returns_empty_integration(...), which does not accept/forward theboundary_degeneratedict collected in the row loop. The newboundary_degenerate_rowssidecar field is silently reset to{}even though degenerate rows were actually observed and counted.Extended reasoning...
A stream where amd-smi only ever emits N/A/0W rows within the ingest band around a short benchmark window (e.g. monitor cold-start racing a very short window, or full-window telemetry outage) triggers
no_usable_power_samples; before returning,boundary_degeneratealready has non-zero per-GPU counts, but_empty_integrationbuildsPowerIntegrationwithout passing them, so the validation sidecar reportsboundary_degenerate_rows: {}instead of the true counts, hiding exactly the diagnostic signal (many degenerate teardown/startup rows, zero usable samples) this PR added the field to surface for run investigation. Fix: threadboundary_degeneratethrough to_empty_integration(or move theif not raw_samplescheck to build a PowerIntegration that carries it) at this call site.Verification: nit. Factually correct diagnostic-loss defect. In integrate_power (utils/aggregate_power.py), boundary_degenerate is populated in the new row-loop branch (lines 417-426:
boundary_degenerate[key] = boundary_degenerate.get(key,0)+1; continue). If every in-band row is degenerate-outside-window (all skipped) and no valid in-window sample lands, raw_samples stays empty and the early return at lines…
| agentx_monitor_stopped=0 | ||
| # This function runs in a subshell, so these handlers cannot replace | ||
| # launcher-owned traps. The stopped flag keeps explicit and signal/EXIT | ||
| # cleanup idempotent. | ||
| trap '_stop_agentx_power_monitor' EXIT | ||
| trap '_stop_agentx_power_monitor; exit 130' INT | ||
| trap '_stop_agentx_power_monitor; exit 143' TERM | ||
| # cleanup idempotent. Abort mode only ever fires when the explicit | ||
| # post-replay stop did not run (abnormal exit), where the coverage | ||
| # wait would only slow teardown down. | ||
| trap '_stop_agentx_power_monitor abort' EXIT | ||
| trap '_stop_agentx_power_monitor abort; exit 130' INT | ||
| trap '_stop_agentx_power_monitor abort; exit 143' TERM | ||
| fi | ||
|
|
||
| echo "$REPLAY_CMD" > "$result_dir/benchmark_command.txt" |
There was a problem hiding this comment.
🟣 The explicit post-replay stop (line 2385, non-abort mode) sets agentx_monitor_stopped=1 before calling stop_gpu_monitor, then runs the new up-to-30s _wait_for_amd_stop_coverage poll. If INT/TERM arrives during that poll, the EXIT/INT/TERM trap fires '_stop_agentx_power_monitor abort; exit 1xx', but the idempotency guard (agentx_monitor_stopped already '1') makes that call a no-op, so the trap immediately exits the subshell without ever reaching stop_gpu_monitor's kill "$GPU_MONITOR_PID" line. This pre-existing race (previously bounded by only a ~3s fixed sleep) is now exercisable for up to AMD_MONITOR_STOP_TIMEOUT_S seconds (default 30s) because of this diff, making the leak far more likely to actually trigger.
Extended reasoning...
A scheduler/user cancels the benchmark job (SIGTERM/SIGINT) in the tens-of-seconds window after the replay command finishes but before the AMD coverage wait completes. The trap's abort call is swallowed by the idempotency flag, exit happens immediately, and the amd-smi/awk GPU_MONITOR_PID process is orphaned instead of killed, continuing to append to GPU_METRICS_CSV indefinitely and potentially corrupting telemetry for any later measurement window/point that reuses the same CSV path. A correct fix must let a signal received during the explicit (non-abort) wait itself short-circuit stop_gpu_monitor's poll and still guarantee the kill, e.g. by tracking 'wait in progress' state independent of agentx_monitor_stopped, or by killing GPU_MONITOR_PID unconditionally in the trap regardless of the idempotency flag.
Verification: Severity: pre-existing (nit at most). The mechanism is real and reachable. In run_agentic_replay_and_write_outputs, line 2384-2385 calls _stop_agentx_power_monitor (non-abort). The function (diff) sets agentx_monitor_stopped=1 BEFORE running stop_gpu_monitor, whose AMD branch now runs _wait_for_amd_stop_coverage — a poll bounded by AMD_MONITOR_STOP_TIMEOUT_S (default 30s) with `sleep…
![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 7277aec. Configure here.
| # A cancelled run's power validity is moot; skip the AMD | ||
| # coverage wait so signal teardown stays fast. | ||
| AMD_MONITOR_STOP_TIMEOUT_S=0 | ||
| fi |
There was a problem hiding this comment.
Signal during coverage wait leaks monitor
Medium Severity
_stop_agentx_power_monitor sets agentx_monitor_stopped before stop_gpu_monitor returns, while INT/TERM traps stay installed through the new coverage wait. A signal during that wait runs abort-mode cleanup, which is a no-op because the flag is already set, so the amd-smi/awk pipeline is never killed.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 7277aec. Configure here.
1 participant
Fixes the
benchmark_window_not_bracketedpower-validation failures on AMD single-node AgentX runs (e.g. run 32433563482,kimik3-fp4-mi355x-atom-agentic-mtp, 3 of 4 points failed).Problem
On run 32433563482 conc1 the amd-smi stream's last CSV tick is
1787277605(integer epoch second) while the aiperf window end is1787277609.157— telemetry stops ~4 s before the formal window end. Three interacting mechanisms, none bounded by the old fixedsleep interval+2before killing the pipeline:GPU_MONITOR_PIDis theawkconsumer, notamd-smi; rows sitting in the OS pipe (awk's per-rowfflush()can block on NFS-backed$result_dir) are discarded at kill.amd-smi metric -p -c -t -uiteration over 8 GPUs takes non-trivial time, so the newest emitted tick can trail wall clock by seconds.invalid_power_sample, and a 0 W row past the window end could silently fake end bracketing while corrupting the boundary interpolation.Solution
benchmarks/benchmark_lib.sh— deterministic poll-until-covered stop.stop_gpu_monitor's AMD branch now polls$GPU_METRICS_CSVuntil every observed GPU has a usable tick (numeric epoch timestamp, numeric power > 0) stamped ≥ floor(stop-entry epoch)+1, then kills. The window end is always ≤ the stop-entry wall clock and amd-smi stamps integer seconds, so that tick strictly covers any fractional window end — no aiperf knowledge needed in the shell. Details:AMD_MONITOR_STOP_TIMEOUT_S(default 30 s, env-overridable,0skips the wait; a non-integer value warns and falls back to 30 instead of aborting the stop). On timeout or early monitor death: warn and proceed — aggregation then flags the point exactly as today (fail-safe, never fail-silent).interval+2fixed tail once; millisecond epochs (>1e12) are normalized to seconds, mirroring_parse_timestampinutils/aggregate_power.py.INT/TERM/EXITtraps stop in abort mode (AMD_MONITOR_STOP_TIMEOUT_S=0): a cancelled run's power validity is moot and teardown stays fast. The explicit post-replay stop performs the full wait. Idempotency viaagentx_monitor_stoppedpreserved.stop_gpu_monitor(allbenchmarks/single_node/fixed_seq_len/*recipes), not just AgentX.utils/aggregate_power.py— explicit boundary-degenerate handling.integrate_powernow skips-and-counts rows inside the ±max_sample_gap_singest band but outside the formal window whose power is unparseable, non-finite, or ≤ 0, into a new per-GPU sidecar fieldboundary_degenerate_rows("unknown"buckets rows without a GPU identity). They are never used for bracketing and never poison validity. In-window semantics are byte-identical: in-window N/A still ⇒invalid_power_sample; in-window 0 W still integrates.Contract compatibility
Relevant to the queued row-level power provenance change (
feat/power-row-provenancein InferenceX) and to InferenceX-app PR #939:benchmark_window_not_bracketedkeeps meaning "a real usable sample does not bracket the boundary".power_validstill numeric; nopower_invalid_reasonson aggregates).schema_versionstays 1;boundary_degenerate_rowsis additive and readers tolerate its absence.Validity flips on re-aggregated legacy artifacts (intentional)
Only when explicitly re-running aggregation on old raw artifacts (e.g. the failed-ingest recovery path):
benchmark_window_not_bracketed(they were silently corrupt before).Already-ingested aggregate rows are untouched.
Cost
Normal case ~1–2 s at stop (first whole second after stop entry). Pathological hang: up to 30 s (was 3 s), bounded, logged, env-tunable.
Tests
bash -n benchmarks/benchmark_lib.shclean.utils/test_aggregate_power.py+utils/agentic/aggregation/test_power_lifecycle.py: 74 passed (57 + 17)....609.157497, real power values, quoted list cells, CR endings, blank inter-tick lines, plus synthetic N/A / 0 W teardown shapes) assertingbenchmark_window_not_bracketedattribution.stop_gpu_monitoragainst a scripted producer (coverage wait for all GPUs; degenerate rows never satisfy coverage → timeout warning; per-GPU min semantics; ISO-timestamp legacy fallback; non-integer timeout survival; millisecond-epoch normalization), plus the signal test now pins abort mode (AMD_MONITOR_STOP_TIMEOUT_S=0at trap-driven stop) and the new trap strings.utils/test_process_result.py: the AMD stop test now pins the new contract (covered tick ⇒ no legacy sleep, energy snapshot still written); truncated-row repair test runs with the wait skipped.test_aggregate_power*,test_process_result,utils/agentic/aggregation/): 224 passed.utils/suite: 804 passed; the only 2 failures (utils/evals/test_run_eval_dispatch.py) reproduce identically on cleanorigin/mainin this local environment (macOS temp-path issue), unrelated to power.Review notes
AMD_MONITOR_STOP_TIMEOUT_S(e.g."30s") aborted the wholestop_gpu_monitorcall via a bash arithmetic error underset -e— leaking the monitor process and skipping tail repair and the energy sidecar. Fixed: the timeout is sanitized once before any arithmetic (warn + fall back to 30); the timeout warning now always prints a real number. Pinned bytest_amd_stop_survives_non_integer_timeout._parse_timestamp. Pinned bytest_amd_stop_normalizes_millisecond_epoch_timestamps.Evidence note
The retrieved run-32433563482 conc1 artifact shows the trailing rows all carry valid socket_power (254–264 W) with N/A activity/voltage cells — the observed failure is purely mechanisms (1)+(2) (the stream stops 4 s early). The degenerate-row handling hardens against mechanism (3) using the synthetic N/A / 0 W teardown shapes exercised in the tests.
Hardware smoke (post-merge hardware validation; not a merge gate for this PR, but the acceptance gate for the fix)
Re-sweep
kimik3-fp4-mi355x-atom-agentic-mtp(configs/amd-master.yaml:656) from this PR branch on the MI355X ATOM cluster. For each point (conc 1, 4, 8, 10) in the raw-results artifact:agg_*.jsonhaspower_valid == 1and finiteavg_power_w/total_gpu_energy_j;results/**/power_validation.jsonhasreasons == [], and for every GPU the newest usablegpu_metrics.csvtick ≥benchmark_window.end_time_unix;boundary_degenerate_rows(if non-empty) shows teardown rows were counted, not integrated;accumulator_check.within_tolerance == truewhere the sidecar snapshots exist;never covered the stop requestwarning, and per-point wall time increased by ≲ a few seconds versus the failing run.If any point still fails with
benchmark_window_not_bracketed, attach its CSV tail +power_validation.jsonto this PR before iterating (per-GPUsampling_gap_exceededwould instead indicate the separate sampling-cadence issue tracked as follow-up work to this PR).Note
Medium Risk
Changes AMD benchmark stop timing and power-validation semantics (including intentional validity flips when re-aggregating legacy artifacts); bounded fail-safe behavior but affects published power metrics on AMD paths.
Overview
Fixes AMD AgentX and single-node runs failing
benchmark_window_not_bracketedwhen amd-smi integer-second ticks stop before the fractional benchmark window end.stop_gpu_monitor(AMD) replaces the fixedinterval+2sleep with a bounded poll ofgpu_metrics.csvuntil every GPU has a usable sample (numeric epoch, power > 0) at or after the next whole second past stop entry, configurable viaAMD_MONITOR_STOP_TIMEOUT_S(default 30s;0skips). ISO timestamps keep the legacy tail; ms epochs are normalized likeaggregate_power. AgentX INT/TERM/EXIT traps use abort mode (AMD_MONITOR_STOP_TIMEOUT_S=0) so cancelled runs tear down quickly; the normal post-replay stop keeps the full wait.integrate_powernow skips—and counts in the validation sidecar asboundary_degenerate_rows—ingest-band rows outside the formal window with missing, non-finite, or ≤0 W power, so teardown N/A/0 W rows no longer poison validity or fake end bracketing (in-window behavior unchanged).Tests cover the real shell stop path, signal abort mode, timeout sanitization, and aggregation regressions including the MI355X conc1 shape.
Reviewed by Cursor Bugbot for commit 0c4f204. Bugbot is set up for automated code reviews on this repo. Configure here.