Skip to content

fix: count DRAM in RAPL energy total - #1344

Open
davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/rapl-include-dram
Open

davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/rapl-include-dram

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Description

On Linux, setting rapl_include_dram=True selected DRAM RAPL domains and read their energy every cycle, but that energy never reached the reported total, so the result was identical to rapl_include_dram=False even though the log claimed DRAM was included. _create_rapl_files in codecarbon/core/cpu.py now assigns DRAM domains the same Power-Gadget-compatible display name used for package/psys domains, so the aggregator in codecarbon/external/hardware.py picks them up. A follow-up commit switches mirror-domain detection to an explicit RAPLFile.is_dram flag instead of the display name, and adjusts the no-package/no-psys fallback to include DRAM only when the flag is set.

Related Issue

Fixes #1305

Motivation and Context

DRAM domains fell into an else branch and kept their raw sysfs name dram, which the aggregator's ^Processor Energy Delta_\d regex filters out, silently dropping DRAM energy from the reported total whenever a user opted in to measuring it. This also brings Linux RAPL in line with Windows EMI, which already names every selected channel this way.

How Has This Been Tested?

Added test_rapl_include_dram_energy_is_aggregated in tests/test_rapl_parameters.py, parametrized over both flag values: it builds a fake sysfs tree with package-0 and dram, advances both counters, calls get_cpu_details(), and asserts the summed Processor Energy Delta_* value. It fails on master (package-only) and passes with this change. test_rapl_include_dram_true_explicit was updated since it previously asserted a RAPL file was still named dram, which is exactly the behavior being fixed. The suite is gated on Linux; locally (macOS) the tests were driven directly with sys.platform patched, and all tests in tests/test_rapl_parameters.py and tests/test_rapl_mmio_scanning.py pass.

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)

Note: this is a measurement change. Anyone already setting rapl_include_dram=True will see reported CPU energy increase (correctly), since DRAM is typically 10-20% of package power on memory-heavy workloads.

AI Usage Disclosure

  • 🟥 AI-vibecoded
  • 🟠 AI-generated
  • ⭐ AI-assisted
  • ♻️ No AI used

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.70%. Comparing base (e5e46ab) to head (c2cc54b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1344   +/-   ##
=======================================
  Coverage   91.70%   91.70%           
=======================================
  Files          49       49           
  Lines        5157     5159    +2     
=======================================
+ Hits         4729     4731    +2     
  Misses        428      428           

☔ 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 17:37
@davidberenstein1957
davidberenstein1957 requested a review from a team as a code owner August 12, 2026 17:37
@davidberenstein1957
davidberenstein1957 force-pushed the fix/rapl-include-dram branch 2 times, most recently from f5a7183 to cb38949 Compare August 19, 2026 14:29
Aggregate the dram domain into the reported processor energy alongside package/psys, with tests covering aggregation and the non-power-domain fallback.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@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

The bug is real. DRAM domains keep the raw name dram, and _get_energy_from_cpus / _get_power_from_cpus only sum ^Processor Energy Delta_\d / ^Processor Power. So on master, rapl_include_dram=True does nothing. Renaming DRAM to Processor Energy Delta_N fixes that, but it collides with code that landed on master after this branch was created.

Must fix:

  1. It breaks master's mirror detection, and a master test fails once merged.
    • IntelRAPL.start() (codecarbon/core/cpu.py ~L890-895) leaves DRAM out of mirror detection with if "dram" not in rapl_file.name.lower(). After the rename, DRAM files are called Processor Energy Delta_N(kWh), so the guard never matches.
    • On the PR merged with master, tests/test_rapl_mmio_scanning.py::test_rapl_start_keeps_dram_when_it_matches_a_package_counter fails: DRAM gets flagged as a mirror of a package counter and dropped.
    • Fix: base the exclusion on the domain type, not the display name. For example, keep domain_name on RAPLFile or add an is_dram flag. Also update that test's assert "dram" in details.
  2. The fallback path ignores the flag.
    • In the fallback branch of _select_domains_to_use ("No package or psys domains found, using all available domains"), readable_domains includes top-level DRAM regardless of rapl_include_dram.
    • With the rename, DRAM would then be summed as CPU energy even with the flag off. That contradicts the description.
    • Please filter DRAM by the flag there too. Add a test next to test_rapl_non_power_domain_keeps_its_own_name, which only covers core.

Worth documenting:

  • With the flag on, memory power ends up in both cpu_energy (RAPL DRAM) and ram_energy (the RAM estimate). docs/how-to/configuration.md says this is by design, but it should go in the changelog.
  • DRAM exposed as a package subzone (intel-rapl:N:M) is still skipped, so the flag stays a no-op there. That's pre-existing; maybe open an issue.
  • No psys+DRAM double count: DRAM is only added in the package branch.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

Made the changes in c2cc54b: merged master, DRAM mirror exclusion now uses a RAPLFile.is_dram flag (master's DRAM test updated and passing), and the fallback path respects rapl_include_dram (new test). Added the CPU+RAM double-count and subzone notes to the description's changelog section. Didn't open the subzone issue; that's for a maintainer to decide.

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_include_dram=True reads DRAM domains but never adds them to the energy total

2 participants