Revert nvJitLink driver-major compatibility fallback#2355
Conversation
This reverts commit e83d434.
isVoid
added
bug
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
isVoid
changed the title
|
leofang
left a comment
leofang
left a comment
There was a problem hiding this comment.
LGTM. The inline comments below are nitpicks about tests for the cuLink backend path, which does not appear to have CI coverage today — feel free to defer.
-- Leo's bot
| def test_linker_options_as_bytes_invalid_backend(): | ||
| """Test LinkerOptions.as_bytes() with invalid backend""" | ||
| options = LinkerOptions(arch="sm_80") | ||
| with pytest.raises(ValueError, match="only supports 'nvjitlink' backend"): | ||
| options.as_bytes(backend) | ||
| options.as_bytes("invalid") | ||
|
|
||
|
|
||
| @pytest.mark.skipif(not is_culink_backend, reason="driver backend test") | ||
| def test_linker_options_as_bytes_driver_not_supported(): | ||
| """Test that as_bytes() is not supported for driver backend""" | ||
| options = LinkerOptions(arch="sm_80") | ||
| with pytest.raises(RuntimeError, match="as_bytes\\(\\) only supports 'nvjitlink' backend"): | ||
| options.as_bytes("driver") |
There was a problem hiding this comment.
nit: this resurrects a pre-#2320 test bug: as_bytes("driver") raises ValueError (the backend-string check fires before the availability check), so pytest.raises(RuntimeError) won't catch it — the test fails whenever it isn't skipped:
cuda-python/cuda_core/cuda/core/_linker.pyx
Lines 465 to 469 in f835032
#2320's parametrization was correct on both backends:
| def test_linker_options_as_bytes_invalid_backend(): | |
| """Test LinkerOptions.as_bytes() with invalid backend""" | |
| options = LinkerOptions(arch="sm_80") | |
| with pytest.raises(ValueError, match="only supports 'nvjitlink' backend"): | |
| options.as_bytes(backend) | |
| options.as_bytes("invalid") | |
| @pytest.mark.skipif(not is_culink_backend, reason="driver backend test") | |
| def test_linker_options_as_bytes_driver_not_supported(): | |
| """Test that as_bytes() is not supported for driver backend""" | |
| options = LinkerOptions(arch="sm_80") | |
| with pytest.raises(RuntimeError, match="as_bytes\\(\\) only supports 'nvjitlink' backend"): | |
| options.as_bytes("driver") | |
| @pytest.mark.parametrize("backend", ("invalid", "driver")) | |
| def test_linker_options_as_bytes_invalid_backend(backend): | |
| """Test LinkerOptions.as_bytes() with invalid backend""" | |
| options = LinkerOptions(arch="sm_80") | |
| with pytest.raises(ValueError, match="only supports 'nvjitlink' backend"): | |
| options.as_bytes(backend) |
| # ``time`` is a presence gate: the linker emits ``-time`` for any | ||
| # non-None value, so True / "path" produce the same flag. | ||
| pytest.param({"time": True}, {"time": "timing.csv"}, id="time_true_eq_path"), |
There was a problem hiding this comment.
nit: on a cuLink-backend host, _LinkerBackend.validate() rejects non-None time, so this case fails there:
cuda-python/cuda_core/cuda/core/utils/_program_cache/_keys.py
Lines 509 to 522 in f835032
The monkeypatch in this test only pins the version probe, not the backend decision. Pinning the decision too (same idiom as test_make_program_cache_key_ptx_driver_ignored_fields_collapse) keeps the case portable:
from cuda.core import _linker
monkeypatch.setattr(_linker, "_decide_nvjitlink_or_driver", lambda: False) # nvJitLink| def test_make_program_cache_key_accepts_side_effect_options_for_ptx(option_kw): | ||
| """nvJitLink ``-time`` writes only to its info log, not the filesystem.""" | ||
| """The side-effect guard is NVRTC-specific: PTX (linker) and NVVM must | ||
| not be blocked by options whose side effects only apply under NVRTC.""" | ||
| _make_key(code=".version 7.0", code_type="ptx", options=_opts(**option_kw)) # no raise |
There was a problem hiding this comment.
nit: same as the comment above — without the skipif, time_true/time_path hit the driver-unsupported-options rejection on cuLink hosts. Pinning the backend avoids both the failure and the skip:
| def test_make_program_cache_key_accepts_side_effect_options_for_ptx(option_kw): | |
| """nvJitLink ``-time`` writes only to its info log, not the filesystem.""" | |
| """The side-effect guard is NVRTC-specific: PTX (linker) and NVVM must | |
| not be blocked by options whose side effects only apply under NVRTC.""" | |
| _make_key(code=".version 7.0", code_type="ptx", options=_opts(**option_kw)) # no raise | |
| def test_make_program_cache_key_accepts_side_effect_options_for_ptx(option_kw, monkeypatch): | |
| """The side-effect guard is NVRTC-specific: PTX (linker) and NVVM must | |
| not be blocked by options whose side effects only apply under NVRTC.""" | |
| from cuda.core import _linker | |
| monkeypatch.setattr(_linker, "_decide_nvjitlink_or_driver", lambda: False) # nvJitLink | |
| _make_key(code=".version 7.0", code_type="ptx", options=_opts(**option_kw)) # no raise |
2 participants
Summary
Rationale
The backend-selection policy and the cuLink storage-lifetime fix are independent. This change rolls back the compatibility fallback while preserving the memory-safety fix that keeps storage referenced by
CUlinkStatealive untilcuLinkDestroy.User impact
cuda.corewill use nvJitLink whenever it is available and exposes the required version symbol, even when the CUDA driver has a newer major version. When the cuLink backend is used, its option arrays and log buffers remain valid for the full lifetime of the link state.Testing
pixi run --frozen -e cu13 pytest tests/test_linker.py tests/test_optional_dependency_imports.py tests/test_program_cache.py -q(278 passed, 4 skipped)git diff --check upstream/main...HEADPartially reverts #2320.