Skip to content

feat(power): enable official dcgm energy lane for gb200/gb300 1p1d / 打开 gb200/gb300 官方 1P1D 能耗采集 - #2456

Open
edwingao28 wants to merge 4 commits into
wenyao/power-pr3-multinode-consumptionfrom
wenyao/power-pr4-official-energy
Open

feat(power): enable official dcgm energy lane for gb200/gb300 1p1d / 打开 gb200/gb300 官方 1P1D 能耗采集#2456
edwingao28 wants to merge 4 commits into
wenyao/power-pr3-multinode-consumptionfrom
wenyao/power-pr4-official-energy

Conversation

@edwingao28

Copy link
Copy Markdown
Collaborator

Draft on purpose: the producer pin points at the validated fork SHA (edwingao28/srt-slurm@6609d46). Before this leaves draft, the pin swaps to the upstream srt-slurm merge SHA (one constant per launcher) and both platforms re-run a c4 smoke.

Stacked on #2437 (multinode consumer), which stacks on #2323 (single-node).

What this adds

  • telemetry block in the two official 1P1D 8k1k recipes. gb300 uses port 19401: 9401 is already bound by the cluster-level exporter on im-gb300 nodes.
  • lane-scoped provisioning in the gb200/gb300 launchers: a run opts in only when its recipe carries an enabled dcgm-power block. Every other model/recipe keeps the exact same path as before.
  • producer pin contract: power lanes clone the pinned SHA, assert HEAD matches, and write power-producer-sha.txt. CI derives POWER_PRODUCER_SHA from that stamp; the workflow input stays as a manual override.
  • contract tests for the above (utils/test_gb200_power_official_contract.py, utils/test_gb300_power_official_contract.py).

Scope: 1P1D only. No nvidia-master.yaml change. Dispatches for this PR use exact-key test-config, not full-sweep.

Validation so far (fork pin): GB200 run 30618706258 (340.5 W/GPU avg, 193,404 J), GB300 run 30663050396 (349.3 W/GPU avg, 172,800 J), both with power_valid=1 and stored==recomputed sidecars. Local: 124 tests pass.

中文:为 gb200/gb300 官方 1P1D lane 打开 DCGM 能耗采集。recipe 声明 telemetry(gb300 用 19401 端口),launcher 按 recipe 判定是否 provision exporter 与 pin producer(非 power lane 路径不变),CI 从 launcher stamp 读取 POWER_PRODUCER_SHA。当前 pin 指向已验证的 fork SHA,upstream merge 后换 pin 并重跑双平台 c4 smoke,再转正式 review。

…in gb launchers

中文:launcher 按 recipe 判定 power lane,provision exporter 并 pin producer;非 power lane 行为不变。
…de template

中文:CI 从 launcher stamp 读取 producer SHA,workflow input 保留为手动覆盖。
中文:两个官方 1P1D recipe 声明 telemetry;gb300 用 19401 端口避开集群级 exporter。
中文:契约测试覆盖 lane 检测、恰好两个 power recipe、pin 单一来源与 stamp 一致性。
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

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 As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

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 模板,包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have

如需进行 PR 验证,请为此 PR 添加 full-sweep-fail-fast 标签(强烈推荐)— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动(flake),重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

@edwingao28
edwingao28 marked this pull request as ready for review August 3, 2026 06:25
@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @edwingao28's task in 3m 59s —— View job


Review — PR #2456 (official dcgm energy lane, gb200/gb300 1P1D)

  • Gather context (diff vs wenyao/power-pr3-multinode-consumption, read changed files)
  • Review launcher lane-gating & producer-pin logic
  • Review recipe telemetry blocks & workflow changes
  • Verify contract tests (statically — pytest execution isn't permitted in this environment; CI runs them via test-process-result.yml)
  • Post review feedback

LGTM - no blocking issues found.

Things I specifically checked and confirmed sound:

  • Lane detection ordering: USES_DCGM_POWER is derived from the workspace recipe mirror before the srt-slurm clone in both launchers, and CONFIG_FILE's :selector suffix is stripped (${CONFIG_FILE%%:*}) before path resolution, matching how CONFIG_PATH is handled later.
  • Non-power path preservation: every power addition (exporter import, pinned clone, sed yaml injection, provenance copies) is gated on USES_DCGM_POWER == 1; gb300's fp8/fp4 ref logic (v1.0.25 / sa-submission-q2-2026) is byte-preserved in the else branch.
  • sed injection target: nginx-sqsh: exists exactly once in both generated srtslurm.yaml heredocs, so the append lands inside the containers map in both launchers.
  • Workflow wiring: POWER_PRODUCER_SHA is job-level env from the input, so the -z check correctly treats the input as an override; the stamp is written to $GITHUB_WORKSPACE during the launch step and read relative to the default working dir in the same job. Stale-stamp carryover across runs is prevented by actions/checkout with clean: true before the launch step.
  • New repo-wide invariant (test_workflows_carry_no_producer_sha_literal): passes against the current tree (no non-uses: 40-hex literals in any workflow).

One non-blocking observation (no action needed now): the lane check is two independent greps (provider: dcgm-power + enabled: true anywhere in the recipe), so a future recipe with telemetry.enabled: false plus an unrelated enabled: true elsewhere would false-positive. The test_exactly_two_recipes_opt_into_dcgm_power contract currently fences this in, so it's fine for this PR's scope — just something to keep in mind if telemetry blocks spread to more recipes.

Also noting the PR's own plan: the producer pin currently points at the fork SHA (edwingao28/srt-slurm@6609d46) — per the description this swaps to the upstream merge SHA plus a two-platform c4 smoke before merge. The test_pin_constants_identical_across_launchers / test_pin_literal_lives_only_in_the_two_launchers tests will keep that swap honest.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bugs were found, but I'm deferring to a human reviewer given the scope and risk profile of this change. This PR is explicitly marked draft-on-purpose — the producer pin currently points at a third-party fork (edwingao28/srt-slurm) rather than the eventual upstream merge SHA — and it modifies the critical gb200/gb300 multinode launcher scripts that gate all benchmark submissions on those clusters, not just the new power lane.

What was reviewed: the recipe-driven power-lane detection logic in both launchers (grep-based, resolved against the workspace recipe mirror before the srt-slurm clone exists), the pinned-clone-and-assert-HEAD contract that writes power-producer-sha.txt, the CI step in benchmark-multinode-tmpl.yml that derives POWER_PRODUCER_SHA from that stamp (input still overrides), and the new contract tests covering both launchers. The bug hunting system additionally checked the gb300 dcgm-exporter image enroot import syntax and ruled it out as correct.

Extended reasoning...

Overview

This PR wires an official DCGM power-telemetry lane into the gb200/gb300 1P1D qwen3.5 recipes. It adds a telemetry block to the two recipe YAMLs, adds recipe-gated (grep-detected) exporter provisioning and a pinned srt-slurm producer clone to both runners/launch_gb200-nv.sh and runners/launch_gb300-nv.sh, threads a power-producer-sha.txt stamp from the launcher through benchmark-multinode-tmpl.yml into POWER_PRODUCER_SHA, and adds static contract tests (utils/test_gb200_power_official_contract.py, utils/test_gb300_power_official_contract.py) asserting the launcher text matches the intended contract. Non-power lanes are intended to be byte-identical to their pre-PR behavior (exporter/pin logic is gated behind a USES_DCGM_POWER flag derived from the recipe).

Security risks

The main risk is supply-chain: the power lane clones and executes setup/build steps from a personal fork (edwingao28/srt-slurm) rather than the org-controlled NVIDIA/srt-slurm. This is mitigated by pinning to an exact 40-hex commit SHA and asserting git rev-parse HEAD matches it before proceeding, and the PR description itself states this is a temporary, draft-only state pending the real upstream merge SHA. Still, until that swap happens, CI on official gb200/gb300 power-lane runs executes code from a non-org-controlled repository. No other injection/auth-bypass/data-exposure concerns stood out — the new grep-based recipe detection only reads local repo files, and the sed/yaml edits are scoped to generated config, not user-controlled input.

Level of scrutiny

This warrants a higher level of scrutiny than a typical recipe or config PR: it touches the shared launcher scripts (runners/launch_gb200-nv.sh, runners/launch_gb300-nv.sh) that every model/precision/framework combination on those clusters depends on, and it is explicitly flagged by the author as draft/not-yet-final (fork pin instead of upstream pin, pending a second smoke test on both platforms). A human familiar with the srt-slurm producer and the gb200/gb300 CI paths should confirm the pin-swap plan and re-validate before this leaves draft.

Other factors

The author has already done meaningful validation (real GB200/GB300 runs with power_valid=1 and stored==recomputed sidecars, 124 local tests passing) and added static contract tests enforcing the launcher/recipe/workflow text stays in sync. The bug-hunting system found no bugs. Given the explicit draft status and the fork-pin caveat called out by the author themselves, this PR is not yet intended to be merged as-is, so deferring for human sign-off rather than approving is the right call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant