Skip to content

[Power] feat: stamp whole-deployment semantics with a schema version / 为功耗指标加盖整机语义版本号 #2599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
edwingao28 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Power] feat: stamp whole-deployment semantics with a schema version / 为功耗指标加盖整机语义版本号 #2599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/AGENT_OPERATIONS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ Single-node fixed-sequence results may include `power_valid`, `avg_power_w`, `av

Multinode disaggregated results add `prefill_gpu_energy_j`, `decode_gpu_energy_j`, `prefill_avg_power_w`, `decode_avg_power_w`, `prefill_joules_per_input_token`, and `decode_joules_per_output_token`. Role energy covers the full formal benchmark window, not kernel-level phases, and the role watts are that energy divided by the same window and by the role's GPU count.

Every power result — valid or invalid, single-node or multinode — carries `power_metric_schema_version`. Version 2 defines each unprefixed `joules_per_*` field as whole-deployment GPU-board energy over the named denominator; role-scoped energy uses the explicit `prefill_*` / `decode_*` keys. Rows without the field predate the whole-deployment switch and their unprefixed joules are not comparable across topologies.

For srt-slurm recipes, `telemetry: {provider: dcgm-power}` enables official energy collection. `runners/launch_gb200-nv.sh` and `runners/launch_gb300-nv.sh` are the source of truth for `POWER_SRT_SLURM_PIN`. CI derives `POWER_PRODUCER_SHA` from the launcher stamp. `utils/test_gb200_power_official_contract.py` and `utils/test_gb300_power_official_contract.py` enforce the recipe/launcher contract. Only `PRECISION=fp8` dcgm-power lanes are validated.

Power audit artifacts are named `power_audit_<result>` and contain `power_validation_<result>.json` for single-node runs or `power_validation_<result>_*.json` for multinode runs. They are uploaded even when validation fails.
Expand Down
7 changes: 7 additions & 0 deletions utils/aggregate_power.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@
"joules_per_total_token",
}

# The unprefixed joules_per_* fields silently switched from role-local to
# whole-deployment energy when multinode aggregation landed, and the values
# alone cannot distinguish the two. Stamp the semantics so consumers fail
# closed on unversioned rows instead of guessing.
POWER_METRIC_SCHEMA_VERSION = 2


@dataclass(frozen=True)
class PowerIntegration:
Expand Down Expand Up @@ -756,6 +762,7 @@ def _patch_power_result(
data.pop(key, None)
# Keep the canonical aggregate numeric-only for InferenceX-app's metric
# auto-capture. Detailed reason codes live in the validation sidecar.
data["power_metric_schema_version"] = POWER_METRIC_SCHEMA_VERSION
data["power_valid"] = int(power_valid)
data.pop("power_invalid_reasons", None)
if power_valid:
Comment on lines 762 to 768

@claude claude Bot Aug 14, 2026

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.

🟡 Three modules independently reset the power fields on the aggregate JSON (pop stale keys, stamp power_metric_schema_version, set power_valid, pop power_invalid_reasons) — aggregate_power.py::_patch_power_result (762-767), aggregate_power_multinode.py::_patch_agg (~1167-1171), and process_result.py::record_power_internal_error (~77-83). This PR adds the same new schema-version line to all three copies instead of factoring the shared stamp logic into one helper in aggregate_power.py (which the other two already import from). Not a correctness issue, but the next contract change (e.g. bumping to version 3) has to be made correctly in three places with three different key-set variables.

Extended reasoning...

What: aggregate_power.py's _patch_power_result, aggregate_power_multinode.py's _patch_agg, and process_result.py's record_power_internal_error each contain an almost-identical four-step block: pop a set of stale power metric keys, set data['power_metric_schema_version'] = POWER_METRIC_SCHEMA_VERSION, set power_valid (as int(power_valid), int(audit.power_valid), and the literal 0 respectively), and pop power_invalid_reasons. This PR's diff makes the triplication visible in the most direct way possible: it adds the exact same new line — data["power_metric_schema_version"] = POWER_METRIC_SCHEMA_VERSION — to all three functions, each as a separate hunk in three different files.\n\nWhy it happens: the only real variation between the three sites is the set of keys being popped (_POWER_METRIC_KEYS vs _ALL_POWER_METRIC_KEYS vs _POWER_METRIC_KEYS + _MULTINODE_ROLE_METRIC_KEYS) and where power_valid comes from. process_result.py's copy exists for a legitimate reason — it's the fallback path used when aggregate_power/aggregate_power_multinode itself failed to import, so it deliberately keeps its own literal copy of the multinode role-metric keys rather than importing them. But that constraint only explains the key-set duplication, not the stamp/valid/pop sequence itself, which could still be centralized.\n\nWhy nothing catches this today: there's no test or lint rule enforcing that the three envelope-patching functions stay in sync; they're only kept consistent by whoever remembers to touch all three when the contract changes. This PR is itself proof of the pattern working correctly this time (all three got the new line), but that's diligence, not a structural guarantee.\n\nImpact: none today — the PR is correct as written and all 126 tests pass. The cost is future maintenance risk: the next schema-affecting change (e.g. bumping to version 3, adding a new required top-level field, or changing how power_invalid_reasons is handled) requires editing three call sites with three different key-set variables and remembering that process_result.py's copy is a fallback that must independently track aggregate_power_multinode.ROLE_METRIC_KEYS as a literal. Missing one site silently leaves stale keys or an unstamped schema version on some code path, which is exactly the kind of drift this PR itself was written to close (the unprefixed joules_per_* semantics silently changed once already without any version marker).\n\nSuggested fix: extract a small helper in aggregate_power.py, e.g. _stamp_power_envelope(data, *, metric_keys, power_valid) that performs the pop-loop, schema-version stamp, power_valid assignment, and power_invalid_reasons pop in one place. aggregate_power_multinode.py and process_result.py already import constants from aggregate_power.py, so they could import this helper too — process_result.py's fallback path would still pass its own literal _POWER_METRIC_KEYS + _MULTINODE_ROLE_METRIC_KEYS tuple as the metric_keys argument, preserving its independence from the multinode module's import.\n\nProof of the triplication — grep the three sites: aggregate_power.py:762-767 has for key in _POWER_METRIC_KEYS: data.pop(key, None) then the stamp/valid/pop trio; aggregate_power_multinode.py:_patch_agg (~1167-1171) has for key in _ALL_POWER_METRIC_KEYS: data.pop(key, None) then the identical stamp/valid/pop trio; process_result.py:record_power_internal_error (~77-83) has two pop loops (_POWER_METRIC_KEYS then _MULTINODE_ROLE_METRIC_KEYS) then the same trio again. Diff each function's tail four lines side by side and they are structurally identical modulo the power_valid source expression.

Comment on lines 762 to 768

@claude claude Bot Aug 14, 2026

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.

🟡 In _patch_power_result, non-finite metrics (e.g. an absurd-but-finite power sample like 9e307 W overflowing the trapezoidal integration to inf) are only detected inside the per-key write loop, which raises ValueError before _write_json_atomic runs. Since run()'s except handler doesn't retry the write, the on-disk agg keeps whatever process_result.py originally wrote -- no power_metric_schema_version and no power_valid -- breaking this PR's own contract that every power result carries the schema version. The multinode sibling (aggregate_power_multinode.py) already guards this exact overflow case up front so _patch_agg still stamps power_valid=0 plus the schema version; aggregate_power.py should get the analogous guard.

Extended reasoning...

What happens: In utils/aggregate_power.py, run() calls _derived_metrics() unguarded and passes the result straight to _patch_power_result(). That function only checks for non-finite values inside its per-key write loop (if value is None or not math.isfinite(value): raise ValueError(...)), and this check happens after it has already mutated the in-memory data dict (stamping power_metric_schema_version and power_valid) but before _write_json_atomic(agg_path, data) is called. Since the raise happens before the write, the on-disk agg file is left completely untouched by this invocation.

Trigger path: integrate_power() only rejects a power sample if it's non-finite or negative (not math.isfinite(power) or power < 0). An absurd-but-finite value like 9e307 W passes that check. When _integrate_device() sums such values across the trapezoidal integration, the result overflows to inf. power_valid stays True (no reasons were ever appended), so _derived_metrics() runs and returns a metrics dict containing inf/nan values (it only guards against None, not non-finite floats). _patch_power_result() then raises ValueError on the first such key, mid-loop, before the atomic write.

Why the except handler doesn't save it: run() wraps the _patch_power_result() call in except (OSError, json.JSONDecodeError, ValueError), which does catch the raised error -- so there's no crash and the run still exits 0 (or 1 under REQUIRE_POWER). But the handler only resets the local power_valid/metrics variables that feed the sidecar/validation JSON; it never re-invokes _patch_power_result or otherwise re-writes the agg JSON. The agg file on disk ends up exactly what process_result.py wrote before power aggregation ran -- meaning it has neither power_metric_schema_version nor power_valid.

Why this matters for this PR specifically: the PR's entire premise (per AGENT_OPERATIONS.md) is that every power result, valid or invalid, carries power_metric_schema_version, and that rows missing the field predate the whole-deployment semantics switch. This bug produces a post-PR row that is missing the field for an unrelated reason (an aggregation-time crash), which a downstream consumer would misinterpret as "this row predates the schema" rather than "this row's power integration overflowed."

The sibling module already fixed this exact case: aggregate_power_multinode.py's validate_and_integrate() computes metrics, then explicitly checks non_finite = sorted(key for key, value in metrics.items() if not math.isfinite(value)) before calling _patch_agg, and if any are found it adds reason non_finite_power_metric and returns an invalid audit -- so _patch_agg still runs and stamps power_valid=0 plus the schema version. There's even a dedicated test, test_overflowing_power_metric_is_invalid_not_stale, proving the maintainers consider this scenario test-worthy. aggregate_power.py never received the analogous up-front guard.

Proof walkthrough:

  1. Telemetry CSV contains a power reading of 9e307 W for a GPU device across the benchmark window.
  2. integrate_power(): the per-sample check not math.isfinite(power) or power < 0 is False (9e307 is finite and positive) so the sample is accepted with no reasons appended.
  3. _integrate_device(): the trapezoidal sum (right_time - left_time) * (left_power + right_power) / 2.0 overflows float64 range and returns inf.
  4. Back in integrate_power(), per_gpu_energy_j contains inf; total_gpu_energy_j = sum(per_gpu_energy_j.values()) is inf; power_valid = not reasons is still True.
  5. run() calls derived_metrics(integration, benchmark) since power_valid and benchmark is not None, returning a dict where total_gpu_energy_j, joules_per*, etc. are all inf.
  6. _patch_power_result(agg_result, power_valid=True, metrics={...inf...}) is invoked: it pops old keys, sets data['power_metric_schema_version'] = 2 and data['power_valid'] = 1 in memory only, then in the per-key loop hits the first inf value and raises ValueError('non-finite power metric: ...') before ever reaching _write_json_atomic.
  7. run()'s except (OSError, json.JSONDecodeError, ValueError) catches it, appends aggregate_result_unwritable to the local reasons list, and resets local power_valid = False, metrics = {} -- but these locals only feed the validation sidecar, not the agg file.
  8. Final state: agg_.json on disk is exactly what process_result.py originally wrote -- no power_metric_schema_version, no power_valid key at all -- even though the sidecar (power_validation_.json) correctly records power_valid: false and the reason.

Suggested fix: mirror the multinode guard -- compute the non-finite metric keys up front in run() (or inside _derived_metrics / before calling _patch_power_result) and fall back to power_valid=False, metrics={} so _patch_power_result is always called with values that can be written, guaranteeing the schema version and power_valid=0 land on disk even in this overflow case.

Expand Down
2 changes: 2 additions & 0 deletions utils/aggregate_power_multinode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from pathlib import Path, PurePosixPath

from aggregate_power import (
POWER_METRIC_SCHEMA_VERSION,
BenchmarkData,
_append_reason,
_integrate_device,
Expand Down Expand Up @@ -1165,6 +1166,7 @@ def _patch_agg(agg_path: Path, audit: MultinodePowerAudit) -> None:
data = json.loads(agg_path.read_text(encoding="utf-8"))
for key in _ALL_POWER_METRIC_KEYS:
data.pop(key, None)
data["power_metric_schema_version"] = POWER_METRIC_SCHEMA_VERSION
data["power_valid"] = int(audit.power_valid)
data.pop("power_invalid_reasons", None)
if audit.power_valid:
Expand Down
2 changes: 2 additions & 0 deletions utils/process_result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ def record_power_internal_error(
try:
from aggregate_power import (
_POWER_METRIC_KEYS,
POWER_METRIC_SCHEMA_VERSION,
_empty_integration,
_validation_payload,
_write_json_atomic,
Expand All @@ -77,6 +78,7 @@ def record_power_internal_error(
agg_data.pop(key, None)
for key in _MULTINODE_ROLE_METRIC_KEYS:
agg_data.pop(key, None)
agg_data["power_metric_schema_version"] = POWER_METRIC_SCHEMA_VERSION
agg_data["power_valid"] = 0
agg_data.pop("power_invalid_reasons", None)
_write_json_atomic(agg_result, agg_data)
Expand Down
3 changes: 3 additions & 0 deletions utils/test_aggregate_power.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,7 @@ def test_run_skips_when_bench_window_missing(tmp_path: Path):
assert "avg_power_w" not in patched
assert patched == {
"hw": "h200",
"power_metric_schema_version": 2,
"power_valid": 0,
}

Expand Down Expand Up @@ -755,6 +756,7 @@ def test_run_emits_complete_whole_deployment_metric_contract(tmp_path: Path):

assert exit_code == 0
patched = json.loads(agg.read_text())
assert patched["power_metric_schema_version"] == 2
assert type(patched["power_valid"]) is int
assert patched["power_valid"] == 1
assert "power_invalid_reasons" not in patched
Expand Down Expand Up @@ -809,6 +811,7 @@ def test_run_best_effort_marks_invalid_power_and_preserves_benchmark(tmp_path: P
patched = json.loads(agg.read_text())
assert patched["hw"] == "h200"
assert patched["conc"] == 4
assert patched["power_metric_schema_version"] == 2
assert type(patched["power_valid"]) is int
assert patched["power_valid"] == 0
assert "power_invalid_reasons" not in patched
Expand Down
2 changes: 2 additions & 0 deletions utils/test_aggregate_power_multinode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ def assert_invalid(pkg, expected_reason, **run_kwargs):
"""Both modes must reject: metrics withheld always, exit code differs."""
assert pkg.run(require_power=False, **run_kwargs) == 0
agg = pkg.agg()
assert agg["power_metric_schema_version"] == 2
assert agg["power_valid"] == 0
for key in apm.WHOLE_METRIC_KEYS + apm.ROLE_METRIC_KEYS:
assert key not in agg
Expand All @@ -229,6 +230,7 @@ def test_emits_all_metrics_exactly(self, tmp_path):
assert pkg.run() == 0

agg = pkg.agg()
assert agg["power_metric_schema_version"] == 2
assert agg["power_valid"] == 1
assert agg["avg_power_w"] == 350.0
assert agg["avg_total_gpu_power_w"] == 1400.0
Expand Down
4 changes: 4 additions & 0 deletions utils/test_process_result.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,7 @@ def test_require_power_accepts_valid_single_node_measurement(

assert result.returncode == 0, result.stderr
agg = json.loads((tmp_path / "agg_benchmark_result.json").read_text())
assert agg["power_metric_schema_version"] == 2
assert agg["power_valid"] == 1
assert agg["total_gpu_energy_j"] == pytest.approx(40_000.0)
validation = json.loads(
Expand Down Expand Up @@ -914,6 +915,7 @@ def test_internal_aggregation_error_is_always_auditable(

assert result.returncode == expected_returncode
agg = json.loads((tmp_path / "agg_benchmark_result.json").read_text())
assert agg["power_metric_schema_version"] == 2
assert agg["power_valid"] == 0
assert "power_invalid_reasons" not in agg
validation = json.loads(
Expand Down Expand Up @@ -1221,6 +1223,7 @@ def test_valid_package_patches_role_energy(self, tmp_path, power_env):

assert result.returncode == 0, f"Script failed: {result.stderr}"
agg = json.loads((tmp_path / "agg_benchmark_result.json").read_text())
assert agg["power_metric_schema_version"] == 2
assert agg["power_valid"] == 1
assert agg["prefill_gpu_energy_j"] == 48000.0
assert agg["decode_gpu_energy_j"] == 36000.0
Expand All @@ -1233,6 +1236,7 @@ def test_missing_package_is_best_effort(self, tmp_path, power_env):

assert result.returncode == 0, f"Script failed: {result.stderr}"
agg = json.loads((tmp_path / "agg_benchmark_result.json").read_text())
assert agg["power_metric_schema_version"] == 2
assert agg["power_valid"] == 0
for key in WHOLE_METRIC_KEYS + ROLE_METRIC_KEYS:
assert key not in agg
Expand Down