Skip to content

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

Open
davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/gpu-without-energy-counter
Open

davidberenstein1957 wants to merge 3 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 91.74%. Comparing base (e5e46ab) to head (19613ce).

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.
📢 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
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>
@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 and others added 2 commits September 23, 2026 16:40
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>
@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.

This branch has not been deployed

No deployments
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

2 participants