fix(core): don't abort the build when an integer build knob is set but empty - #2547
Open
LeSingh1 wants to merge 1 commit into
Open
fix(core): don't abort the build when an integer build knob is set but empty#2547LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
cuda_core/build_hooks.py reads two integer knobs from the environment with
a bare int():
COMPILE_FOR_COVERAGE = bool(int(os.environ.get("CUDA_PYTHON_COVERAGE", "0")))
nthreads = int(os.environ.get("CUDA_PYTHON_PARALLEL_LEVEL", os.cpu_count() // 2))
os.environ.get returns the empty string, not the default, when a variable is
set but empty -- and `VAR= pip install .` (or an empty Dockerfile ENV, or an
unfilled CI job variable) is exactly how these get neutralised. The first
line runs at module scope, so the failure lands while pip is still importing
the PEP 517 backend:
$ CUDA_PYTHON_COVERAGE= pip install ./cuda_core
ValueError: invalid literal for int() with base 10: ''
cuda_core/setup.py carries the same two expressions verbatim.
Add build_hooks.env_int and route all four call sites through it. Unset and
empty/whitespace-only both fall back to the default; a value that is set to
something non-integer still raises, but names the variable
(`environment variable CUDA_PYTHON_COVERAGE='yes' must be an integer`)
instead of an anonymous int() failure. Silently ignoring `=yes` would be
worse than stopping here -- it would hand back a build with no coverage
instrumentation.
Also stop assuming os.cpu_count() returns a number. It is documented to
return None when the count cannot be determined, which made the nthreads
default a TypeError. Fall back to a serial build instead.
setup.py already imports build_hooks, so both build entry points now parse
these knobs through the same helper rather than duplicating the expression.
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_core/build_hooks.pyreads two integer build knobs with a bareint():os.environ.get(name, default)returns the empty string, not the default, when a variable is set but empty — andVAR= pip install ., an emptyENVin a Dockerfile, or an unfilled CI job variable is exactly how these get neutralised.The first line runs at module scope, so the failure lands while pip is still importing the PEP 517 backend, before any build output:
cuda_core/setup.pycarries the same two expressions verbatim (lines 13–14), so the second build entry point fails the same way.Measured against the module as it stands on
main:CUDA_PYTHON_COVERAGEmain0/1""ValueError: invalid literal for int() with base 10: ''" "ValueError"yes"ValueError(correct to reject, but the message names nothing)Separately,
os.cpu_count()is documented to returnNonewhen the count cannot be determined, which makes thenthreadsdefaultNone // 2→TypeError.Fix
Add
build_hooks.env_int(name, default)and route all four call sites through it:CUDA_PYTHON_COVERAGE=yeswould hand back a build with no coverage instrumentation, which is worse than stopping — but names the variable:environment variable CUDA_PYTHON_COVERAGE='yes' must be an integer;os.cpu_count() or 1so an undetermined CPU count falls back to a serial build instead of aTypeError.setup.pyalready doesimport build_hooks, so both entry points now share one helper instead of duplicating the expression.Integer values keep their exact current meaning:
0→ off, non-zero → on, andCUDA_PYTHON_PARALLEL_LEVEL=4still yields 4.Tests
Added
TestEnvIntto the existingcuda_core/tests/test_build_hooks.py(which already loadsbuild_hooks.pyby path via_load_build_hooks()and needs no builtcuda.core):env_intover unset /""/" "/"0"/"4"/" 4 ";test_backend_imports_with_an_empty_coverage_flagreloads the backend module withCUDA_PYTHON_COVERAGE=""and" "and asserts it imports withCOMPILE_FOR_COVERAGE is False— this is the regression test for the crash above.Verification I could and could not do
upstream/mainand the fixedbuild_hooks.pyby path and ran the new test bodies against each. Onmain, importing the backend withCUDA_PYTHON_COVERAGE=""and" "raisesValueError(both new cases fail); with the fix both import cleanly withCOMPILE_FOR_COVERAGE is False. Theenv_inttable above was produced the same way.ruff check/ruff format --checkandpython -m py_compileclean on all three changed files.pytest cuda_core/tests/test_build_hooks.pyitself. The module doesfrom cuda.pathfinder import get_cuda_path_or_homeat import, andcuda.pathfindercannot be imported on macOS (it resolvesdlinfo, which does not exist there), so collection fails locally for reasons unrelated to this change. Cython and setuptools are present, andbuild_hooks.pyitself loads fine — that is what the verification above exercises. Please treat CI as the first real run of the test file.