[PowerX] isolate AMD monitor windows and worker lifecycle / 隔离 AMD 监控窗口与工作节点生命周期 - #3055
[PowerX] isolate AMD monitor windows and worker lifecycle / 隔离 AMD 监控窗口与工作节点生命周期#3055edwingao28 wants to merge 33 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 暂存失败语义并继承原生验证修复。
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 8c2f834. Configure here.
发布就绪收据前检查采集进程仍存活，并将 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 采集说明及追加式变更记录。
1 participant
Description
Bound AMD monitor shutdown by usable sample coverage, coordinate worker collectors, and forward required-power policy into containers while preserving optional serving outcomes.
Testing: Linux CPU CI: 534 passed, including Docker policy propagation and collector receipt regressions.
Pending: AMD hardware qualification, sweep/evals and CODEOWNER sign-off.
中文
按有效样本覆盖限制 AMD 监控停止时间，协调工作节点采集器，将严格功率策略传入容器，同时保留可选遥测模式下的服务运行状态。
测试： Linux CPU CI 534 项通过，包含 Docker 策略传递与采集器回执回归测试。
待完成： AMD 硬件资格验证、sweep/评估及 CODEOWNER 签核。
Related Issue
Depends on #3051; split from #3026, including #3041 repairs.
依赖 #3051；拆分自 #3026，包含 #3041 修复。
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, Slurm job exit codes, and power validity gates across many AMD fixed-sequence configs; mistakes could leak monitor processes or mis-attribute benchmark success vs optional telemetry failures.
Overview
This PR tightens AMD PowerX collection for fixed 8K/1K multinode Slurm runs and makes single-node AMD
stop_gpu_monitorwait for real telemetry coverage instead of a fixed sleep.Single-node AMD monitor (
benchmark_lib.sh) routesamd-smithrough a per-run FIFO, tracks and kills both the producer andawkconsumer, and on stop pollsgpu_metrics.csvuntil every GPU has a positive power sample at or after the next whole second (bounded byAMD_MONITOR_STOP_TIMEOUT_S, default 30;0skips). AgentX abort teardown sets the timeout to0so cancelled runs do not block on coverage. FIFO setup failures fail the run only whenREQUIRE_POWERis set.AMD multinode launchers add
power.shto startnative_power_collect.shper serving node (topology-aware GPU indices), coordinateready/stop/donereceipts underpower-control-<job>, and wirebench.sh/server.sh/job.slurmso collector or staging errors respectREQUIRE_POWERwithout masking benchmark exit codes. Slurm jobs preserve physical node counts, pass PowerX env into containers, stagenative_poweras the runner user, and propagate step RC when power is required. MI355x CI copies stagednative_powerintoLOGS/before cleanup.Docs,
perf-changelog.yaml, CI path filters, and broad shell-contract tests cover the new stop logic and lifecycle behavior.Reviewed by Cursor Bugbot for commit 44a4dc1. Bugbot is set up for automated code reviews on this repo. Configure here.