fix: raise on offline tracker configuration errors - #1334
davidberenstein1957 wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1334 +/- ##
==========================================
+ Coverage 91.69% 91.88% +0.19%
==========================================
Files 49 49
Lines 5152 5165 +13
==========================================
+ Hits 4724 4746 +22
+ Misses 428 419 -9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
85eedc8 to
3d7c6ec
Compare
3d7c6ec to
df16736
Compare
`@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>
df16736 to
7b8487b
Compare
Verdict: 💬 Needs a maintainer decision (leaning approve, for a minor release)The bug is real. A bad Behaviour change to be aware of:
Nit:
|
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>
|
Made the changes in e77657f: Not done, pending a maintainer decision: whether |
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>
|
Made the changes in 1e238ba: |
Description
Removes
@suppress(Exception)fromOfflineEmissionsTracker.__init__incodecarbon/emissions_tracker.py, so configuration errors raised during construction propagate to the caller instead of being swallowed. Also wraps the body ofBaseEmissionsTracker.__init__(after_configure_multiple_runs) intry/except Exception, releasingself._lockif it was acquired before re-raising, so a constructor failure withallow_multiple_runs=Falsedoes not leave a stale lock blocking the next tracker. The offline-onlyregionandcountry_2letter_iso_codetype checks, which run beforesuper().__init__, now raiseValueErrorwith a descriptive message instead of a bareassert. In@track_emissions, tracker construction is now wrapped in its owntry/except: on failure, the error is logged and the decorated function still runs untracked (the call tofn(*args, **kwargs)sits outside theexceptblock, so the construction error is not chained via__context__/exc_infoto any exception the function itself raises).Related Issue
Fixes #1311
Motivation and Context
The decorator swallowed configuration errors raised during construction, most visibly the
OSErrorfrom_set_from_confwhenoutput_dirdoes 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 subsequentstart()andstop()calls are themselves suppressed, so they degraded intoAttributeErrorwarnings, and the run finished with no emissions file and no exception. Under the CLI defaultlog_level="error", nothing was printed at all. The onlineEmissionsTrackeralready raises for the same input, so this also removes an asymmetry between the two constructors. Suppression onstart,flush, andstopis 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_dirintests/test_offline_emissions_tracker.py, asserting both the offline and the online tracker raiseOSErrorfor a non-existentoutput_dir. It fails on master and passes with this change.test_offline_tracker_raises_on_invalid_regioncovers the offline-only region check (ValueErrorfor a non-stringregion), andtest_offline_tracker_raises_on_invalid_country_2letter_iso_codecovers the same forcountry_2letter_iso_code; both new tests fail if@suppress(Exception)is restored on the constructor.test_offline_tracker_lowercases_valid_regionandtest_offline_tracker_uppercases_valid_country_2letter_iso_codeguard against the stricter checks rejecting valid input.test_constructor_failure_releases_lock(intests/test_emissions_tracker.py) asserts a constructor failure withallow_multiple_runs=Falsereleases the lock file so a subsequent tracker can start.test_track_emissions_runs_function_when_tracker_construction_failsandtest_untracked_function_errors_are_not_chained_to_construction_errorcover the@track_emissionsdecorator: the function still runs untracked on a construction failure, and the construction error is not chained to an exception the function itself raises.Screenshots (if appropriate):
N/A
Types of changes
AI Usage Disclosure
Checklist:
CHANGELOG.mddoes not exist on master yet, so the release note lives indocs/reference/api.mdunder "Constructor errors (changed in v3.4.0)":OfflineEmissionsTrackerno longer swallows exceptions raised while constructing the tracker; configuration errors (e.g. a non-existentoutput_dir) now propagate to the caller, matchingEmissionsTracker. Suppression onstart/flush/stopis unchanged. Master has since merged this change in.