Repository navigation
feat(power): report each round's power-throttle share and sustained clock - #2038
Conversation
…lock Make visible, per round, how the cards ran under their power cap: - The measured-phase sampler adds --clock to its amd-smi query (no measurable cost) and records each card's mean sclk while busy (deep-sleep readings below the XCD's min_clk are idle time and left out) and its clock limit. - It reads the amd-smi --violation accumulators as each measured window opens and closes (summed over windows, so a GEAK replay's boots between replicas do not count) and reports power_throttle_fraction: PPT-limited ticks over all ticks in the measured phase. - gpu_power.json lists serving cards throttled for at least 10% of the phase in power_throttled_gpus, and the round logs a warning naming each card's throttled share and mean clock. Nothing changes the cards. - A measured window whose counters could not be read leaves the share unread instead of counting only the windows that were: every card's when a --violation read failed, one card's when it was missing from either end or its counters went back or stopped advancing. A failed read is counted in throttle_query_errors / throttle_last_error, so query_errors / last_error keep meaning the power reads failed. - sclk_max_mhz is read from the XCD slots amd-smi fills, and is unread when they report different limits in any sample. The new fields are additive; existing readers of gpu_power.json are unaffected. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/review |
|
Automated review started for head 196f385. Progress: https://github.com/AMD-AGI/Hyperloom/actions/runs/39341905732 |
Code metrics gate: PASSEDJudged: the 3 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
100 findings in files this change does not touchOutside this change (informational): 100
...and 75 more; the job log lists every row.
Thresholds and sources: |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
PR #2038 -- feat(power): report each round's power-throttle share and sustained clockWhat it does: A round run under a low GPU power cap was indistinguishable from a clock-bound one in every artifact. The measured-phase GPU power sampler (src/hyperloom/orchestrator/actions/executors/_gpu_power.py) now also reads each card's graphics clock with the same amd-smi query and the PPT violation accumulators as each measured window opens and closes, so gpu_power.json gains per-card sclk_mean_mhz, sclk_max_mhz and power_throttle_fraction plus a top-level power_throttled_gpus, and a round with a serving card power throttled for at least 10% of its measured phase logs a warning naming the cards, their throttled share and mean clock. Windows whose counters could not be read leave the share null rather than summing only the windows that were; nothing changes the cards and no gate or decision reads the new fields. Blocking issues: none SKIPPED: V4 -- "# PARTIAL: #1276 changes 1311 files, only its first 100 were compared" -- overlap with those files unchecked Checked: _gpu_power.py at head and base (parse_clock_sample/parse_throttle_sample, _read_throttle/_close_window/_complete_throttle_totals, summary/close, _power_throttled, _POWER_THROTTLE_REPORT_FRACTION); test_gpu_power_sampling.py (all 43 tests, _Replay/_with_counters/_run_measured helpers); docs/conceptual/optimization-loop.md against the code; tree-wide greps for the new fields, _query_amd_smi, GpuPowerRecorder, parse_clock_sample, parse_throttle_sample and gpu_power across src and docs; production wiring in _subprocess_kill.py:_RecorderFanout and _geak_sweep.py; read_measured_gpu_power_by_gpu consumers in test_power_budget.py | Ran: pytest src/hyperloom/inference_optimizer/tests/test_gpu_power_sampling.py (43 passed) and src/hyperloom/inference_optimizer/tests/test_power_budget.py (71 passed) at head; ruff C901 (max 20) and PLR0913 (max 5) on head and base copies of _gpu_power.py (both clean); direct GpuPowerRecorder reproduction of a throttled card with unread clock | Base: b7004d1 | Head: 196f385 LGTM |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/review |
|
Automated review started for head 2825429. Progress: https://github.com/AMD-AGI/Hyperloom/actions/runs/39345196012 |
|
Automated review: no verdict applied -- the review job ended with result 'failure'. Comment |
|
/review |
|
Automated review started for head 2825429. Progress: https://github.com/AMD-AGI/Hyperloom/actions/runs/39346757649 |
|
Automated review: no verdict applied -- the review job ended with result 'failure'. Comment |
|
/review |
|
Automated review started for head 2825429. Progress: https://github.com/AMD-AGI/Hyperloom/actions/runs/39351438384 |
|
Automated review: no verdict applied -- the review job ended with result 'failure'. Comment |
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2038 -- feat(power): report each round's power-throttle share and sustained clock
What it does: the measured-phase GPU power sampler adds --clock to its existing amd-smi metric query and reads the amd-smi metric --violation accumulators as each measured window opens and closes, so gpu_power.json gains per-card sclk_mean_mhz, sclk_max_mhz and power_throttle_fraction and a top-level power_throttled_gpus, and a round with a card power throttled for at least 10% of its measured phase logs a warning. The arithmetic matches amd-smi's own PVIOL% (checked against the amdsmi rocm-7.2.0 source: ppt_accumulated and accumulation_counter come from one gpu_metrics snapshot, so the delta ratio cannot exceed 1, and a reset or stalled counter is caught), the deep-sleep cut uses the same min_clk rule amd-smi uses, the ~1 s --violation read runs on the sampler thread and never on the event loop or inside note_phase, and the measured-phase and client boundaries are untouched.
Blocking issues: 1
- [D] A card whose counters are missing at both ends of a window keeps a share computed from the other windows only [verified]
Problem:_close_windowmarks a card unread only foropened.keys() ^ closed.keys(), a card present at one end and missing at the other. A card missing at both ends of a window is not in the symmetric difference, gets no delta for that window and is not marked unread, and_complete_throttle_totals(:314-321) publishes its share from the remaining windows. The read itself succeeded, so_throttle_read_faileddoes not fire either. The code's invariant comment (:222-223) and optimization-loop.md both promise "unread" here. src/hyperloom/orchestrator/actions/executors/_gpu_power.py:282
Impact: amd-smi's CLI prints a card's wholethrottleblock as "N/A" with exit 0 whenamdsmi_get_violation_statusraises for that card, andparse_throttle_samplethen drops the card. Reproduced throughGpuPowerRecorder: window 1 has 900 of 1,000 ticks PPT-limited on both cards; in window 2 card 4 reads N/A at both ends while card 5 shows 100 of 98,000. Card 4 is published aspower_throttle_fraction0.9 and listed inpower_throttled_gpuswiththrottle_query_errors0. Native rounds already split the phase into a short warmup window and the benchmark window, so a double miss on the benchmark window publishes the idle window's share as the whole phase.
Action: tie validity to the windows read instead of adding another check: count closed windows, accumulate per card[ticks, ppt, windows_read]only when the card had good counters at both ends, and publish a card only when itswindows_readequals the window count. A failed read, a card missing at one or both ends, and a counter going back then all mean the same thing, which also removes_throttle_unreadand_throttle_read_failed. Add the both-ends test.
Non-blocking:
- If
close()'s join times out while the thread is still in a window-open--violationread (amd-smi slow twice in a row), the last window is dropped without the card being marked unread; the window count above does not help there because the window never closes.close()also now runs the closing read on the watchdog thread (about 1 s per round end, up to the 10 s query timeout), while the class docstring says the watchdog must not stall on amd-smi. - amd-smi copies one
min_clk/max_clkper clock type into every filledgfx_<n>slot, so XCDs cannot disagree within one sample; the case modelled inparse_clock_sampleand its test cannot occur, and a nullsclk_max_mhzreally means the limit changed between samples. - The warning prints "at a mean clock of None MHz" when a throttled card had no awake-XCD sample.
- Clock parsing runs inside the power sample's
try; a parse failure would drop the power reading the power budget gates on. Parsing clocks after the power reading is recorded would keep them apart.
Checked: _gpu_power.py at head and base (parse_clock_sample, parse_throttle_sample, _read_throttle, _close_window, _complete_throttle_totals, summary, close, the sampler loop and thread); amdsmi rocm-7.2.0 CLI and library source for --clock and --violation fields, N/A handling and PVIOL%; ServerLogPhaseDriver and note_client_phases boundaries; readers of gpu_power.json; optimization-loop.md; the description | Ran: pytest test_gpu_power_sampling.py test_power_budget.py with a stub fcntl (all 43 sampling tests pass; the only failures are power-budget CLI tests needing signal.SIGHUP on Windows); a GpuPowerRecorder probe with a card N/A at both ends of window 2 (share 0.9 published); a timing probe with a fake 1 s --violation and 0.3 s power read (note_phase 0 s, close() 1 s) | Base: b7004d1 | Head: 2825429
… window A card whose --violation counters were missing at both ends of a window was neither given that window's delta nor marked unread, so its share came from the other windows only (for example a short warmup window standing in for the whole phase). - Count the measured windows, and per card the windows it was read across with counters moving forward; publish a card only when the two match. A failed read, a card missing at one or both ends, and counters going back now all mean the same thing, replacing the unread set and the read-failed flag. - close() counts one more, unread window when the sampler is still blocked in amd-smi after the join timeout, and the class docstring says close() waits. - Clock parsing runs after the power reading is recorded. - The warning names an unread clock as unread, not "None MHz". - sclk_max_mhz is described as unread when the limit changed during the round: amd-smi copies one limit into every filled XCD slot. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks @chaojhou. Addressed in 6c833b7: Blocking: a card missing at both ends of a window. Done as you suggested. The recorder counts measured windows and, per card, the windows it was read across with counters moving forward. A card is published only when the two counts match. A failed read, a card missing at one or both ends, and counters going back now all leave the share unread, and Non-blocking:
The description and |
Co-authored-by: Cursor <cursoragent@cursor.com>
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2038 -- feat(power): report each round's power-throttle share and sustained clock
What it does: the measured-phase GPU power sampler adds --clock to its existing amd-smi metric query and reads the amd-smi metric --violation accumulators as each measured window opens and closes, so gpu_power.json gains per-card sclk_mean_mhz, sclk_max_mhz and power_throttle_fraction plus power_throttled_gpus, and a round with a card power throttled for at least 10% of its measured phase logs a warning. The previous blocker is fixed in 6c833b7: the recorder counts closed windows and keeps [ticks, ppt, windows_read] per card, and publishes a share only when the card was read across every window, so _throttle_unread / _throttle_read_failed are gone. My earlier probe (card 4 N/A at both ends of window 2) now leaves card 4 unread and unlisted while card 5 keeps its own share, pinned by test_a_card_unread_at_both_ends_of_a_window_is_left_unread. The non-blocking items are addressed too: a join timeout counts one more window so no card is published a window short, the clock-limit model describes a limit that changed during the round, the warning prints unread instead of None MHz, and clock parsing runs after the power reading, outside its try.
Blocking issues: none
Non-blocking:
- When the sampler outlives
close()'s join, it can make one more--violationread after the round is summarised (_window_openwas cleared and the stop flag is only checked at the top of the loop). It changes no published value and needs two amd-smi calls near the 10 s timeout in a row.
Checked: git diff 2825429d7 4f20a60d0; _gpu_power.py at head (_loop, _read_throttle, _close_window, _sample_once, _complete_throttle_totals, summary, close, parse_clock_sample); the description and optimization-loop.md unread rules against the code; the author's reply against the code | Ran: GpuPowerRecorder probes at head: the both-ends N/A case, single and zero windows, a failed read at window open and close, a card missing at one end, counters and PPT going back, stalled counters, a card present only in window 2, close() running the closing read after a finished thread and after the sampler closed the window (no double count), the warning with a sleeping and an awake clock; a real sampler thread blocked in the opening read, in a power sample and in the closing read with the query timeout patched to 0.2 s (every share unread, as intended); pytest test_gpu_power_sampling.py (46 passed); CI at head (33 pass, 7 skipped) | Base: b7004d1 | Head: 4f20a60
LGTM
Description: what and why
A round run under a low GPU power cap gives no sign that the cap, not the clock limit, set its throughput.
gpu_power.jsonrecords only watts and VRAM, so an operator comparing rounds at different caps cannot tell a power-bound round from a clock-bound one. This PR makes that visible per round, without changing the cards:--clockto its existing read-onlyamd-smi metricquery. Each card ingpu_power.jsongainssclk_mean_mhz, its mean graphics clock while busy (an XCD in deep sleep, below its ownmin_clk, is idle time and left out), andsclk_max_mhz, its clock limit, read from the XCD slots amd-smi fills (nullwhen none reported one, or when the limit changed during the phase).amd-smi metric --violationaccumulators as each measured window opens and closes, summed over windows so a GEAK replay's boots between replicas do not count. Each card gainspower_throttle_fraction: PPT-limited ticks over all ticks in the measured phase.gpu_power.jsongainspower_throttled_gpus: the serving cards throttled for at least 10% of the measured phase. For those the round logs a warning naming each card's share and mean clock.power_throttle_fractionunread (null) instead of counting only the windows that were read: every card's when a--violationread failed, one card's when it was missing from either end of a window or both, or its counters went back or stopped advancing, and every card's when the sampler was still blocked inamd-smias the round closed. A card is published only when its counters were read across every measured window. A failed read is counted in the newthrottle_query_errors/throttle_last_error, soquery_errors/last_errorkeep meaning the power reads failed.Measured on MI355X (amd-smi 26.2.2) under a synthetic compute load: capped at 700 W, the cards were power throttled for about 99.9% of the phase at 920 to 1020 MHz; at the default 1400 W cap the same load still reached the cap and was throttled for 14 to 52%. The extra
--clockfield costs nothing measurable per sample; the--violationread takes about a second and runs only at window boundaries, after the window's first power sample.Linked issue(s): none. Follow-up to the measured-phase power sampler from feat(power): --max-power-w total and --max-per-gpu-power-w budgets #1770.
Tests: added/updated? commands run?
src/hyperloom/inference_optimizer/tests/test_gpu_power_sampling.py: clock parsing (XCD averaging, the limit, deep-sleep exclusion, and amd-smi's eight printedgfx_<n>slots of which a partitioned card fills fewer), throttle-counter parsing, a throttled and an unthrottled round end to end throughGpuPowerRecorder(fields,power_throttled_gpus, the warning text), counters summed over measured windows only, the sampler closing a window at the phase change, a round with idle gaps, a failed--violationread leaving the power reading andquery_errorsalone, a failed read in either window (or at both ends of one) leaving every card's share unread, a card missing from one or both ends of a window or with counters going back left unread, a sampler still blocked inamd-smiat close leaving every share unread, a limit that changed during the round left unread, and an unread clock named as such in the warning.amd-smistand-ins (_Replay,test_native_power_uses_client_boundaries_and_waits_for_responses,test_a_geak_replay_is_sampled_over_its_measured_phase): the recorder'squeryseam now takes theamd-smi metricarguments, and a--violationread goes through the same seam.pytest src/hyperloom/inference_optimizer/tests/test_gpu_power_sampling.py src/hyperloom/inference_optimizer/tests/test_power_budget.py; ruff check and format; pylint errors-only;lint-imports --no-cache;scripts/code_metrics.py --base-ref origin/main;scripts/pr_hygiene.py size;scripts/check_cli_references.py.Size/complexity triggers crossed: none. 204 production Python lines changed (budget 1000); no function over the complexity or parameter limits.
If this simplifies or refactors: no. The per-card power fields move into
_power_fieldsbeside the new_clock_fieldswith the same keys and rounding.Observable effect:
gpu_power.jsongains per-cardsclk_mean_mhz,sclk_max_mhzandpower_throttle_fraction, and top-levelpower_throttled_gpus,throttle_query_errorsandthrottle_last_error; a round with a card power throttled for at least 10% of its measured phase logs a warning naming the cards, their throttled share and mean clock; the sampler runs one more read-onlyamd-smi metric --violationper measured window boundary. Before, a power-bound round looked the same as a clock-bound one in every artifact. Nothing changes the cards, and no gate or decision reads the new fields.Breaking changes: no. The fields are additive;
schema_versionstays 1 and existing readers ofgpu_power.jsonread only the fields they read before.PR addresses single concern: yes
Root cause is upstream: no