fix(core): don't crash import cuda.core when CUDA_CORE_DONT_FIX_TAB_COMPLETION is not an integer - #2535
Open
LeSingh1 wants to merge 1 commit into
Open
fix(core): don't crash import cuda.core when CUDA_CORE_DONT_FIX_TAB_COMPLETION is not an integer#2535LeSingh1 wants to merge 1 commit into
import cuda.core when CUDA_CORE_DONT_FIX_TAB_COMPLETION is not an integer#2535LeSingh1 wants to merge 1 commit into
Conversation
`cuda/core/__init__.py` reads `CUDA_CORE_DONT_FIX_TAB_COMPLETION` with a
bare `int(os.environ.get(..., "0"))` at import time. `int()` raises for any
value that is not a base-10 integer, and `os.environ.get` returns the empty
string (not the `"0"` default) when the variable is set but empty, so:
export CUDA_CORE_DONT_FIX_TAB_COMPLETION=
python -c "import cuda.core"
ValueError: invalid literal for int() with base 10: ''
Clearing a variable with `export VAR=` is the usual way to neutralize it in
a shell profile, a Dockerfile, or a CI job spec, and `=true` / `=yes` are
the obvious guesses for a boolean-looking opt-out. All of them make the
whole package unimportable, which is a hard failure for a knob whose only
purpose is to skip an optional `rlcompleter` patch.
Parse the value leniently instead. Integer values keep their existing
meaning (non-zero opts out, so `0` and `00` still install the patch), while
a non-integer, non-empty value is honored as an opt-out rather than being
silently ignored. Unset and empty/whitespace-only both mean "not set".
Also document the variable, which was not listed on the environment
variables page, and drop the stale "only installed in interactive mode"
comment: the interactivity gate was intentionally removed in NVIDIA#2055 ("Always
install the monkeypatch"), so the patch has been unconditional since then.
The new parametrized test asserts the resulting behavior for eight values;
four of them ("", " ", "true", "yes") fail on main because the subprocess
exits non-zero with the ValueError above.
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/cuda/core/__init__.pyreads the tab-completion opt-out with a bareint()at import time:os.environ.get(name, "0")returns the empty string, not the"0"default, when the variable is set but empty — andint("")raises. Because_patch_rlcompleter_for_cython_properties()is called unconditionally at module scope, the exception escapesimport cuda.core:The same happens for
=true,=yes,=on, or any other non-integer value.Clearing a variable with
export VAR=is the standard way to neutralize it in a shell profile, a DockerfileENV, or a CI job spec, and boolean-looking spellings are the obvious guess for a knob namedDONT_FIX_.... Every one of them makes the whole package unimportable — a hard, confusing failure for a variable whose only job is to skip an optionalrlcompleterpatch.Fix
Parse the value leniently. Integer values keep exactly their previous meaning (non-zero opts out, so
0and00still install the patch); a non-integer, non-empty value is honored as an opt-out rather than silently ignored; unset and empty/whitespace-only both mean "not set".cuda.corealready takes the tolerant approach for its other integer environment variable —default_stream()in_stream.pyxparsesCUDA_PYTHON_CUDA_PER_THREAD_DEFAULT_STREAMwithstrtol, with an explicit comment that weird values are handled rather than fatal. This brings the opt-out in line with that.Resulting behavior:
CUDA_CORE_DONT_FIX_TAB_COMPLETION""/" ""0"/"00""1"/"2""true"/"yes"Also in this PR
cuda_core/docs/source/environment_variables.rstlists the runtime environment variables that affectcuda.core, butCUDA_CORE_DONT_FIX_TAB_COMPLETIONwas missing from it even though the code describes it as an "explicit opt-out for users".1.2.0-notes.rst.Test
test_opt_out_env_var_valuesincuda_core/tests/test_rlcompleter_patch.pyrunsimport cuda.corein a subprocess for eight values of the variable and asserts both that the import succeeds and whether therlcompleterpatch was installed (the stdlibrlcompletermodule has nopropertyattribute of its own, so its presence is exactly the patch signal).Four of the eight cases —
""," ","true","yes"— fail onmain: the subprocess exits non-zero with theValueErrorabove.The test needs no CUDA device; the opt-out is evaluated at import time.
Verification I could and could not do
ValueErroron 4 of the 8 values and agrees with the new one on the other 4.ruff check/ruff format --checkon both changed Python files: no new findings versus anupstream/mainbaseline of the same files (the pre-existingUP038in__init__.pyandARG001in the test file are untouched).python -m py_compileon both changed Python files.cuda_coresuite. I do not have an environment wherecuda.coreis importable (no CUDA driver / no built extension modules), so the test was written against the existing helpers in that module but not executed. Please treat CI as the first real run.