[PowerX] isolate AMD monitor windows and worker lifecycle / 隔离 AMD 监控窗口与工作节点生命周期 - #3055
[PowerX] isolate AMD monitor windows and worker lifecycle / 隔离 AMD 监控窗口与工作节点生命周期#3055edwingao28 wants to merge 36 commits into
Conversation
保留完整请求结果状态与诊断批次，拆分 Slurm 生命周期和原生功耗采集。
拆分原生多节点功耗采集契约、UTC 上下文和生命周期屏障，不启用新路径。
|
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 关于重新运行失败任务的文档 |
8b42c5a to
35c09fa
Compare
保留每个测量窗口的 SMI 上下文工件，覆盖原 #3027 的共享上传规则。
隔离 AMD 监控窗口与工作节点功耗生命周期，保留可选采集故障证据。
由实际重放进程在设置信号处理后发布就绪标记，消除 exec 前 SIGINT 竞态。
35c09fa to
07d1cf1
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
独立生成 prefill 与 decode 角色指标，并在 AMD 采集器被回收前完成取消状态记录。
接入原生角色指标与取消状态契约修复，保留 AMD 完整监控窗口行为。 # Conflicts: # benchmarks/benchmark_lib.sh
在现有 CPU 工作流中执行原生采集契约回归，并跟踪对应文件变更。
接入原生采集器与 AMD 工作节点生命周期的 CPU 回归及路径过滤。
将已验证的信号就绪屏障移至共享原生采集前置，避免重放 exec 前的竞态。
可选功耗初始化失败时保留服务启动及退出状态，仅在 REQUIRE_POWER 开启时阻止服务。
复用原生采集前置中的信号就绪测试修复。
保留主分支已发布的 Kimi-K3 B200 改动，将当前契约声明追加至 changelog 末尾。
保留原生采集测试并纳入主分支 Kimi-K3 测试，复用已验证的业务改动。
保留 AMD 与原生回归覆盖，并纳入主分支 Kimi-K3 测试。
![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 checked whether start_gpu_monitor's new REQUIRE_POWER=1 fail-fast on mkfifo failure (benchmarks/benchmark_lib.sh:214) is actually enforced across call sites — only native_power_collect.sh invokes it with || exit 1; every other benchmark script (dozens under benchmarks/single_node/...) calls it bare, so the fail-fast is effectively opt-in rather than a universal guard. That looks like intentional scoping (only the dedicated native-power collector needs to hard-fail) rather than a bug.
Extended reasoning...
Verified via grep that start_gpu_monitor is called from ~140 single-node benchmark scripts as well as benchmark_lib.sh:3464 and native_power_collect.sh:63; only the latter chains || exit 1, confirming the REQUIRE_POWER=1 fail-fast path added in this PR only has teeth for the new native-power collector script, not for the many pre-existing best-effort monitor invocations. This is consistent with the design intent (native power collection is the only path where a missing FIFO should abort) rather than a regression, so it does not warrant an additional inline finding beyond the two already reported.
Findings marked 🟡 are optional suggestions and need no follow-up push.
|
|
||
| BENCHMARK_STEP_RC=$? | ||
| # Each host copies its own node-local, root-created artifacts as the runner user. | ||
| # No raw telemetry is written as root into the shared checkout. |
There was a problem hiding this comment.
🔴 stage_native_power || BENCHMARK_STEP_RC=1 unconditionally overwrites a successful serving exit code with 1 if the host-side native_power copy fails, with no REQUIRE_POWER/power_required check (unlike bench.sh's gated escalation), so an optional telemetry copy glitch (permission/race/NFS) can flip a passing benchmark run to a reported failure. Fix: only let stage_native_power affect BENCHMARK_STEP_RC when REQUIRE_POWER is set, or log-and-continue otherwise.
Extended reasoning...
job.slurm line 813: BENCHMARK_STEP_RC=$? captures the real serving exit code from the big srun block; line 815 then does stage_native_power || BENCHMARK_STEP_RC=1, discarding a successful 0 if the srun cp step returns non-zero for any node. stage_native_power copies root-created files that native_power_collect.sh chowns to POWERX_HOST_UID/GID only on graceful completion; if the collector is killed early (e.g. via power.sh's EXIT trap racing container teardown) the chown never runs and the subsequent unprivileged cp -r fails, turning a successful serving run into a job-level failure purely due to optional telemetry staging.
Verification: normal. job.slurm line 813 captures the real serving exit code (BENCHMARK_STEP_RC=$?), then line 816 does stage_native_power || BENCHMARK_STEP_RC=1 with no REQUIRE_POWER/power-optional gate, and line 829 exits with that code. This flips a successful serving run (RC=0) to a reported failure (1) whenever the host-side native_power copy fails on any node. Reachability confirmed: - The outer…
| _wait_for_amd_stop_coverage() { | ||
| local target deadline covered timeout_s | ||
| # A non-integer timeout (e.g. "30s") would abort the whole stop_gpu_monitor | ||
| # call under `set -e` at the arithmetic below, leaking the monitor process | ||
| # and skipping tail repair + the energy sidecar; fall back to the default. | ||
| timeout_s="${AMD_MONITOR_STOP_TIMEOUT_S:-30}" | ||
| if [[ ! "$timeout_s" =~ ^-?[0-9]+$ ]]; then | ||
| echo "[GPU Monitor] Warning: ignoring non-integer AMD_MONITOR_STOP_TIMEOUT_S='$timeout_s', using 30" >&2 | ||
| timeout_s=30 | ||
| fi | ||
| if [[ "$timeout_s" -le 0 ]]; then | ||
| return 0 | ||
| fi | ||
| target=$(( $(date +%s) + 1 )) | ||
| deadline=$(( target + timeout_s )) | ||
| while :; do | ||
| covered=$(_amd_monitor_min_covered_tick) | ||
| # The first usable row may arrive after stop begins. Keep the same | ||
| # deadline for empty or unsupported streams instead of stopping early. | ||
| if [[ -n "$covered" && "$covered" -ge "$target" ]]; then | ||
| return 0 | ||
| fi | ||
| if ! _background_process_is_running "$GPU_MONITOR_PID"; then | ||
| echo "[GPU Monitor] Warning: AMD monitor exited before covering the stop request (covered=$covered target=$target)" >&2 | ||
| return 0 | ||
| fi | ||
| if [[ "$(date +%s)" -ge "$deadline" ]]; then | ||
| echo "[GPU Monitor] Warning: AMD telemetry never covered the stop request within ${timeout_s}s (covered=$covered target=$target)" >&2 | ||
| return 0 |
There was a problem hiding this comment.
🟡 (optional) For AMD amd-smi streams that never produce a usable numeric epoch-second sample (e.g. an amd-smi build stamping ISO timestamps, or unexpected CSV column names), _wait_for_amd_stop_coverage does not fall back to a quick wait as docs/results-and-ingestion.md claims ("Streams without usable epoch timestamps use the legacy fixed tail wait"); it instead polls once per second until the full AMD_MONITOR_STOP_TIMEOUT_S deadline (default 30s) before giving up, versus the old fixed ~(interval+2)s (~3s) sleep. This adds ~30s of stall to every stop_gpu_monitor call in that environment, a real wall-clock regression vs base. Fix: detect the 'stream never produced any usable tick' case early and bail with a short wait instead of consuming the full timeout, matching the documented fallback.
Extended reasoning...
In _wait_for_amd_stop_coverage (benchmarks/benchmark_lib.sh:369-398), the while loop calls _amd_monitor_min_covered_tick each second; when the CSV never contains a row matching the numeric-epoch/positive-power/gpu-column pattern (e.g. amd-smi emits ISO8601 timestamps, or a different column name than the regex list), covered is always empty, so the -n "$covered" branch never fires. The monitor process is still alive (GPU_MONITOR_PID running), so the early-exit-on-death branch also never fires. The loop therefore runs until date +%s -ge deadline (target+timeout_s, i.e. ~31s after entry with the default 30s timeout), only then warning and returning 0. The old code (deleted by this diff) just did sleep $((interval+2)), typically ~3s. docs/results-and-ingestion.md explicitly documents a 'legacy fixed tail wait' fallback for this exact case, but no such code path exists post-diff — every stop_gpu_monitor call in an unsupported-timestamp AMD environment now stalls ~10x longer than before, multiplied across every concurrency point of every AMD sweep run.
Verification: nit. Factually correct but bounded-slowdown, not a functional failure. _wait_for_amd_stop_coverage (benchmarks/benchmark_lib.sh:384-400) has no legacy-fixed-tail-wait fallback: when _amd_monitor_min_covered_tick always returns empty (stream with no usable numeric-epoch row), line 388 (-n "$covered") never fires, line 391 death branch never fires while the awk sink GPU_MONITOR_PID stays alive,…
只忽略已定义的诊断 sidecar，保留损坏与缺失点门禁；旧处理器逐点处理并累计失败，新版导入错误不回退。
继承结果处理兼容性修复，保持 AMD 采集逻辑不变。
继承已验证的 sidecar 分类与旧版本逐点结果处理，并保留 UTC 产物测试。
继承共享测试文件的注释格式恢复。
恢复合并时意外改动的测试注释分隔线与空行。
遥测拷贝失败只在必需功耗且服务成功时使作业失败，保留已有服务错误码。
独立核对每种角色的 GPU 数量，并记录窗口外异常样本，窗口内异常仍阻止功耗有效性。
保留 AMD 暂存失败语义并继承原生验证修复。
发布就绪收据前检查采集进程仍存活，并将 AMD 结束身份采集失败写入失败收据。补充真实进程与清单回归测试。
在实际 Docker 环境参数中传递 REQUIRE_POWER，并用嵌套 shell 命令展开结果验证服务与压测的严格失败策略。
同步共享采集器就绪检查与 AMD 结束身份失败收据修复。
中文：合并 main，保留 AMD 监控与多节点采集修复，并保持原生采集器独立 UTC 边界。
中文：合并 main 的 GLM MI355X 配方更新，保留历史变更记录及本 PR 的 AMD 功耗条目。
中文：合并 main 的 TileRT 原生支持，保留 AMD 运行逻辑并合并测试入口、文档及变更记录。
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
中文：合并已验证的 GLM GB200 功耗支持，保持 AMD 行为并追加本 PR 的独立变更记录。
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
中文：同步主分支并保留 AMD 功耗采集改动及完整历史更新记录。
同步已合入的 H200 基础代码，仅解决 changelog 追加冲突；保留本 PR 的运行配置和既有证据。
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
合入最新主线，保留中英文 B300 与 AMD 采集说明及追加式变更记录。
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.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ebaba2a. Configure here.
| if ! wait_amd_multinode_power ready; then | ||
| [[ "$power_required" == 0 ]] || exit 1 | ||
| echo 'PowerX: continuing without ready optional telemetry' >&2 | ||
| fi |
There was a problem hiding this comment.
Ready failure leaves collectors running
Medium Severity
When wait_amd_multinode_power ready fails and REQUIRE_POWER is set, bench.sh exits without writing stop. Surviving collectors keep running until the server EXIT trap kills them, which is after prefill/decode teardown. Coordination is also skipped entirely if rank 0 never exported POWERX_CONTROL_DIR after a rank-local start failure.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ebaba2a. Configure here.
中文：同步主分支并按九条完整配置曲线验证 AMD 采集路径，保留共享生命周期修复并暂缓 DSV4 8K/1K。
中文：暂缓未启用的 ATOM/vLLM 多节点退出改动，明确当前 SGLang 验证及 AgentX 采集边界。
1 participant
Description
Make AMD monitor shutdown wait for usable end samples and coordinate native worker collectors without masking serving failures. Validate nine complete configuration curves across MI300X, MI325X and MI355X; exclude retired DSV4 8k1k.
Testing: 195 focused CPU tests, Bash/YAML and changelog/matrix checks passed.
Pending: 59 benchmark jobs, 32 eval jobs, hardware power acceptance and CODEOWNER sign-off. Multinode AgentX power remains disabled; inactive ATOM/vLLM exit fixes are deferred.
中文
AMD 监控停止前等待有效终点样本，并协调各工作节点采集器，同时保留服务失败状态。使用 MI300X、MI325X 和 MI355X 的九条完整配置曲线验证，排除已退役 DSV4 8k1k。
测试： 195 项定向 CPU 测试、Bash/YAML 和 changelog/矩阵检查通过。
待完成： 59 个 benchmark 作业、32 个 eval 作业、硬件功耗验收与 CODEOWNER 签核。多节点 AgentX 功耗仍未开启；未启用的 ATOM/vLLM 退出修复暂缓。
Related Issue
Independent on main; #3051 has merged. Split from #3026.
直接基于 main；#3051 已合并。拆分自 #3026。
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 power teardown, Slurm/Docker launch paths, and exit-code semantics for many AMD recipes; incorrect coverage or receipt handling could invalidate power or mask serving failures when
REQUIRE_POWERis set.Overview
AMD single-node monitoring in
benchmark_lib.shno longer uses a fixed sleep before killingamd-smi. Stop now pollsgpu_metrics.csvuntil every GPU has a positive power sample at or after the first whole second past the stop request (AMD_MONITOR_STOP_TIMEOUT_S, default 30;0skips). The AMD path uses a per-run FIFO so bothamd-smiand theawkconsumer are torn down cleanly; FIFO failure honorsREQUIRE_POWER. AgentX signal/EXIT cleanup sets the timeout to0so cancelled runs stop quickly.AMD multinode PowerX adds
power.shand wires it throughserver.sh,bench.sh, andjob.slurm: per-nodenative_power_collect.shfor fixed 8192/1024 (non-agentic) runs, ready/done receipts via a shared control dir,stopwhile servers are still up, staging ofnative_poweras the runner user, and Docker env forREQUIRE_POWER, clock sync context, and collector revision. Exit codes keep serving failures primary; collector/staging failures fail the job only when power is required. SlurmNUM_NODESnow reflects physical nodes when TP spans hosts. MI355x CI copiesnative_powerintoLOGS.Docs,
perf-changelog.yaml(broad AMD 8k/1k configs), and shell-contract tests cover stop coverage, lifecycle policy, and wiring.Reviewed by Cursor Bugbot for commit ebaba2a. Bugbot is set up for automated code reviews on this repo. Configure here.