Skip to content

fix: raise on offline tracker configuration errors - #1334

Open
davidberenstein1957 wants to merge 9 commits into
masterfrom
fix/suppress-constructor-failures
Open

davidberenstein1957 wants to merge 9 commits into
masterfrom
fix/suppress-constructor-failures

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Removes @suppress(Exception) from OfflineEmissionsTracker.__init__ in codecarbon/emissions_tracker.py, so configuration errors raised during construction propagate to the caller instead of being swallowed. Also wraps the body of BaseEmissionsTracker.__init__ (after _configure_multiple_runs) in try/except Exception, releasing self._lock if it was acquired before re-raising, so a constructor failure with allow_multiple_runs=False does not leave a stale lock blocking the next tracker. The offline-only region and country_2letter_iso_code type checks, which run before super().__init__, now raise TypeError with a descriptive message instead of a bare assert. In @track_emissions, tracker construction is now wrapped in its own try/except: on failure, the error is logged and the decorated function still runs untracked (the call to fn(*args, **kwargs) sits outside the except block, so the construction error is not chained via __context__/exc_info to any exception the function itself raises).

Related Issue

Fixes #1311

Motivation and Context

The decorator swallowed configuration errors raised during construction, most visibly the OSError from _set_from_conf when output_dir does not exist. The constructor then returned an object that had never reached _initialize_runtime_state() / _initialize_scheduler_state(), so it had no _start_time, _hardware, _scheduler, or _output_handlers. The subsequent start() and stop() calls are themselves suppressed, so they degraded into AttributeError warnings, and the run finished with no emissions file and no exception. Under the CLI default log_level="error", nothing was printed at all. The online EmissionsTracker already raises for the same input, so this also removes an asymmetry between the two constructors. Suppression on start, flush, and stop is deliberately left in place, since runtime measurement errors must never crash a user job; only construction, where the object invariants were never established, now fails loudly. This is a behavior change at a public boundary and is intended to ride a minor release (3.4.0), not a patch.

How Has This Been Tested?

Added test_offline_tracker_raises_on_invalid_output_dir in tests/test_offline_emissions_tracker.py, asserting both the offline and the online tracker raise OSError for a non-existent output_dir. It fails on master and passes with this change. test_offline_tracker_raises_on_invalid_region covers the offline-only region check (TypeError for a non-string region), and test_offline_tracker_raises_on_invalid_country_2letter_iso_code covers the same for country_2letter_iso_code; both new tests fail if @suppress(Exception) is restored on the constructor. test_offline_tracker_lowercases_valid_region and test_offline_tracker_uppercases_valid_country_2letter_iso_code guard against the stricter checks rejecting valid input. test_constructor_failure_releases_lock (in tests/test_emissions_tracker.py) asserts a constructor failure with allow_multiple_runs=False releases the lock file so a subsequent tracker can start. test_track_emissions_runs_function_when_tracker_construction_fails and test_untracked_function_errors_are_not_chained_to_construction_error cover the @track_emissions decorator: the function still runs untracked on a construction failure, and the construction error is not chained to an exception the function itself raises.

uv run pytest tests/test_offline_emissions_tracker.py tests/test_emissions_tracker.py -q
35 passed

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
  • 🟠 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.

CHANGELOG.md does not exist on master yet, so the release note lives in docs/reference/api.md under "Constructor errors (changed in v3.4.0)": OfflineEmissionsTracker no longer swallows exceptions raised while constructing the tracker; configuration errors (e.g. a non-existent output_dir) now propagate to the caller, matching EmissionsTracker. Suppression on start/flush/stop is unchanged.

@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 93.85%. Comparing base (e159a68) to head (9f42c68).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1334      +/-   ##
==========================================
+ Coverage   93.66%   93.85%   +0.18%     
==========================================
  Files          50       50              
  Lines        5591     5610      +19     
==========================================
+ Hits         5237     5265      +28     
+ Misses        354      345       -9     

☔ 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/suppress-constructor-failures branch from 85eedc8 to 3d7c6ec Compare August 19, 2026 09:18
@davidberenstein1957
davidberenstein1957 force-pushed the fix/suppress-constructor-failures branch 2 times, most recently from df16736 to 7b8487b 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: 💬 Needs a maintainer decision (leaning approve, for a minor release)

The bug is real. A bad output_dir returns a half-built OfflineEmissionsTracker that silently records nothing, and the new tests fail on master. Removing @suppress(Exception) from OfflineEmissionsTracker.__init__ (emissions_tracker.py:1312) matches the online EmissionsTracker, which already raises. start, stop and flush stay suppressed.

Behaviour change to be aware of:

  • CodeCarbon's contract has been "never crash user code". With this PR:
    • @track_emissions(offline=True, output_dir="missing") builds the tracker inside wrapped_fn, so the decorated function never runs and the exception reaches the user's training job. Before, the job ran untracked.
    • with OfflineEmissionsTracker(...) now raises too.
  • Suggestion for keeping both behaviours: keep direct construction strict (this PR), but in track_emissions catch construction errors, log them at ERROR, and run the wrapped function untracked.
  • The docs note says "changed in v3.4.0" (latest tag is v3.3.1), so this must go into a minor release, not a patch. Please also add a changelog entry.

Nit:

  • test_offline_tracker_raises_on_invalid_region expects an AssertionError from an assert (emissions_tracker.py ~L1344). Under python -O, asserts are stripped and nothing is raised. Please raise ValueError explicitly.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

Made the changes in e77657f: region / country_2letter_iso_code type checks now raise ValueError instead of assert.

Not done, pending a maintainer decision: whether track_emissions should catch construction errors and run the function untracked, and which release this lands in. No changelog file exists in the repo, so the entry is left for the release notes.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

Made the changes in 1e238ba: @track_emissions now logs construction errors and runs the function untracked, same as start_task. Direct construction still raises. Targeting a minor release (v3.4.0).

@github-actions github-actions Bot added size/L and removed size/S labels Sep 23, 2026
Comment thread codecarbon/emissions_tracker.py Resolved
davidberenstein1957 and others added 9 commits October 31, 2026 13:55
`@suppress(Exception)` on `OfflineEmissionsTracker.__init__` swallowed
configuration errors such as a missing `output_dir`, returning a half-built
object with no `_start_time`, `_hardware` or `_scheduler`. `start()` and
`stop()` then failed silently and no emissions were recorded at all.

Construction now raises, matching `EmissionsTracker`. The suppression on
`start`/`flush`/`stop` is kept, so runtime measurement errors still cannot
crash a user's job. The behaviour change is noted in the docs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Asserts are stripped under python -O, so a non-string region or
country_2letter_iso_code would pass silently.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
track_emissions now logs construction errors and calls the function
without tracking, matching start_task. Direct construction still raises.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…enting __init__

Wrapping the whole BaseEmissionsTracker.__init__ body in try/except
re-indented ~60 lines, which already conflicted with master on rebase and
will conflict with every future change to __init__. A small decorator gives
the same behaviour with a two-line diff to __init__.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JfRKgUd281URY6fMyNAQmh
@benoit-cty
benoit-cty force-pushed the fix/suppress-constructor-failures branch from 7e7de76 to 9f42c68 Compare October 31, 2026 14:07

Copy link
Copy Markdown
Contributor

Rebased on master (cfabf92 -> e159a68; the old merge commit was dropped). The head is now 9f42c68.

  • Rebase: one conflict in BaseEmissionsTracker.__init__ (master added lines to the body that the PR had re-indented). Resolved by re-applying the try/except on master's body. No commit was redundant: master still has @suppress(Exception) on OfflineEmissionsTracker.__init__, the assert type checks, and no untracked fallback in @track_emissions.
  • Re-indented __init__ (284782e): wrapping the whole __init__ body in try/except re-indented about 60 lines, and that is what conflicted. A small _release_lock_on_error decorator on __init__ now does the same job, so the __init__ diff is down to 1 line. test_constructor_failure_releases_lock still passes, and it fails when the decorator is removed.
  • Docs (9f42c68): the v3.4.0 note in docs/reference/api.md now says that a non-string region/country_2letter_iso_code raises TypeError, and that @track_emissions logs the error and runs the function untracked.
  • PR body: two small corrections. ValueError is now TypeError, to match the code and the tests. I removed "Master has since merged this change in", which was not true. One more thing in the body is now out of date: the Description still says the lock release wraps the __init__ body in try/except, but after 284782e it is a decorator.
  • Coverage: all changed lines in codecarbon/ are covered by tests/test_offline_emissions_tracker.py and tests/test_emissions_tracker.py (100% of changed lines).
  • Tests: these 2 files: 50 passed. Full suite (-m 'not integ_test'): the only failures are 4 RAPL permission tests, which also fail on master in this environment because it runs as root. pre-commit passes.

For the maintainer:

  • Behaviour change in online mode: with @track_emissions, a failure while building the online tracker no longer raises. The function now runs untracked, as start_task already does. This follows the earlier review suggestion; please confirm it is wanted.
  • Release: the change is labelled v3.4.0 (master is 3.3.2), so it needs a minor release.
  • Possible follow-up: test_constructor_failure_releases_lock relies on the assert self._tracking_mode in [...] check, and asserts are skipped under python -O.

Generated by Claude Code

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.

OfflineEmissionsTracker silently returns a half-built object when construction fails

4 participants