fix: integrate GPU power when energy counter is absent - #1319
davidberenstein1957 wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1319 +/- ##
==========================================
+ Coverage 91.70% 91.74% +0.04%
==========================================
Files 49 49
Lines 5157 5171 +14
==========================================
+ Hits 4729 4744 +15
+ Misses 428 427 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GPUs without nvmlDeviceGetTotalEnergyConsumption now integrate instantaneous power instead of reporting zero, re-baselining cleanly when the counter returns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4e36cc8 to
1889fda
Compare
Verdict: ✅ Approve with nitsThe bug is real. On master,
54 GPU tests pass on this PR merged with master. Nits / follow-ups:
|
AMD dual-GCD devices set _known_zero_energy_counter there, so the secondary GCD no longer takes the power fallback on its first interval. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Made the changes in 19613ce: merged master, set static details before the first energy read (AMD dual-GCD flag), and updated the description to list all 4 tests. |
Description
GPUDevice.deltanow branches on the backend returningNonefor the cumulative energy counter. When it does, it computesPower.from_watts(self._get_power_usage())andEnergy.from_power_and_time(...)instead of comparing two identical counter readings, and logs a single warning per device so users know which method produced their numbers. The per-samplelogger.warninginNvidiaGPUDevice._get_total_energy_consumptionis demoted todebug, since the once-per-device warning now carries the signal.Related Issue
Fixes #1316
Motivation and Context
On GPUs where NVML does not implement
nvmlDeviceGetTotalEnergyConsumption(pre-Volta cards, many virtualised GPUs) the previous code returnedlast_energyunchanged, yieldingenergy_delta = 0andpower = 0for the entire run, so a GPU-bound job silently reported only CPU + RAM. The fallback lives in the sharedGPUDevice, so the AMD backend (which also returnsNonewhen no energy accumulator is present) is fixed by the same change. Power integration is less accurate than the counter, but it is the same approximation the CPU path already uses and is dramatically better than zero.How Has This Been Tested?
tests/test_gpu_nvidia.py::TestGpu::test_gpu_without_energy_counter_falls_back_to_powerpatches the NVML energy query to raise and asserts 26 W / 29 W and 0.026 / 0.029 kWh over an hour. It fails on master (assert 0.0 == 26) and passes here. Three more tests intests/test_gpu_nvidia.pycover the rest:test_gpu_energy_counter_recovery_does_not_double_count,test_gpu_without_power_usage_reports_zero_instead_of_raisingandtest_start_rebaselines_the_energy_counter.tests/test_gpu.py,tests/test_gpu_nvidia.pyandtests/test_gpu_amd.pyall pass (54 tests).Screenshots (if appropriate):
N/A
Types of changes
AI Usage Disclosure
Checklist: