Skip to content

fix(powermetrics): reject unsupported Macs and report 0 W when no samples - #1345

Merged
benoit-cty merged 2 commits into
masterfrom
fix/powermetrics-nan-totals
Oct 18, 2026
Merged

benoit-cty merged 2 commits into
masterfrom
fix/powermetrics-nan-totals

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Two fixes to codecarbon/core/powermetrics.py. First, get_details() now guards against an empty match list before averaging, so it reports 0.0 W instead of NaN when powermetrics produced no CPU Power: / GPU Power: lines, and logs a warning pointing to the sudoers setup docs the first time there are no CPU samples; the previously-duplicated CPU and GPU branches are now one loop. Second, _setup_cli() now raises SystemError unless the CPU is Apple Silicon (the same is_mac_arm check used by resource_tracker), so an Intel Mac without Power Gadget falls back to TDP / cpu_load instead of selecting powermetrics, since Intel powermetrics output has no CPU Power: X mW line. The now-unreachable Intel-Mac powermetrics branch in resource_tracker._try_platform_cpu_backend is removed, and the command now uses self._cli and drops a stray "" argument. Master's Popen timeout and early-return in get_details are kept.

Related Issue

Fixes #1306. Fixes #1313.

Motivation and Context

np.mean([]) returns nan and only emits a RuntimeWarning, so the surrounding except Exception never caught it. The NaN flowed into the tracker accumulators and, since NaN is absorbing under addition, made energy_consumed, emissions and emissions_rate NaN for the rest of the run, silently written to emissions.csv and POSTed to the API.

How Has This Been Tested?

New tests in tests/test_powermetrics.py cover empty logs (0 W, warning logged once), CPU-only logs, Intel / unknown CPU rejection, Apple Silicon setup, and the command built for Popen. test_resource_tracker.py now asserts that an Intel Mac does not use powermetrics. Full test-package passes.

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.60%. Comparing base (cb86f04) to head (6d91c3d).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1345      +/-   ##
==========================================
+ Coverage   92.58%   92.60%   +0.01%     
==========================================
  Files          49       49              
  Lines        5341     5339       -2     
==========================================
- Hits         4945     4944       -1     
+ Misses        396      395       -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 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/powermetrics-nan-totals branch 2 times, most recently from 5247f57 to f67379e Compare August 19, 2026 14:27
@davidberenstein1957 davidberenstein1957 changed the title fix(powermetrics): report 0 W when powermetrics returns no samples fix(powermetrics): reject unsupported Macs and report 0 W when no samples Aug 19, 2026
@davidberenstein1957
davidberenstein1957 force-pushed the fix/powermetrics-nan-totals branch from f67379e to c583b1d 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 (rebase needed)

Both fixes are real:

  • np.mean([]) gives NaN, which then poisons every total.
  • On an Intel Mac without Power Gadget, master selects powermetrics. Intel output has no CPU Power: X mW line, so every sample is NaN.

Making _setup_cli raise SystemError for non-Apple CPUs correctly falls back to TDP/cpu_load. Removing the stray "" argument and using self._cli is good too.

Must fix:

  1. Merge conflicts with master in codecarbon/core/powermetrics.py and tests/test_powermetrics.py.
    • Master commit 1ea5f93 rewrote _log_values to use Popen with a timeout, and added if not self._log_values(): return dict() in get_details.
    • The rebase must keep master's timeout and early return.
    • test_log_values_builds_clean_command mocks subprocess.call, which master no longer uses, so it needs rewriting against Popen.
  2. No warning is logged, despite the description.
    • get_details falls back to 0.0 silently when there are no CPU samples.
    • In powermetrics mode there is no further fallback, so a missing sudoers rule or an empty log reports 0 W CPU for the whole run with nothing in the logs.
    • Please add a logger.warning, once, that points to the powermetrics/sudoers troubleshooting docs.

Nits:

  • After this change, the Intel-Mac elif powermetrics.is_powermetrics_available() branch in core/resource_tracker.py (~L237) is dead code. Remove it or comment it.
  • There are now two different Apple Silicon checks: startswith("Apple") here and is_mac_arm (\bM\d) in resource_tracker. Please use one helper.
  • The title mentions rejecting unsupported Macs, but the description only covers the empty-sample fix. Please describe both.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

Made the changes in d537043: merged master (kept the Popen timeout and early return, rewrote the command test against Popen), added a one-time warning for empty CPU samples, switched to is_mac_arm, removed the dead Intel-Mac branch, and updated the description.

davidberenstein1957 and others added 2 commits October 18, 2026 11:49
…ples

Two failure modes in the powermetrics backend produced silently wrong
totals.

`_setup_cli` returned without setting `_cli` and without raising on an Intel
Mac, so `ApplePowermetrics()` succeeded there and `ResourceTracker` could
register an `AppleSiliconChip` on hardware that has none. It also crashed
with `AttributeError` when `detect_cpu_model()` returned None. Both paths now
raise `SystemError`, the argv uses the validated `self._cli`, and a stray
empty argument is dropped.

Averaging an empty sample list returned NaN (numpy) or raised
(`statistics.fmean`), and the NaN poisoned every downstream total. Report
0 W instead. This also drops the numpy dependency from this module.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@benoit-cty
benoit-cty force-pushed the fix/powermetrics-nan-totals branch from d537043 to 6d91c3d Compare October 18, 2026 10:00
@benoit-cty
benoit-cty enabled auto-merge October 18, 2026 10:00
@benoit-cty
benoit-cty merged commit 8fe47ad into master Oct 18, 2026
12 checks passed
@benoit-cty
benoit-cty deleted the fix/powermetrics-nan-totals branch October 18, 2026 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants