Skip to content

fix: integrate GPU power when energy counter is absent - #1319

Merged
benoit-cty merged 18 commits into
masterfrom
fix/gpu-without-energy-counter
Oct 17, 2026
Merged

benoit-cty merged 18 commits into
masterfrom
fix/gpu-without-energy-counter

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Description

GPUDevice.delta now branches on the backend returning None for the cumulative energy counter. When it does, it computes Power.from_watts(self._get_power_usage()) and Energy.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-sample logger.warning in NvidiaGPUDevice._get_total_energy_consumption is demoted to debug, 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 returned last_energy unchanged, yielding energy_delta = 0 and power = 0 for the entire run, so a GPU-bound job silently reported only CPU + RAM. The fallback lives in the shared GPUDevice, so the AMD backend (which also returns None when 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_power patches 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 in tests/test_gpu_nvidia.py cover the rest: test_gpu_energy_counter_recovery_does_not_double_count, test_gpu_without_power_usage_reports_zero_instead_of_raising and test_start_rebaselines_the_energy_counter. tests/test_gpu.py, tests/test_gpu_nvidia.py and tests/test_gpu_amd.py all pass (54 tests).

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

AI Usage Disclosure

  • 🟥 AI-vibecoded: You cannot explain the logic. Car analogy : the car drive by itself, you are outside it and just tell it where to go.
  • 🟠 AI-generated: Car analogy : the car drive by itself, you are inside and give instructions.
  • ⭐ AI-assisted. Car analogy : you drive the car, AI help you find your way.
  • ♻️ No AI used. Car analogy : you drive the car.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the docs/how-to/contributing.md document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov

codecov Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.36%. Comparing base (eb46a83) to head (34e9ae2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1319      +/-   ##
==========================================
+ Coverage   92.15%   92.36%   +0.20%     
==========================================
  Files          49       49              
  Lines        5227     5341     +114     
==========================================
+ Hits         4817     4933     +116     
+ Misses        410      408       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@davidberenstein1957
davidberenstein1957 marked this pull request as ready for review August 12, 2026 19:14
@davidberenstein1957
davidberenstein1957 requested a review from a team as a code owner August 12, 2026 19:14
@davidberenstein1957
davidberenstein1957 force-pushed the fix/gpu-without-energy-counter branch from 4e36cc8 to 1889fda Compare August 19, 2026 15:20
@benoit-cty

Copy link
Copy Markdown
Contributor

🤖 This review comment was written and posted by Claude Opus 5.5 (AI assistant), at the request of @benoit-cty. Findings were checked by reading the code and running tests locally (merged with current master where relevant), but please double-check before acting on them.

Verdict: ✅ Approve with nits

The bug is real. On master, _get_energy_kwh() returns last_energy when NVML/AMD exposes no energy counter, so every delta is 0. The fallback is correct:

  • _get_power_usage() already returns W on both backends (NVML mW/1000; AMD average_socket_power).
  • It works per device, so a mix of GPUs with and without counters is fine.
  • Recovering from None to a real counter re-baselines without double counting, and it's tested.
  • A failing power read now reports 0 W instead of raising.

54 GPU tests pass on this PR merged with master.

Nits / follow-ups:

  1. This is a rectangle rule over a single instantaneous sample taken at the end of the interval. Pre-Volta NVML power is instantaneous, so a bursty workload over the default 15 s interval can be noticeably off. A follow-up could average the samples the monitor already takes each second over the interval.
  2. On AMD dual-GCD devices, __post_init__ reads the counter before _known_zero_energy_counter is set, so the secondary GCD's first interval goes through the fallback. It's harmless because that GCD reports 0 W, but setting the flag first would be cleaner.
  3. The description mentions 1 test; the diff adds 4. Please update it.
  4. The branch is behind master; please update it.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

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.
Not done: averaging the monitor's per-second power samples over the interval, left as the suggested follow-up.

@github-actions github-actions Bot added size/L and removed size/M labels Oct 9, 2026
@github-actions github-actions Bot added size/XL and removed size/L labels Oct 17, 2026
davidberenstein1957 and others added 18 commits October 17, 2026 15:50
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>
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>
- Sample GPU power every second in the tracker's monitoring loop while the
  energy counter is unavailable, and average the samples over the interval
  instead of using a single reading taken at its end.
- Keep the last good counter reading during a counter outage and, when it
  comes back, report the counter delta minus what the fallback already
  reported, so the recovered interval uses the counter again.
- Treat a counter going backwards (driver reload, reset) as a fallback
  interval and re-baseline, instead of reporting negative energy.
- Use class-level defaults for the fallback flags so devices built without
  the dataclass __init__ do not raise AttributeError.
- Warn once per device when power cannot be read or reads 0 W without a
  counter; warn once per outage when a working counter fails, and log its
  recovery.
- Lower the per-sample AMD energy counter warnings to debug, like NVIDIA.
- Document the fallback in docs/explanation/power-estimation.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017Bj3CXQU2KcRKCxrb17j9e
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Instance state was declared as class attributes so that instances built
with __new__ in tests still worked, and _power_samples was never declared,
which required getattr guards on the hot path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
start() re-read the energy counter without updating _use_power_fallback.
A transient read failure at start() left the flag False, so no power was
sampled during the first interval, and delta() then logged the permanent
'no energy counter' warning for a device that has one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Clamping the recovered delta to 0 dropped any energy the power fallback
over-reported, so it stayed in the totals forever. The excess is now
deducted from the following counter deltas.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On counter recovery, the counter delta covers every fallback interval of
the outage, but the power was computed over the last interval only. After
a long outage this reported a power several times the real draw.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sample_power() runs in the 1s monitoring thread while delta() swaps the
sample list in the measurement thread. Without a lock a sample could be
appended to the discarded list and lost, or appended after the counter
came back and leak into a later fallback interval.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The second GCD of MI250/MI300 devices reports 0 W by design, its power is
counted by the first GCD. In fallback mode it logged a misleading
under-reporting warning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
self.devices is always an AllGPUDevices, which defines sample_power.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 1s monitoring loop read the power of every GPU, ignoring gpu_ids, so a
node tracking one GPU out of 8 made 8 power calls per second.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Counter read errors were moved to debug level, so at the default level
the fallback warning did not say why the counter could not be read
(permissions, driver error...). The backend error is now appended to it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The GPU power fallback relies on the 1s monitor to sample power, but
stop_task() stopped it for good after start(), and start() only started
it when output handlers existed. Later intervals fell back to a single
power reading. The monitor is now always started by start(), and resumed
with the measurement scheduler after a task.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FakeGPUEnv deleted codecarbon.external.hardware on teardown and left the
GPU modules imported against the fake pynvml. Test files running after
test_gpu_nvidia.py then re-imported hardware and got other GPU/CPU classes
than the ones codecarbon.emissions_tracker holds, so isinstance checks and
mock.patch targets missed: 5 tests in test_emissions_tracker.py failed
when run right after it. The modules are now snapshotted and restored.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@benoit-cty
benoit-cty force-pushed the fix/gpu-without-energy-counter branch from 33fce8d to 34e9ae2 Compare October 17, 2026 13:50
@benoit-cty
benoit-cty merged commit 89a82ec into master Oct 17, 2026
15 checks passed
@benoit-cty
benoit-cty deleted the fix/gpu-without-energy-counter branch October 17, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GPUs without an NVML energy counter report 0 kWh

3 participants