fix(bindings): don't abort the build when an integer build knob is set but empty - #2572
Open
LeSingh1 wants to merge 1 commit into
Open
fix(bindings): don't abort the build when an integer build knob is set but empty#2572LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
…t but empty
`CUDA_PYTHON_COVERAGE= pip install .` is how a variable gets neutralised --
the same shape as an empty value in a Dockerfile `ENV` or a CI job spec. Both
build entry points fed those values to a bare `int()`:
compile_for_coverage = bool(int(os.environ.get("CUDA_PYTHON_COVERAGE", "0")))
so an empty value raised `ValueError: invalid literal for int() with base 10:
''` while the PEP 517 backend module was still being imported, before any build
output, and without naming the variable at fault.
`PARALLEL_LEVEL` had a second problem. The branch was chosen with `is not
None`, so `PARALLEL_LEVEL=` took the deprecated path -- warning about a
variable the caller had explicitly neutralised, then crashing in `int("")` --
instead of falling through to `CUDA_PYTHON_PARALLEL_LEVEL`, which already
guarded itself with `or "0"`.
Route all three knobs through `env_int`, which treats unset and empty alike,
and reports a genuinely non-integer value (`CUDA_PYTHON_COVERAGE=yes`, which
would otherwise silently produce a build with no coverage instrumentation) with
the variable's name attached. `parallel_level()` keeps the deprecation warning
for a value that is actually set.
This mirrors the cuda.core change in NVIDIA#2547 for the cuda-bindings copies; the
two packages have separate build backends and neither imports the other's.
Refs NVIDIA#2547
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
cuda_bindingsreads three integer build knobs from the environment, and each one is fed straight to a bareint():1. An empty value aborts the build.
CUDA_PYTHON_COVERAGE= pip install .is how a variable gets neutralised — the same shape as an emptyENVin a Dockerfile or an unset job-spec variable in CI.int("")raiseswhile the PEP 517 backend module is still being imported, so the build dies before it emits any output and the traceback never names the variable at fault.
CUDA_PYTHON_PARALLEL_LEVELalready guards itself withor "0";CUDA_PYTHON_COVERAGEandPARALLEL_LEVELdo not.2.
PARALLEL_LEVELselects its branch onis not None. SoPARALLEL_LEVEL=takes the deprecated path: it warns about a variable the caller has explicitly neutralised, shadowsCUDA_PYTHON_PARALLEL_LEVEL(which may well be the one that is actually set), and then crashes inint("").3. A genuinely wrong value is not diagnosed.
CUDA_PYTHON_COVERAGE=yesraises the same anonymousinvalid literal for int()— worth stopping for, but worth naming.Fix
Add
env_int(name, default), which treats unset and empty alike and reports a non-integer value with the variable's name attached, plusparallel_level(), which keeps the deprecation warning for a value that is actually set and otherwise falls through toCUDA_PYTHON_PARALLEL_LEVEL.setup.pynow calls the same two helpers rather than keeping a second copy of the parsing, so the two build entry points cannot drift.Behaviour that does not change: a set, integer value parses exactly as before (including
0and negatives), andPARALLEL_LEVEL=<n>still warns and still wins overCUDA_PYTHON_PARALLEL_LEVEL.This mirrors #2547, which makes the same change for the
cuda.corecopies. The two packages have separate build backends and neither imports the other's, so the fixes are independent files; the wording of the helper is kept identical so a future reader sees one convention.Tests
New
cuda_bindings/tests/test_build_hooks.py, modelled on the existingcuda_core/tests/test_build_hooks.py(sameimportlibloader sosys.pathnever gains thecuda_bindings/source directory, same--noconftestnote). 15 cases covering empty/whitespace/unset/integer/non-integer values and all fourPARALLEL_LEVEL×CUDA_PYTHON_PARALLEL_LEVELcombinations.What I ran
Environment: macOS, no CUDA driver and no CUDA toolkit, so
cuda.bindingsis not built here.pytest cuda_bindings/tests/test_build_hooks.py --noconftest— 15 passed. These tests need onlysetuptools;build_hooks.pyimports Cython lazily inside_build_cuda_bindings, so the module loads without a toolkit. (--noconftestis required becausecuda_bindings/tests/conftest.pyimportscuda.bindings.driver;cuda_core/tests/test_build_hooks.pydocuments the same requirement.)build_hooks.pyandsetup.pyfrommainwith the new test file in place — all 15 fail (ValueError: invalid literal for int() with base 10: ''for the empty-value cases,AttributeError: module 'build_hooks' has no attribute 'env_int'for the rest).ruff checkandruff format --checkon the three changed files — clean, no new findings against amainbaseline for the same files.Refs #2547