fix(ci): fail cleanly on a malformed cuda.build.version instead of tracebacking - #2544
Open
LeSingh1 wants to merge 1 commit into
Open
fix(ci): fail cleanly on a malformed cuda.build.version instead of tracebacking#2544LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
check_pixi_cuda_version.py returns a diagnostic exit code for every problem
it anticipates -- missing versions.yml, missing cuda.build.version, missing
pixi.toml, missing feature key -- and then unpacks the version with no
checking at all:
major, minor, *_ = build_version.split(".")
YAML makes that easy to break. `version: "13.3.0"` is quoted today, but drop
the quotes and a two-component value loads as a float and a bare number as an
int, neither of which has `.split`. Against the real pixi.toml files:
version: 13.3 -> AttributeError: 'float' object has no attribute 'split'
version: 13 -> AttributeError: 'int' object has no attribute 'split'
version: -> AttributeError: 'NoneType' object has no attribute 'split'
version: [13, 3] -> AttributeError: 'list' object has no attribute 'split'
version: "13" -> ValueError: not enough values to unpack (expected at least 2, got 1)
All five escape as an uncaught traceback from a pre-commit hook, pointing at
this script rather than at the line the contributor edited.
Add `parse_build_version`, which returns `(major, minor)` only for a
`<major>.<minor>[.<patch>]` string of digits, and have `main` report the bad
value with the same `return 2` shape as its neighbours. The message names the
YAML quoting trap, since that is how the value goes wrong in practice.
Adds the first tests for this script. The parse tests cover the accepted
shapes and every rejected one; the end-to-end tests drive `main()` against a
temporary repo layout and assert exit 2 plus the diagnostic.
Contributor
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.
Problem
ci/tools/check_pixi_cuda_version.pyreturns a deliberate diagnostic exit code for every problem it anticipates — missingversions.yml, missingcuda.build.version, missingpixi.toml, missing feature key — and then unpacks the version with no checking at all:YAML makes that easy to break.
ci/versions.ymlquotes the value today:Drop the quotes and a two-component value loads as a float, a bare number as an int — neither has
.split. And a quoted-but-single-component value survives to the tuple unpacking and dies there instead.Driving
main()against the realpixi.tomlfiles with onlyci/versions.ymlvaried:version:valuemain"13.3.0"str13.3floatAttributeError: 'float' object has no attribute 'split'13intAttributeError: 'int' object has no attribute 'split'NoneAttributeError: 'NoneType' object has no attribute 'split'[13, 3]listAttributeError: 'list' object has no attribute 'split'"13"strValueError: not enough values to unpack (expected at least 2, got 1)All five escape as an uncaught traceback out of a pre-commit hook, pointing the contributor at this script rather than at the line they edited. The
except (KeyError, TypeError)above shows the intent was already to fail cleanly here.Fix
Add
parse_build_version, which returns(major, minor)only for a<major>.<minor>[.<patch>]string of digits andNoneotherwise, and havemainreport the bad value with the samereturn 2shape as its neighbours:The message names the quoting trap because that is how the value actually goes wrong.
No behavior change for well-formed input: the current
"13.3.0"still yieldsexpected="13.3.*"andcuda_feature="cu13", and running the tool against the real repo still exits 0.Tests
This script had no tests. Added
ci/tools/tests/test_check_pixi_cuda_version.py:parse_build_versionover the accepted shapes (13.3.0,12.9.1,13.3,13.3.0.1) and every rejected one (float, int,None, list,"13","","13.",".3","cuda.13").ci/versions.yml+ bothpixi.tomlfiles), monkeypatch the module's path constants, and assertmain()returns 2 with the diagnostic — and returns 0 for a well-formed version.The module imports PyYAML at module scope (the pre-commit hook supplies it via
additional_dependencies), so the test module opens withpytest.importorskip("yaml")rather than failing collection where PyYAML is absent.Verification
Executed in full (pure Python, no GPU):
upstream/mainmain()directly with monkeypatched path constants; five of the six raise uncaught.pytest ci/tools/testsis 73 passed.OK: ...and exits 0.upstream/mainand watch the tests fail" check: because the fix introduces a new function, the test module cannot even import againstmain(ImportError: cannot import name 'parse_build_version'). The evidence that matters is the table above, taken against unmodifiedupstream/maincode.ruff check/ruff format --checkclean on both files;toolshed/check_spdx.pyclean on the new file;python -m py_compileclean. Index verified clean before committing.Related
Independent of #2538, #2543 (both
check_mempool_hygiene.py) and #2496 (check_release_notes.py) — different files, no overlap.