Skip to content

fix: skip uncorrectable RAPL counter wraps - #1321

Open
davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/rapl-wraparound-negative-energy
Open

davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/rapl-wraparound-negative-energy

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Description

RAPLFile.delta() now re-checks the wrap correction: if the corrected reading is still below the previous one, the sample is dropped (zero energy delta, zero power) with a warning, instead of emitting a negative energy delta. RAPLFile._get_value() now returns None instead of 0 when the counter can't be read, and delta() re-baselines on the next readable value. IntelRAPL.start() skips unreadable counters in the mirror detection.

Related Issue

Fixes #1309

Motivation and Context

max_energy_range_uj is set to 0 whenever it cannot be read (codecarbon/core/rapl.py:45). The comment claims wrap detection is then disabled, but only the correction was disabled; the wrap branch still ran and produced energy_delta = energy - last_energy, a large negative value (up to -1.19e-3 kWh for a 2^32 uJ package domain). That flowed unchecked into _total_energy / _total_cpu_energy, and the abs() in Power.from_energies_and_delay masked it in the power column. Dropping a sample loses at most one interval of genuine energy, which is strictly better than subtracting an hour's worth. This mirrors what codecarbon/core/windows_emi.py:566-574 already does for EMI. Systems where max_energy_range_uj is readable are unaffected.

Note from the author: with a readable max, a driver reset or suspend/resume still becomes energy + max - last, a large positive delta; a plausibility bound (e.g. TDP x interval) is left for a follow-up. This PR should land before the rapl_include_dram change, since DRAM domains have smaller max ranges and wrap far more often.

How Has This Been Tested?

tests/test_rapl_permissions.py:

  • test_rapl_wraparound_without_max_skips_sample fails on master (energy_delta.kWh is about -1.11e-3), passes here.
  • test_rapl_wraparound_with_max_is_corrected pins the existing wrap correction against regression.

uv run pytest tests/test_rapl_permissions.py -q -> 2 passed, 2 skipped (the pre-existing Linux-only cases). black --check clean; ruff reports only pre-existing findings in the touched files.

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.62%. Comparing base (3ec31a0) to head (f59ac22).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1321      +/-   ##
==========================================
+ Coverage   91.43%   91.62%   +0.18%     
==========================================
  Files          49       49              
  Lines        5057     5072      +15     
==========================================
+ Hits         4624     4647      +23     
+ Misses        433      425       -8     

☔ 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/rapl-wraparound-negative-energy branch 2 times, most recently from 4e88977 to d4a9959 Compare August 19, 2026 14:29
When `max_energy_range_uj` is unreadable, `max_energy_reading` is 0, so the
wrap-around branch still ran but corrected nothing, emitting a large negative
energy delta straight into the CPU and total energy. Drop the sample instead,
matching the EMI backend's behaviour. The same guard also covers driver resets
and the transient read-error fallback.

A failed read previously returned `Energy(0)`, which looked like a wrap-around
and, with a readable `max_energy_range_uj`, injected a spurious positive delta
of up to one counter range. `_get_value` now returns None, and the sample is
skipped and re-baselined. The uncorrectable-wrap warning is emitted once per
file instead of on every measurement cycle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@davidberenstein1957
davidberenstein1957 force-pushed the fix/rapl-wraparound-negative-energy branch from d4a9959 to f59ac22 Compare August 20, 2026 06:14
@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: 🔧 Request changes (small)

The bug is real, and worse than the description says:

  • When max_energy_range_uj can't be read, a wrap produces a negative delta.
  • On master, _get_value() returns 0 on a transient read error. That injects a spurious +max_energy_reading (up to about 0.07 kWh), and the next sample then counts the whole since-boot counter as a single delta.

Returning None from _get_value() and skipping values that still go backwards after correction fixes both. This PR merges cleanly with #1344, and I'd suggest landing it first.

Must fix:

  1. Crash in IntelRAPL.start() once merged with master.
    • Master's mirror-counter detection does float(rapl_file.last_energy) (codecarbon/core/cpu.py ~L893).
    • With this PR, that value is None when a counter can't be read at start(), giving TypeError: float() argument must be ... not 'NoneType' from CPU.start(). I reproduced this on the PR merged with master.
    • Fix: skip None values in that list comprehension, and add a test.
    • Please rebase on master and rerun the RAPL tests: CI was green on the old base, before the mirror dedup landed.

Should fix:
2. The description overstates the scope. The guard covers a driver reset or suspend/resume only when the max range is unknown. With a readable max, a counter reset still becomes energy + max - last, a large positive delta. Either reword, or add a plausibility bound (e.g. a delta above what TDP × interval allows gets skipped). A follow-up is fine.
3. Mention the _get_value() → None change in the description. It's the most impactful part of the PR.

Nit:

  • The wrap formula omits the +1 µJ. It's negligible.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

Made the changes in b5aaca1: merged master, skipped unreadable (None) counters in the start() mirror detection with a test, and reworded the description (scope, plus the _get_value() -> None change).
Not done: the TDP-based plausibility bound for resets with a readable max (fine as a follow-up, as suggested) and the +1 µJ wrap nit (negligible).

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.

RAPL counter wrap yields negative energy when max_energy_range_uj is unreadable

2 participants