fix(cuda.core): fall back to driver when nvJitLink < 12.3 is installed - #2409
fix(cuda.core): fall back to driver when nvJitLink < 12.3 is installed#2409atiaomar1978-hub wants to merge 6 commits into
Conversation
CI run 29979997352The failing job \PR has assignee, labels, and milestone\ is a metadata gate (missing \�ug\ label, assignee, and milestone). I cannot set these as an external contributor. Updates in latest push
Could a maintainer please add the \�ug\ label, assignee, and milestone so the metadata check passes? |
|
About me: Request: |
f0c5775 to
7f6a534
Compare
Added release notes entryPushed a follow-up commit adding a All commits on this branch are signed and show as verified on GitHub. Current branch
Still blocked only by maintainer metadata (assignee, \�ug\ label, milestone) and the external-contributor workflow approval (\copy-pr-bot). Happy to adjust anything on request. |
|
Hi @rparolin — could you help unblock this PR for #2408? Verified status (as of latest push on
|
| Item | Status |
|---|---|
| Commits | 4ee6ae0 (fix), 7f6a534 (tests), 91a1f4f (1.2.0 release note) — DCO Signed-off-by, GitHub verified signatures |
pre-commit.ci - pr |
Pass |
| Module label on PR | cuda.core (present) |
| PR metadata gate | Fail — missing assignee, bug type label, and milestone on the PR |
| Issue #2408 metadata | Already has bug, milestone cuda.core 1.2.0, and assignee @atiaomar1978-hub (issue assignment does not carry over to the PR) |
I cannot set PR assignee / milestone / some labels as an external contributor. Once those are on the PR, the metadata workflow should re-run on labeled / assigned events.
NVIDIA CI / copy-pr-bot
copy-pr-bot reported that workflows on NVIDIA runners need additional validation before they can run. After metadata is fixed, could you (or another CPR vetter) vet/approve so the full cuda.core test pipeline can run?
Happy to adjust anything in review. Thanks for taking a look.
— Omar (@atiaomar1978-hub)
|
@isVoid Could please review this? Thanks. |
| # Do not probe via module.version(): nvJitLink 12.0-12.2 lacks the unversioned | ||
| # nvJitLinkVersion symbol, so calling version() raises FunctionNotFoundError. | ||
| # Use _nvjitlink_has_version_symbol() below instead (symbol pointer inspection). | ||
| nvjitlink_module = _optional_cuda_import("cuda.bindings.nvjitlink") |
There was a problem hiding this comment.
The general solution looks good: we validate the nvjitlink module by checking the existence of __nvJitLinkVersion to see if it's "too old". _nvjitlink_has_version_symbol under the hood loads the nvjitlink module and inspect the function pointer, in theory this provide as strong validation as module.version, and is safer.
Nit to change: if we lift the probe function to below, this function is now out of the protection scope of try: ... except DynamicLibNotFoundError inside _optional_cuda_import. Imagine a case where the dylib is missing while the binding module exists, this error could surface and we won't be able to gracefully fallback to culink. We should place the version check inside this try except block somehow.
There was a problem hiding this comment.
Thanks @isVoid - addressed in 5478f2d.
The version-symbol check now runs inside _optional_cuda_import's probe (_probe_nvjitlink), so a missing nvJitLink dylib raises DynamicLibNotFoundError and falls back to cuLink. We still avoid module.version() so nvJitLink 12.0-12.2 does not raise FunctionNotFoundError (#2408).
Distinct warnings are preserved:
- binding/dylib unavailable -> not available
- symbol missing (<12.3) -> too old (<12.3)
Also added coverage for the missing-dylib path and a guard that the probe does not call module.version().
Addressed review feedback (@isVoid)Pushed 5478f2d (signed / verified):
Local tests
|
| has_version_symbol = [False] | ||
|
|
||
| def _probe_nvjitlink(module): | ||
| from cuda.bindings._internal import nvjitlink | ||
|
|
||
| has_version_symbol[0] = _nvjitlink_has_version_symbol(nvjitlink) |
There was a problem hiding this comment.
I'm not really a fan of introducing side-effects inside the function. Here I think using an explicit try-except block is probably more readable:
mod = _optional_cuda_import(...)
if mod is None:
...
else:
try:
has_version_symbol = _nvjitlink_has_version_symbol(mod)
except DynamicLibNotFoundError:
...
There was a problem hiding this comment.
Thanks @isVoid — updated in d84fa43.
Dropped the probe side-effect. We now import with _optional_cuda_import(...) (no probe), then use an explicit try/except DynamicLibNotFoundError around _nvjitlink_has_version_symbol, matching the shape you suggested. Missing dylib still falls back to cuLink; nvJitLink <12.3 still gets the “too old” warning without calling module.version().
Stop probing nvJitLink availability via module.version(), which calls the unversioned nvJitLinkVersion symbol missing in nvJitLink 12.0-12.2. Use symbol pointer inspection via _nvjitlink_has_version_symbol() instead, restoring cuda-core 0.6.0 fallback behavior. Fixes NVIDIA#2408 Signed-off-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add regression tests for Linker.which_backend() and _decide_nvjitlink_or_driver() when the nvJitLinkVersion symbol is missing (nvJitLink 12.0-12.2). Related to NVIDIA#2408 Signed-off-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Document the NVIDIA#2408 regression fix in the cuda.core 1.2.0 release notes. Related to NVIDIA#2408 Signed-off-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
… guard Address review feedback: keep the >=12.3 version-symbol check inside _optional_cuda_import's probe so a missing nvJitLink dylib still falls back to cuLink. Continue avoiding module.version(), which raises FunctionNotFoundError on nvJitLink 12.0-12.2 (NVIDIA#2408). Add coverage for missing-dylib fallback and a guard that the probe does not call module.version(). Signed-off-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Address review feedback: drop the probe side-effect and catch DynamicLibNotFoundError around _nvjitlink_has_version_symbol so missing dylibs still fall back to cuLink. Keep avoiding module.version() for nvJitLink <12.3 (NVIDIA#2408). Mark newly added tests with agent_authored authorship markers. Signed-off-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
d84fa43 to
210c7a2
Compare
|
/ok to test 210c7a2 |
isVoid
left a comment
There was a problem hiding this comment.
One more nit on review round trips: let's try not to force push. This rewrites the review history and could make the PR hard to read.
| # Do not call module.version(): nvJitLink 12.0-12.2 lack the unversioned | ||
| # nvJitLinkVersion export, so version() raises FunctionNotFoundError (#2408). | ||
| # Inspect the symbol pointer instead, and catch DynamicLibNotFoundError when | ||
| # the dylib is missing so we can fall back to cuLink. |
There was a problem hiding this comment.
We can probably drop these comments. They simply document a fixed bug.
Signed-off-by: Omar Atie <atiaomar1978-hub@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
atiaomar1978-hub
left a comment
There was a problem hiding this comment.
Thanks for the note on force-pushes — understood. I rebased earlier to sync with main; I will avoid rewriting history on further review rounds and push incremental commits instead.
|
/ok to test e6f3923 |
|
@isVoid I noticed CI is showing three failing jobs. Is there anything I should do on my side, or is this something that needs to be triggered/handled by a maintainer? |
|
No I think these are either one time hiccups or CI breakage that exists on other PRs too. |
|
Summary
Fixes #2408.
When nvJitLink 12.0–12.2 is installed,
Linker.which_backend()andLinkerconstruction raisedFunctionNotFoundError: function nvJitLinkVersion is not foundinstead of warning and falling back to the driver (cuLink) backend.This is a regression introduced in cuda-core 0.7.0. cuda-core 0.6.0 handled this correctly.
Root cause
nvJitLink 12.0–12.2 only export versioned symbols (
__nvJitLink*_12_X); the unversionednvJitLinkVersionexport was added in 12.3.Since cuda-core >= 0.7.0,
_decide_nvjitlink_or_driver()probed availability via:On nvJitLink 12.0–12.2,
module.version()calls the missing unversioned symbol and raisesFunctionNotFoundError._optional_cuda_import()only treatsDynamicLibNotFoundErroras "unavailable", so the exception escapes instead of triggering driver fallback.cuda-core 0.6.0 used
_inspect_function_pointer("__nvJitLinkVersion")(symbol pointer inspection, no function call), which handled this correctly.Fix
Stop probing via
module.version(). Importcuda.bindings.nvjitlinkwithout a probe function and rely on the existing_nvjitlink_has_version_symbol()helper for the >= 12.3 check.Reproduction (before fix)
Result:
FunctionNotFoundError: function nvJitLinkVersion is not foundIndependently confirmed on Windows x86_64 (no NVIDIA GPU) with Python 3.12.10 and
nvidia-nvjitlink-cu12==12.0.140.Expected (and restored) behavior:
Tests added
test_decide_nvjitlink_or_driver_falls_back_when_nvjitlink_too_oldtest_decide_nvjitlink_or_driver_selects_nvjitlink_when_version_symbol_presenttest_which_backend_falls_back_when_nvjitlink_too_old(Linker API regression test for [BUG]: Linker raises FunctionNotFoundError instead of falling back to the driver when nvJitLink < 12.3 is installed #2408)version()probe on importTest plan
cuda_pathfinder/tests/test_optional_cuda_import.pylocally (5 passed)cuda_coretests require NVIDIA driver/GPU locally (nvcuda.dll); CI will run full suitecuda-toolkit[nvjitlink]==12.0.1returns"driver"withRuntimeWarningCI note
The failing
PR has assignee, labels, and milestonecheck is a metadata gate, not a code/test failure. External contributors cannot add the requiredbuglabel, assignee, or milestone. Could a maintainer please add:bugcuda.core 1.2.0(or appropriate release)Signed-off-by: Omar Atie atiaomar1978-hub@users.noreply.github.com