feat(power): aggregate measured GPU power into agg result JSON #1551
Claude / Claude Code Review
completed
May 22, 2026 in 12m 57s
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | utils/test_aggregate_power.py:1-20 |
New test_aggregate_power.py tests will not run in CI |
| 🟡 Nit | perf-changelog.yaml:3086-3091 |
perf-changelog pr-link points to superseded fork PR #1551 instead of this PR #1558 |
| 🟡 Nit | utils/aggregate_power.py:141-168 |
avg_power_w misreports total system power when CSV lacks a GPU index column |
Annotations
Check failure on line 20 in utils/test_aggregate_power.py
claude / Claude Code Review
New test_aggregate_power.py tests will not run in CI
The 26 new unit tests in `utils/test_aggregate_power.py` will not run in CI. The `.github/workflows/test-process-result.yml` workflow hardcodes `pytest test_process_result.py -v` (line 36), which never collects the new test file — even though this PR happens to trigger the workflow because `process_result.py` is in the paths filter. Additionally, the `paths` filter (lines 5-7) doesn't include `utils/aggregate_power.py` or `utils/test_aggregate_power.py`, so future PRs touching only those files w
Check warning on line 3091 in perf-changelog.yaml
claude / Claude Code Review
perf-changelog pr-link points to superseded fork PR #1551 instead of this PR #1558
The new `perf-changelog.yaml` entry's `pr-link` points to https://github.com/SemiAnalysisAI/InferenceX/pull/1551, but the PR description states this PR (#1558) supersedes #1551 and #1551 will be closed without merge. Update the link to `/pull/1558` so the changelog navigates to the PR with the actually-merged commit history.
Check warning on line 168 in utils/aggregate_power.py
claude / Claude Code Review
avg_power_w misreports total system power when CSV lacks a GPU index column
Latent robustness bug in `aggregate_power` (utils/aggregate_power.py:141-168): when `_detect_columns` can't find a GPU index column (strict regex `^(index|gpu|gpu_id|gpu_index|card|device)$`), every row collapses to `gpu_id='0'`, causing `avg_power_w` to report system-total power (N × W) instead of per-GPU mean. Today's nvidia-smi (`index`) and amd-smi (`gpu`) headers match the regex so the in-tree pipeline is safe, but any future amd-smi schema variant (e.g. `device_id`, `GPU ID`) would silentl
Loading