feat(power): validate single-node GPU energy metrics / 校验单节点 GPU 能耗指标 - #2322
feat(power): validate single-node GPU energy metrics / 校验单节点 GPU 能耗指标#2322edwingao28 wants to merge 3 commits into
Conversation
Integrate per-device telemetry over the formal benchmark window, enforce expected GPU topology, emit auditable validity artifacts, and add best-effort and strict power modes with CPU coverage. 中文：校验单节点 GPU 能耗指标。基于正式基准测试窗口对逐卡遥测数据进行积分，校验预期 GPU 拓扑，输出可审计的有效性产物，并为默认尽力模式和严格功耗模式补充 CPU 测试覆盖。
Treat unparseable timestamps and in-window power values as validation failures so best-effort runs emit power_valid=0 and strict runs fail without publishing Joule metrics. Add regression coverage for both cases. 中文：拒绝格式异常的 GPU 遥测样本。将无法解析的时间戳和窗口内功耗值视为校验失败，使尽力模式输出 power_valid=0，严格模式失败，并避免发布 Joule 指标；同时补充两个回归测试。
|
@claude review |
|
Claude finished @edwingao28's task in 5m 53s —— View job Code Review
SummaryThis is a well-built PR. The per-device trapezoidal integration with boundary interpolation is mathematically correct (I verified the test arithmetic), the validation reason codes are auditable, writes are atomic, the strict/best-effort split via Findings1. HIGH —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, three other candidate issues were examined this run and ruled out: an unparseable timestamp mid-CSV doesn't invalidate the whole run (by design — only power/GPU-id values outside tolerance do; timestamp parse failures are dropped per-row); multinode + REQUIRE_POWER=1 fails the job without writing a power_validation sidecar (multinode telemetry is explicitly out of scope for this PR, so no sidecar is expected there); and the power metric key list is duplicated between aggregate_power.py's _POWER_METRIC_KEYS and process_result.py's record_power_internal_error (cosmetic duplication, not a correctness issue).
Extended reasoning...
This PR is large and touches benchmark-result-affecting logic (new power integration/validation algorithm, fail-closed REQUIRE_POWER semantics, and several CI workflow changes), so it warrants human review regardless of the minor docstring nit already flagged inline. The three items above were raised by finder agents and investigated this run; none turned out to be bugs, but recording them here avoids re-litigating the same questions in a future pass.
| def _append_reason(reasons: list[str], reason: str) -> None: | ||
| if reason not in reasons: | ||
| reasons.append(reason) | ||
|
|
||
|
|
||
| def _gpu_sort_key(gpu_id: str) -> tuple[int, int | str]: | ||
| return (0, int(gpu_id)) if gpu_id.isdigit() else (1, gpu_id) | ||
|
|
||
|
|
||
| def _empty_integration( | ||
| *, | ||
| expected_num_gpus: int | None, | ||
| reasons: list[str], | ||
| ) -> PowerIntegration: | ||
| return PowerIntegration( | ||
| power_valid=False, | ||
| invalid_reasons=tuple(reasons), | ||
| expected_num_gpus=expected_num_gpus, | ||
| observed_gpu_ids=(), | ||
| per_gpu_sample_counts={}, | ||
| per_gpu_max_sample_gap_s={}, | ||
| per_gpu_energy_j={}, | ||
| device_issues={}, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🟡 Five new private helpers in utils/aggregate_power.py (_append_reason, _gpu_sort_key, _empty_integration, _write_json_atomic, _validation_payload) lack docstrings, even though AGENTS.md's Python conventions call for docstrings on functions and every other function in this file (pre-existing and newly added) has one. Not blocking — just add one-line docstrings to these five for consistency.
Extended reasoning...
What the bug is. AGENTS.md's "Code Conventions" section (line 76) explicitly lists "docstrings on functions" as a Python convention for this repo. utils/aggregate_power.py follows this convention consistently everywhere else: every pre-existing function, including private helpers like _parse_timestamp, _parse_power, and _detect_columns, carries a docstring, and the majority of new functions this PR adds (_interpolate_power, _integrate_device, integrate_power, _load_benchmark_data, _derived_metrics, run) do too. However, five new private helpers introduced by this PR have no docstring at all — their bodies start directly with code:
_append_reason(~line 236)_gpu_sort_key(~line 240)_empty_integration(~line 244)_write_json_atomic(~line 607)_validation_payload(~line 671)
Where this shows up in the diff. All five are net-new additions in this PR's diff to utils/aggregate_power.py — they did not exist before and so cannot be attributed to prior technical debt. Each is genuinely a fresh omission introduced alongside sibling functions in the same PR that do follow the convention, which is what makes this a real (if narrow) inconsistency rather than pre-existing drift.
Why existing tooling does not catch this. The PR description states Ruff and mypy both passed. Neither tool enforces docstring presence by default (that would require enabling a plugin like pydocstyle/ruff --select D, which this repo does not appear to have configured), so a missing docstring on a private helper is silent at both lint and type-check time. Nothing else in CI (the pytest suite, workflow contract tests) exercises documentation completeness either.
Impact. Purely cosmetic — none of the five helpers has any runtime, correctness, or behavioral effect from missing a docstring. They are also short and largely self-explanatory from their names and one-line bodies (e.g. _gpu_sort_key returns a sort key tuple; _write_json_atomic writes JSON to a temp file and renames it). This does not affect merge safety.
Proof (concrete diff excerpt). Compare a passing case to a failing case within the same PR diff:
def _interpolate_power(samples: list[tuple[float, float]], timestamp: float) -> float:
"""Linearly interpolate power at a timestamp bracketed by ``samples``."""
...
def _gpu_sort_key(gpu_id: str) -> tuple[int, int | str]:
return (0, int(gpu_id)) if gpu_id.isdigit() else (1, gpu_id)_interpolate_power (also new in this PR) has a docstring immediately following the def line; _gpu_sort_key (also new in this PR, defined a few lines earlier at ~line 240) jumps straight to its return statement. The same asymmetry repeats for _append_reason, _empty_integration, _write_json_atomic, and _validation_payload versus their well-documented siblings _integrate_device, integrate_power, _load_benchmark_data, and _derived_metrics.
Suggested fix. Add a one-line docstring to each of the five helpers describing its purpose/return value, matching the style used elsewhere in the file (e.g. """Return a sort key that orders numeric GPU IDs before non-numeric ones.""" for _gpu_sort_key). This is a few minutes of work and does not require any behavioral change.
Remove an incomplete trailing nvidia-smi CSV row before appending the deterministic post-benchmark sample, preventing row concatenation from silently corrupting energy integration. Add a shell-lifecycle regression test covering the interrupted-write case. 中文：在追加确定性的基准测试后采样前，删除未写完整的 nvidia-smi CSV 末行，避免行拼接静默污染能耗积分；新增覆盖中断写入场景的 shell 生命周期回归测试。
|
Superseded by #2323. I reopened this from an upstream branch so repository CI, automated review, and the H100/H200 canary can run with the required permissions. The same implementation history, including the truncated NVIDIA telemetry-row fix, is preserved in the replacement PR. 中文：本 PR 已由 #2323 替代。新的 PR 从 upstream 分支提交，使仓库 CI、自动代码审查和 H100/H200 canary 能够获得所需权限并正常运行；原有实现历史及 NVIDIA 遥测残行修复均已保留。 |
2 participants
Summary
avg_power_wmeaning and add whole-deploymentavg_total_gpu_power_w,total_gpu_energy_j, J/successful query, and J/input, output, and total token.power_valid=0plus an auditable validation sidecar when telemetry is invalid;REQUIRE_POWER=1makes study/canary jobs fail after artifacts are written.gpu_metrics.csv, aggregate JSON, and validation JSON as an independent power audit bundle.This builds on the measured-power path introduced in #1558. It keeps the single-node aggregation scope separate from the multi-node and role-level work discussed in the closed #1635.
Scope
This PR covers single-node, non-disaggregated fixed-sequence serving, with H100/H200 8k/1k as the initial validation target.
Not included: GB200/GB300 multi-node or disaggregated telemetry, prefill/decode role attribution, srt-slurm changes, MI355X hardware validation, 4P1D/8P1D rack-scale experiments, repeated publication runs, plots, or article work.
Validation
python -m pytest utils/test_aggregate_power.py utils/test_process_result.py -q— 80 passed中文说明
avg_power_w语义，并新增整套部署的avg_total_gpu_power_w、total_gpu_energy_j、每个成功查询的能耗，以及每个输入、输出和总 token 的能耗。power_valid=0和可审计的校验 sidecar；设置REQUIRE_POWER=1后，研究或 canary 任务会在写出产物后失败。gpu_metrics.csv、聚合 JSON 和校验 JSON。本 PR 基于 #1558 引入的实测功耗路径，并将单节点聚合范围与已关闭的 #1635 中讨论的多节点和角色级功能保持分离。
范围
本 PR 仅覆盖单节点、非分离式固定序列推理，初始验证目标为 H100/H200 8k/1k。
不包括：GB200/GB300 多节点或分离式遥测、预填充/解码角色归因、srt-slurm 修改、MI355X 硬件验证、4P1D/8P1D 机架级实验、用于发布的重复实验、绘图或文章工作。
验证
python -m pytest utils/test_aggregate_power.py utils/test_process_result.py -q— 80 项通过