[PowerX] support opt-in NVIDIA SRT 8k1k power / 支持按需启用 NVIDIA SRT 8k1k 功耗 - #3027
[PowerX] support opt-in NVIDIA SRT 8k1k power / 支持按需启用 NVIDIA SRT 8k1k 功耗#3027edwingao28 wants to merge 23 commits into
Conversation
|
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 关于重新运行失败任务的文档 |
c905ebf to
1230737
Compare
1230737 to
fb6acb9
Compare
|
Claude finished @edwingao28's task in 11m 35s —— View job Review of PR #3027
SummaryNo blocking issues found. Three 🟡 warnings posted inline:
Checked and fine: workflow |
1a75f68 to
42bd42c
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| git clone https://github.com/edwingao28/srt-slurm.git "$destination" || return | ||
| cd "$destination" || return | ||
| git checkout --detach "$revision" || return | ||
| [[ "$(git rev-parse HEAD)" == "$revision" ]] || return 1 | ||
| cp -a "$GITHUB_WORKSPACE/benchmarks/multi_node/srt-slurm-recipes/." recipes/ || return |
There was a problem hiding this comment.
🟡 WARNING: Every fixed 8K/1K srt-slurm lane on gb200/gb300/b200/b300/h100/h200 now takes this single clone path, but only in-repo recipes are overlaid. configs/nvidia-master.yaml still references roughly 200 fixed 8K/1K recipes that have no checked-in copy and previously came from lane-specific upstream refs (sa-submission-q2-2026, v1.0.25, v1.0.31, main): all recipes/trtllm/{b200,b300,h100,h200,gb200,gb300}-*/8k1k/**, recipes/h200/8k1k/*, recipes/h100/8k1k/*, recipes/gb200-fp8/8k1k/{mid-curve,max_tpt}.yaml, recipes/gb200-fp4/8k1k/*, recipes/gb300-fp{4,8}/8k1k/*, recipes/b200-fp{4,8}/8k1k*.yaml, and recipes/DeepSeek-V4-Pro/disagg/**.
Why it matters: If any of those is absent from edwingao28/srt-slurm@3f3b7af, prepare_srt_power.py fails on source.read_text() (or the launcher's "CONFIG_FILE does not exist" check fires) and the whole 8K/1K matrix for that lane goes red at submit time. The fact that low-latency-1p1d.yaml had to be added explicitly for the GB200 DSR1 lane suggests the fork tree does not mirror every upstream branch. I could not verify the fork tree from this sandbox (no network).
Fix: Confirm every CONFIG_FILE= path in the NVIDIA master config resolves under the pinned fork commit after the overlay, e.g. run the equivalent of git ls-tree -r --name-only 3f3b7af26e -- recipes/ against the extracted CONFIG_FILE list, and check in (or add to the fork) any recipe that is missing before enabling the requirement.
| fi | ||
| if [[ ! -f "$POWERX_CONTROL_DIR/$phase-$rank" ]]; then | ||
| pending=1 | ||
| elif [[ "$phase" == done && "$(cat "$POWERX_CONTROL_DIR/done-$rank")" != 0 ]]; then |
There was a problem hiding this comment.
🟡 WARNING: The done-<rank> receipt is read from a shared control directory (llm-d: $BENCHMARK_LOGS_DIR/power_control-*; TileRT: $GITHUB_WORKSPACE/LOGS/power_control-*) that another node writes with printf '%s\n' > path followed by chown. On NFS/Lustre the directory entry can become visible before the data is flushed, so a poller here can observe an existing but empty file. "" != 0 is true, so failed=1 is latched and the function returns 1 on the very next iteration with no retry.
Why it matters: A perfectly good run reports "One or more PowerX collectors failed" and, with REQUIRE_POWER=1 now forced for all 8K/1K runs, the job fails. Same class of race exists for the ready-<rank> receipt (harmless, since only existence is checked) and for the TileRT sentinel (see comment in run_node.sh).
Fix: Treat an empty receipt as still pending, or publish receipts atomically (write to name.tmp, chown, then mv -f to name) in both powerx_write_control here and write_control in native_power_collect.sh.
| elif [[ "$phase" == done && "$(cat "$POWERX_CONTROL_DIR/done-$rank")" != 0 ]]; then | |
| elif [[ "$phase" == done ]] && { receipt=$(<"$POWERX_CONTROL_DIR/done-$rank"); [[ -z "$receipt" ]]; }; then | |
| pending=1 | |
| elif [[ "$phase" == done && "$receipt" != 0 ]]; then |
| if [[ -f "$DONE_SENTINEL" ]]; then | ||
| echo "[decode] done sentinel received, shutting down"; kill "$DECODE_PID" 2>/dev/null || true; exit 0 | ||
| echo "[decode] done sentinel received, shutting down" | ||
| exit "$(cat "$DONE_SENTINEL")" |
There was a problem hiding this comment.
🟡 WARNING: The decode role polls -f "$DONE_SENTINEL" every 5 s on the shared workspace and then exits with the file's content. The prefill node writes the sentinel with printf > file and only afterwards chowns it, so a decode read that lands between create and data flush yields an empty string. exit "" is a bash error ("numeric argument required") and the step exits 2, which finish_tilert_submit then propagates as the job status because it now takes the decode step's rc when prefill was 0.
Why it matters: A green benchmark is reported as failed with no diagnostic beyond "exit 2".
Fix: Read the sentinel once, treat empty as "not yet written" and keep polling, or have the prefill side write to a temp name and mv -f it into place so the file only ever appears fully written.
| exit "$(cat "$DONE_SENTINEL")" | |
| sentinel_rc=$(<"$DONE_SENTINEL") | |
| exit "${sentinel_rc:-1}" |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large change (72 files) touching multi-node SLURM orchestration, the new PowerX native power-collector lifecycle, and permission/ownership handling across the NVIDIA and AMD launchers, a human look would still be worthwhile.
What was reviewed: the new PowerX control/raw-data directory permissions (chmod 777) in benchmarks/multi_node/llm-d/job.slurm — needed for cross-container writability under the existing SLURM trust boundary, not a new exposure. The USES_DCGM_POWER allow-list bypass for powerx_fixed_8k1k in runners/launch_gb200-nv.sh — scoped to the new fixed-8k1k recipe path rather than a general bypass. Reuse of wait_owned_step/TILERT_DECODE_DRAIN to bound the power-data copy in benchmarks/multi_node/tilert_utils/submit.sh — bounded by the existing drain timeout. The shared control_dir/stop file and single-coordinator shutdown semantics in benchmarks/multi_node/llm-d/server.sh — consistent with the pre-existing done-marker coordination model.
Extended reasoning...
Overview
This PR wires a new "PowerX" native power-measurement requirement into the 8k1k benchmark launchers across NVIDIA and AMD routes. It touches SLURM job/step scripts (job.slurm, server.sh, submit.sh), the per-runner launcher scripts (launch_*-nv.sh, launch_*-slurm.sh), a new powerx_8k1k.sh/native_power_lifecycle.sh/prepare_srt_power.py set of shared helpers, several new deepseek-v4 SLURM recipe YAMLs, docs, and a perf-changelog entry. 72 files changed in total, with the bulk of behavioral risk concentrated in a handful of bash scripts that coordinate multi-node collector startup/drain/teardown and permission handling for shared scratch directories.
Security risks
No injection, auth-bypass, or credential-exposure patterns were found. The main risk category is process/permission robustness in a shared multi-tenant SLURM environment: world-writable (chmod 777) scratch directories for the new native collector, and a pinned third-party fork commit for the SRT runtime (runners/powerx_8k1k.sh) whose exact compatibility changes are not fully described upstream. Neither rises to a classic security vulnerability, but the world-writable directories and pinned external commit are the kind of details a maintainer familiar with the SLURM cluster's trust model should confirm are acceptable.
Level of scrutiny
This warrants human review rather than an automatic approval. The diff is large (72 files), spans multiple distinct subsystems (SLURM job orchestration, per-node collector lifecycle with readiness/drain barriers, recipe YAML additions, CI workflow templates), and the PR description itself flags several pre-merge qualification steps (runner availability, final runtime pin, cancellation/ownership behavior, a full green sweep) that have not yet been completed. That combination — complexity, cross-node coordination logic, and the author's own acknowledgment of open validation gaps — puts this above the bar for a mechanical/config-only approval even though no concrete bug was found in this pass.
Other factors
The PR reports 72 passing targeted unit/integration tests (with 4 skipped pending a pinned dependency) and passing syntax/YAML/changelog checks, which is reasonable coverage for the parts that can be tested without GPU hardware. The four candidate concerns identified during review (chmod 777 scratch dirs, the DCGM allow-list bypass scoped to the new path, shared drain-timeout reuse for the power-data copy, and the single-coordinator stop-file semantics) were each examined against the surrounding code and did not show a clear defect, but they sit in cross-node coordination paths that are inherently harder to fully validate without live multi-node runs, reinforcing the case for a human's additional look before merge.
172482b to
bccf199
Compare
79e1b4b to
0e73cc6
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
保留完整请求结果状态与诊断批次，拆分 Slurm 生命周期和原生功耗采集。
为固定序列场景增加默认关闭的功耗要求，并贯通矩阵及工作流；保持 AgentX 和评估隔离。
验证 Slurm 完成状态并让 llm-d 工作进程正常退出，先保存诊断证据。
整合必要的 Slurm 完成状态契约。 # Conflicts: # docs/results-and-ingestion.md # docs/results-and-ingestion_zh.md # perf-changelog.yaml
整合按场景启用功耗的矩阵契约。
cd0870f to
c304072
Compare
支持按需启用 NVIDIA SRT 实测功耗，保留固定 runtime、所选配方及生产者证据；不默认启用场景。
同步 Slurm 前置交付引用。 # Conflicts: # docs/results-and-ingestion.md # docs/results-and-ingestion_zh.md # perf-changelog.yaml # runners/launch_gb200-nv.sh
保留每个测量窗口的 SMI 上下文工件，覆盖原 #3027 的共享上传规则。
0e73cc6 to
5eb5ca1
Compare
将 SRT 功耗快照与复制行为限制到显式启用路径，保留现有 AgentX 和普通路由行为。
edwingao28
changed the title
整合严格匹配 Slurm ExitCode 字段的修复。
将新增功耗回归测试与相关路径接入现有 CPU 工作流。
同步共享 Slurm 生命周期 CPU 测试接线。
矩阵生成同时识别校验允许的字段名与别名，避免静默丢失功耗门槛。
同步矩阵字段别名的回归修复。
保留主分支已发布的 Kimi-K3 B200 改动，将当前契约声明追加至 changelog 末尾。
保留普通固定序列与 AgentX 路由隔离、失败结果保留和 CPU 测试并集。
同步结果契约前置并保留各 scope 的独立条目。
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 364f2c5. Configure here.
同步 Slurm 独立前置的已验证信号就绪测试修复。
删除功耗分支对 GB200 模型路径和 GB300 模型别名的覆盖，沿用已解析映射。
只忽略已定义的诊断 sidecar，保留损坏与缺失点门禁；旧处理器逐点处理并累计失败，新版导入错误不回退。
同步结果前置的 sidecar 排除及旧版处理器失败传播。
中文：同步已合入的请求诊断修复，保留 SRT 功耗路由及 Slurm 前置变更。
中文：保留 Slurm 完成状态前置提交，明确 SRT 功耗路由的依赖关系。
同步已合入的原生功耗基础设施和 Slurm 完成竞态修复，保留 SRT 与原生采集的完整测试入口。
1 participant
Description
Support explicitly requested NVIDIA SRT 8192/1024 power using the audited pinned runtime. Resolve overrides, inject concurrencies and retain failure evidence. No scenario enables it by default.
Testing: 33 targeted checks passed, including four schema cases using the prepared pinned runtime.
Scope: Depends on #3052; hardware qualification remains pending.
中文
使用已审计的固定 runtime，支持显式请求的 NVIDIA SRT 8192/1024 功耗；解析 override、注入并发度并保留失败证据，默认不启用。
测试： 33 项针对性检查通过，包括在已准备的固定 runtime 下完成的四项 schema 检查。
范围： 依赖 #3052；硬件资格验证仍待完成。
Related Issue
Parent #3052; #3026 and #3050 are merged. Native paths: #3056 and #3057.
前置为 #3052，#3026 与 #3050 已合并；原生路径见 #3056、#3057。
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
Touches many Slurm launchers and srt-slurm checkout/prepare paths, but activation is gated on REQUIRE_POWER plus 8k1k fixed-seq-len and adds targeted tests.
Overview
Adds an opt-in PowerX path for fixed-sequence 8192/1024 multinode SRT runs when
REQUIRE_POWER=1(not AgentX, not eval-only). Shared helpers inpowerx_8k1k.shdetect the lane, clone a pinnedsrt-slurmcommit, and on exit snapshot logs/provenance even on failure.prepare_srt_power.pyresolves the selected recipe override, injects matrix concurrencies, maps DeepSeek-V4 tokenizer settings, and writes a derived recipe with required DCGM-power telemetry beforesrtctlsubmit. Slurm launchers for B200/B300/GB200/GB300/H100/H200 (and compat paths) source the helper, bypass existing dcgm-power allowlists only on this lane, and merge-copyLOGSinstead of replacing prior artifacts. H100 gains DCGM exporter squash setup when power is active; H200 keeps AgentX power injection separate from the fixed-sequence path.Documentation (EN/ZH), a broad
perf-changelog.yamlentry (no master scenario enables it by default), CI path triggers, andtest_prepare_srt_power.pycover the new behavior.Reviewed by Cursor Bugbot for commit 20eb5c0. Bugbot is set up for automated code reviews on this repo. Configure here.