ci: test every Python version the package promises - #1342
Open
davidberenstein1957 wants to merge 4 commits into
Open
ci: test every Python version the package promises#1342davidberenstein1957 wants to merge 4 commits into
davidberenstein1957 wants to merge 4 commits into
Conversation
requires-python is >=3.10 and the classifiers list 3.10-3.14, but test-package.yml only exercised 3.12-3.14. Add 3.10 and 3.11 to the matrix with fail-fast: false so one version's failure does not hide the others. Also add a bare-install smoke job to package-validation-reusable.yml: the existing wheel job syncs the dev group before installing the wheel, so it cannot catch a runtime dependency that is missing from [project.dependencies]. The new job installs only the wheel, on the floor interpreter, and runs an OfflineEmissionsTracker plus the CLI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1342 +/- ##
==========================================
+ Coverage 91.39% 91.43% +0.03%
==========================================
Files 49 49
Lines 5056 5056
==========================================
+ Hits 4621 4623 +2
+ Misses 435 433 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The amdsmi import-failure tests re-import codecarbon.core.gpu_amd with a patched __import__ and restore sys.modules afterwards, but importlib also rebinds gpu_amd on the codecarbon.core package, and that binding was left pointing at the throwaway module whose amdsmi is None. On Python < 3.12 mock.patch resolves a dotted target through getattr on the parent package, so every later patch of codecarbon.core.gpu_amd.amdsmi hit the throwaway module while the code under test still used the real one, failing with AttributeError: 'NoneType' object has no attribute 'amdsmi_init'. Python 3.12+ resolves via pkgutil.resolve_name and hid the leak. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tracker.stop() returns None on every internal give-up path, so printing it left the smoke job green on a silently broken wheel. Assert the shape rather than the magnitude, since 0.0 is legitimate on a runner without RAPL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957
marked this pull request as ready for review
August 12, 2026 19:14
The new bare-install-smoke job lives in a workflow the `package` paths filter does not watch, so it never ran on this PR and would first execute on master. Watch the two workflow files too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #1338.
requires-python = ">=3.10"(pyproject.toml:9) and the classifiers advertise 3.10 through 3.14. The matrix ran["3.12", "3.13", "3.14"]. Two of five promised versions were exercised by nothing — confirmed across all workflows;package-validation-reusable.ymlpins 3.12 for both its jobs and nothing else touches the SDK matrix.Honest framing: no user is broken today
I tried to produce a break on 3.10 and could not. A clean 3.10 venv resolved and installed from source with only declared runtime dependencies;
import codecarbonworked;codecarbon --helpworked; a realOfflineEmissionsTrackerrun returned emissions (5.95e-07 kg). Full non-integration suite on the same machine: 3.10 → 619 passed / 5 failed, 3.12 → 623 passed / 1 failed, with non-overlapping failure sets that all pass in isolation — macOS shared-state ordering flakes, not version incompatibilities. Same 624 tests collected on both. Grepping for the usual hazards (datetime.UTC,tomllib,StrEnum,ExceptionGroup,itertools.batched,match) found zero hits incodecarbon/.So what is broken is the enforcement, not the code: nothing stops the first
datetime.UTCfrom shipping. This is the cheapest moment to fix it, precisely because 3.10 is green and widening the matrix costs no fix-the-code work.Changes
test-package.yml— matrix widened to["3.10", "3.11", "3.12", "3.13", "3.14"], plusfail-fast: falseso a single-version failure is diagnosable rather than confusing.package-validation-reusable.yml— newMIN_PYTHON_VERSION: "3.10"and abare-install-smokejob that installs only the built wheel into an empty 3.10 venv, then runs import,codecarbon --help, and a 2sOfflineEmissionsTracker. This closes a second real gap: the existingtest-package-from-wheeljob runsuv sync --group devbefore installing the wheel, so a runtime dependency missing from[project.dependencies]but present in the lockfile goes unnoticed. It runs on PRs, sincepackage.ymlcalls this workflow onpull_request.tests/test_gpu_amd.py— out of the stated scope of this PR, flagged explicitly so nobody is surprised by it in the diff. It is not a matrix change; it is the fix required to make 3.10 and 3.11 green, and without it this PR cannot go in._import_gpu_amd_with_amdsmi_errorre-importscodecarbon.core.gpu_amdwith a poisoned__import__and then restoressys.modules. That is not enough:importlib.import_modulealso rebindsgpu_amdas an attribute of the parentcodecarbon.corepackage, and on Python < 3.12mock.patch("codecarbon.core.gpu_amd.<x>")resolves its target by walking that attribute chain rather than bysys.moduleslookup. The stale binding therefore sent every later test's patches to the throwaway module, so the AMD tests failed on 3.10/3.11 while passing on 3.12+. The teardown now restores the parent attribute too. Nine lines, test-only, no runtime code touched.bare-install-smokeasserts a result, not just the absence of a crashThe first revision only printed
tracker.stop(). Sincestop()returnsNoneon every internal give-up path — tracker never started, another instance holding the lock, emissions engine failed to initialise — that job would have printedemissions: Noneand still exited 0, i.e. stayed green on a wheel whose tracker does not work at all. It now asserts the shape of the result and deliberately tolerates a zero:No magnitude assertion. A runner with no RAPL falls back to constant CPU power, and
0.0over a 2-second window is a legitimate outcome there; asserting> 0would be the flaky version of this check. Verified locally on a bare install: returns5.46e-07, assertions pass.Cost
Three matrix jobs become five. Roughly +12-18 CI minutes per triggering PR, plus ~2 min for the smoke job; wall clock unchanged since matrix jobs run in parallel, and this is a public repo so the minutes are free.
Choices against the original proposal
UPrule change.ruff check --select UP --target-version py310 codecarbon/reports 552 errors, 521 auto-fixable, essentially all cosmetic (Tuple→tuple) — andUPdoes not flagfrom datetime import UTCat all, so it would not catch the hazard used to motivate it. A 500-line stylistic diff that misses the stated threat isn't a cheap first line of defence.include/primarysplit. Codecov merges matrix uploads rather than double-counting; the repo already uploads three times today.Verified vs. unverified
Verified locally: both workflow files parse as YAML, the matrix expands to exactly five entries, the smoke script survives YAML block-scalar and
python -c "…"shell quoting (round-tripped throughyaml.safe_load), and its substance succeeds on a bare install here. Full suite626 passed, 21 skipped;uv run pre-commit run --all-filespasses.Unverified (cannot run Actions locally): that
uv python install 3.10/3.11resolve onubuntu-24.04(very likely — uv ships its own builds); that the suite passes on 3.10/3.11 on Linux CI rather than macOS; and that the smoke job's tracker run succeeds on a runner with no RAPL. If 3.10 or 3.11 fails on Linux in a way it doesn't here, this becomes a fix-the-code task —fail-fast: falseis what makes that legible. If five interpreters per PR proves too slow, moving 3.10/3.11 to push-and-nightly is the escape hatch; I did not pre-build it.Still open
Deciding the
requires-pythonfloor before any extras split lands, andtest-package.yml:43installingdash/fireby hand instead of via the[carbonboard]extra, which lets packaging metadata drift from what CI actually installs.🤖 Generated with Claude Code