fix(bindings): CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING=0 must not disable it - #2581
fix(bindings): CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING=0 must not disable it#2581LeSingh1 wants to merge 2 commits into
Conversation
…isable it
The suppression check tests the raw environment string for truthiness:
if os.environ.get("CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING"):
return
so `=0` -- the obvious way to spell "no, keep warning me" -- suppresses the
warning just as effectively as `=1`. The warning's own text says
"(Set CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING=1 to suppress this warning.)",
which reads as though 0 is the off value, and the other two boolean knobs in
this repository disagree with it:
cuda_bindings/.../_internal/runtime_linux.pyx:29
bool(int(os.getenv('CUDA_PYTHON_CUDA_PER_THREAD_DEFAULT_STREAM', default=0)))
cuda_core/cuda/core/__init__.py:44
if int(os.environ.get("CUDA_CORE_DONT_FIX_TAB_COMPLETION", "0")):
Both parse with int(), so `=0` is off for them. A user who sets all three to
0 gets the behaviour they asked for from two of them and the opposite from
this one, silently losing a compatibility warning that exists to explain why
their driver is too old.
Parse the value with int() here too. Unset and empty still mean "not
disabled". A value that is not an integer keeps the old set-means-disabled
behaviour, so anyone currently relying on a spelling like `=true` does not
start seeing the warning again -- `0` is the only input whose meaning
changes.
github-actions
Bot
added
the
cuda.bindings
| """Whether the user asked to suppress the major-version warning. | ||
|
|
||
| ``=0`` means "do not suppress". A bare truthiness test on the raw string | ||
| made ``CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING=0`` suppress the warning | ||
| -- the exact opposite of what the warning itself tells the user to type, | ||
| and the opposite of the other boolean knobs in this repository | ||
| (``CUDA_PYTHON_CUDA_PER_THREAD_DEFAULT_STREAM`` and | ||
| ``CUDA_CORE_DONT_FIX_TAB_COMPLETION``), which both parse their value with | ||
| ``int()``. | ||
|
|
||
| Unset and empty still mean "not disabled". A value that is not an integer | ||
| keeps the old set-means-disabled behaviour, so anyone currently relying on | ||
| a spelling like ``=true`` does not silently start seeing the warning again. | ||
| """ | ||
| raw = os.environ.get(_DISABLE_WARNING_ENV_VAR, "").strip() | ||
| if not raw: | ||
| return False | ||
| try: | ||
| return int(raw) != 0 | ||
| except ValueError: | ||
| return True |
There was a problem hiding this comment.
Make a helper function for parsing a bool-like value from an environment variable. Call it envvar_bool and put it in utils/__init__.py. Then just use that, passing in the environment variable name, from warn_if_cuda_major_version_mismatch.
There are other instances of parsing bool-like envvars throughout the codebase that could then be updated to use this new helper function.
| @pytest.mark.agent_authored(model="claude-opus-5") | ||
| @pytest.mark.parametrize( | ||
| ("raw", "expected"), | ||
| [ | ||
| pytest.param("0", False, id="zero"), | ||
| pytest.param(" 0 ", False, id="zero-padded"), | ||
| pytest.param("", False, id="empty"), | ||
| pytest.param(" ", False, id="blank"), | ||
| pytest.param("1", True, id="one"), | ||
| pytest.param("2", True, id="two"), | ||
| # Not an integer: keep the old set-means-disabled behaviour so no | ||
| # one relying on a spelling like `=true` starts seeing the warning | ||
| # again. | ||
| pytest.param("true", True, id="true"), | ||
| pytest.param("yes", True, id="yes"), | ||
| ], | ||
| ) | ||
| def test_disable_flag_parsing(self, monkeypatch, raw, expected): | ||
| monkeypatch.setenv("CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING", raw) | ||
| assert _version_check._warning_disabled() is expected | ||
|
|
||
| @pytest.mark.agent_authored(model="claude-opus-5") | ||
| def test_disable_flag_unset(self, monkeypatch): | ||
| monkeypatch.delenv("CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING", raising=False) | ||
| assert _version_check._warning_disabled() is False |
There was a problem hiding this comment.
Given my other suggestion of a generic helper function, the test should be also made generic (and not tied to a specific environment variable).
Replaces the flag-specific _warning_disabled with a shared envvar_bool, exported from cuda.bindings.utils. It recognises 1/true/yes/on and 0/false/no/off case-insensitively, falls back to C truthiness for other integers, and treats unset, empty, and whitespace-only as the caller's default. The helper lives in utils/_envvar.py rather than directly in utils/__init__.py because __init__ already imports _version_check, which needs the helper -- defining it in __init__ would make that import circular. It is re-exported so the public name is cuda.bindings.utils.envvar_bool. The parsing test moves to test_envvar.py and drives an arbitrary variable name instead of the version-check one.
|
Done — added One deviation from your suggestion, and I want to flag it rather than bury it: I put the function in It recognises On the other bool-like envvars: I surveyed them and most can't actually use this helper.
So the reachable conversions are One thing that's yours to decide: an unrecognised value like |
|
/ok to test 75efd63 |
mdboom
left a comment
mdboom
left a comment
There was a problem hiding this comment.
Thanks. Looks in good shape now.
|
2 participants
Problem
The suppression check tests the raw environment string for truthiness (
_version_check.py:35):so
CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING=0— the obvious way to spell "no, keep warning me" — suppresses the warning exactly as effectively as=1.Two things make that a trap rather than a convention:
(Set CUDA_PYTHON_DISABLE_MAJOR_VERSION_WARNING=1 to suppress this warning.), which implies0is the off value.int():A user (or a CI job template, or a Dockerfile) that sets all three to
0gets the behaviour they asked for from two of them and the opposite from this one — silently losing a compatibility warning whose whole purpose is to explain why their driver is too old for the wheel they installed.Fix
Parse the value with
int()here too, in a small_warning_disabled()helper.Compatibility: unset and empty still mean "not disabled", every non-zero integer still disables, and a value that is not an integer keeps the old set-means-disabled behaviour so nobody relying on a spelling like
=truestarts seeing the warning again.0is the only input whose meaning changes — which is the point.Also uses the env-var name constant in the warning text so the message and the lookup cannot drift apart.
Tests
Added to
cuda_bindings/tests/test_version_check.py:test_disable_flag_parsing— parametrised over0,0, empty, blank,1,2,true,yes.test_disable_flag_unset.test_warning_not_suppressed_when_env_var_is_zero— end-to-end throughwarn_if_cuda_major_version_mismatch, mirroring the existingtest_warning_suppressed_by_env_varit sits next to.The existing
test_warning_suppressed_by_env_var(=1) is unchanged and still passes.What I ran
Environment: macOS, no CUDA driver and no CUDA toolkit, so
cuda.bindingsis not built here.cuda_bindings/tests/test_version_check.pyitself — it importscuda.bindings.driverat module level._version_check.pyimports onlyos/threading/warningsat module level and pulls incuda.bindings.driverinside the function, so I loaded the real module with a stubbed driver (CUDA_VERSION=13000,cuDriverGetVersion -> 12080) and counted emitted warnings for each env value:python -m py_compile,ruff check,ruff format --checkon both changed files — clean, no new findings against amainbaseline.warn_if_cuda_major_version_mismatchhas exactly one non-test caller,Device_ensure_cuda_initializedincuda_core/cuda/core/_device.pyx:1613-1625, which calls it immediately after a successfulcuInit(0)and does not inspect the env var itself.