feat(power): add query energy and TDP dashboard axes / 新增每请求能耗与 TDP 占比图表轴 - #735
feat(power): add query energy and TDP dashboard axes / 新增每请求能耗与 TDP 占比图表轴#735edwingao28 wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @edwingao28's task in 5m 19s —— View job Review — PR #735: power-derived axes
Summary🟢 Solid, well-tested change. The three new derived axes are wired correctly end-to-end and verified at runtime. Verified:
One non-blocking note (inline): the PR body says "%TDP has no Pareto direction", but the config does assign it a roofline and the test enforces it — so Optimal Only filters the %TDP axis to a frontier (confirmed at runtime). Behavior is self-consistent and defensible; just reconcile the description (or drop the roofline) so reviewers aren't misled. No blocking issues found. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bbd7f58. Configure here.
| "y_measuredPowerPercentTdp_labelZh": "实测平均功耗（TDP 占比）", | ||
| "y_measuredPowerPercentTdp_title": "Measured Average Power as Percent of TDP", | ||
| "y_measuredPowerPercentTdp_titleZh": "实测平均功耗占 TDP 百分比", | ||
| "y_measuredPowerPercentTdp_roofline": "lower_right", |
There was a problem hiding this comment.
TDP axis has Pareto direction
Medium Severity
y_measuredPowerPercentTdp declares lower_right / lower_left rooflines even though this axis is a utilization gauge, not an efficiency frontier. That turns on Pareto / Optimal Only / best-per-SKU logic that treats lower % TDP as better, which misranks configs for this metric.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit bbd7f58. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 308fbdd3e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'y_measuredJPerSuccessfulQuery', | ||
| 'y_measuredWhPerSuccessfulQuery', | ||
| 'y_measuredPowerPercentTdp', |
There was a problem hiding this comment.
Registering these metrics makes computeAllRooflines calculate their frontiers, but markRooflinePoints has no reset or assignment branches for measuredJPerSuccessfulQuery, measuredWhPerSuccessfulQuery, or measuredPowerPercentTdp. Consequently, transformBenchmarkRows always returns their metric-specific roof flags as false, breaking the chart-ready transform contract for any consumer that uses those flags. Add all three to the reset and onCurrentRoofline assignment logic.
AGENTS.md reference: AGENTS.md:L249-L254
Useful? React with 👍 / 👎.
| 'y_measuredJPerSuccessfulQuery', | ||
| 'y_measuredWhPerSuccessfulQuery', | ||
| 'y_measuredPowerPercentTdp', |
There was a problem hiding this comment.
These lines expose three new dashboard selections, but the commit adds only a Cypress component test and unit tests; no cypress/e2e spec exercises selecting an axis against actual chart data. The repository's UI-testing requirement calls for an E2E test for new UI features, and without one the selector-to-chart flow—including official and unofficial data, rendering, and Optimal Only behavior—remains unverified.
AGENTS.md reference: AGENTS.md:L132-L139
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "y_measuredPowerPercentTdp_labelZh": "实测平均功耗（TDP 占比）", | ||
| "y_measuredPowerPercentTdp_title": "Measured Average Power as Percent of TDP", | ||
| "y_measuredPowerPercentTdp_titleZh": "实测平均功耗占 TDP 百分比", | ||
| "y_measuredPowerPercentTdp_roofline": "lower_right", |
There was a problem hiding this comment.
🟡 WARNING (behavior vs. description mismatch, non-blocking): The PR description states "%TDP has no Pareto direction — it is a utilization gauge, not an efficiency frontier." But this config assigns y_measuredPowerPercentTdp_roofline (lower_right here, lower_left on the e2e block at line 369), and measured-power-direction.test.ts explicitly asserts it. So computeAllRooflines does build a Pareto frontier for %TDP, and "Optimal Only" filters to it.
I confirmed this at runtime: with the %TDP axis selected and Optimal Only on, the chart shows only the single frontier config (B200), not every deployment.
Why it matters: Reviewers trusting the PR body would expect all configs to remain visible under Optimal Only on this axis. The behavior is internally consistent (config + test + code agree) and defensible — %TDP is monotonic in avg_power_w per hardware, so it mirrors the existing measuredAvgPower frontier treatment. But the description is stale/contradictory.
Fix: Either drop the _roofline keys for y_measuredPowerPercentTdp (and relax the test) if a frontier is genuinely unwanted for a "gauge" axis, or update the PR description to match the shipped behavior. No code change needed if the frontier is intended.
|
Claude finished @edwingao28's task in 3m 58s —— View job Re-review — PR #735: power-derived axes
SummaryThe three new axes are wired correctly end-to-end (config EN/ZH labels, One 🟡 WARNING (inline, also Codex's P1): Non-blocking (already noted by Bugbot / Codex / prior review): the PR body says "%TDP has no Pareto direction", but the config assigns it No blocking issues found. |
| 'y_measuredJPerSuccessfulQuery', | ||
| 'y_measuredWhPerSuccessfulQuery', | ||
| 'y_measuredPowerPercentTdp', |
There was a problem hiding this comment.
🟡 WARNING: These three keys are registered in Y_AXIS_METRICS, so computeAllRooflines builds Pareto frontiers for the two carrying a _roofline direction — but markRooflinePoints (below, ~L824–907) has no reset or assignment branch for measuredJPerSuccessfulQuery, measuredWhPerSuccessfulQuery, or measuredPowerPercentTdp.
Why it matters: transformBenchmarkRows therefore returns their per-point roof flag as permanently false, unlike the six existing measured axes. Live rendering is unaffected — both ScatterGraph.tsx:799 and GPUGraph.tsx:255 recompute the frontier from the chart-definition direction rather than reading this flag, so Optimal Only still works at runtime — but this violates the "Add/modify a metric" checklist in AGENTS.md (step 1: register in markRooflinePoints) and leaves the transform's roof contract silently inconsistent; any future consumer that trusts point.<metric>.roof will mis-treat these three as never-optimal. Matches Codex's P1 note.
Fix: In markRooflinePoints, add the three to the reset block (~L824–829) and to the assignment chain after the y_measuredJPerInputToken branch (~L905), e.g.:
} else if (chartDefYKey === 'y_measuredJPerSuccessfulQuery' && newPoint.measuredJPerSuccessfulQuery) {
newPoint.measuredJPerSuccessfulQuery.roof = onCurrentRoofline;
} else if (chartDefYKey === 'y_measuredWhPerSuccessfulQuery' && newPoint.measuredWhPerSuccessfulQuery) {
newPoint.measuredWhPerSuccessfulQuery.roof = onCurrentRoofline;
} else if (chartDefYKey === 'y_measuredPowerPercentTdp' && newPoint.measuredPowerPercentTdp) {
newPoint.measuredPowerPercentTdp.roof = onCurrentRoofline;
}(and the matching if (newPoint.measuredJPerSuccessfulQuery) newPoint.measuredJPerSuccessfulQuery.roof = false; lines in the reset block). If a frontier for %TDP/Wh is genuinely unwanted, instead drop their _roofline keys from the config so the intent is unambiguous.
1 participant
What
Three derived measured-power axes for the inference dashboard:
y_measuredJPerSuccessfulQuery— joules per successful query (producer emitsjoules_per_successful_query)y_measuredWhPerSuccessfulQuery— the same energy in Wh (÷ 3600) for people who think in battery unitsy_measuredPowerPercentTdp—avg_power_w / TDP × 100, TDP from the shared hardware registry inpackages/constants/src/gpu-keys.ts(all 10 SKUs carry atdpentry)All three: EN/ZH labels, roofline directions (
lower_righton the interactivity block,lower_lefton the e2e block; %TDP has no Pareto direction — it is a utilization gauge, not an efficiency frontier), scatter + historical trends + unofficial-run overlay wiring, and gap-preserving behavior when the underlying fields are absent.Depends on
Stacked on #734 (power_valid / schema-semantics guard) — the first two commits here are that PR. Review only the last commit:
feat(power): add query energy and TDP dashboard axes. Merge #734 first, then this rebases to a single commit.Like the base PR, the J/query axes only render whole-deployment-stamped rows on disaggregated deployments; %TDP and role watts are unaffected.
Out of scope
The AI-generated chart path (
ai-chart/) shares theY_AXIS_METRICSwhitelist and has pre-existing defects for every lower-is-better metric (missing-as-zero in line charts, no radar inversion, top-N picks maxima). Those predate this PR — they date from the original six measured axes — and are tracked separately rather than mixed in here. This PR only adds avalidateSpectest pinning that the new keys survive spec validation.Verification
Note
Medium Risk
Touches benchmark ingest and transform gating for measured power semantics; mis-gating could hide or mislabel energy on disagg or legacy rows, though coverage is extensive in tests.
Overview
Adds three derived measured-power Y-axis options (joules and Wh per successful query, plus average power as % of TDP) across scatter charts, historical trends, unofficial-run overlays, and the Measured Energy metric picker, with EN/ZH labels and lower-is-better Pareto directions on both interactivity and e2e chart blocks.
Data plumbing:
buildMeasuredPowerChartFieldscentralizes measured telemetry on chart points (Wh/query from J÷3600; % TDP fromavg_power_wand registry TDP via newtdponGpuSpecs).rowToAggDataEntrygates unprefixed joules onpower_validand exact schema version 2 for disagg whole-deployment semantics; ETL and overlay normalization fail closed on malformedpower_valid/power_metric_schema_version. Metric keys and types extend forjoules_per_successful_queryand role-specific joules fields.Reviewed by Cursor Bugbot for commit bbd7f58. Bugbot is set up for automated code reviews on this repo. Configure here.