[fix] Let wheels resolve CUDA math libraries from pip packages - #76
[fix] Let wheels resolve CUDA math libraries from pip packages#76behnamasadi wants to merge 5 commits into
Conversation
libcuvslam.so links against cuBLAS, cuSOLVER and cuSPARSE, which are
excluded from the wheel during auditwheel repair and were neither
bundled nor declared anywhere. On a machine without a CUDA Toolkit
installation, `pip install cuvslam-*.whl` succeeds and `import cuvslam`
then fails with:
ImportError: libcusolver.so.11: cannot open shared object file
Declare the missing libraries as cu12/cu13 extras so pip can provide
them, and load them from the nvidia-* pip layout before the extension
module is imported: pip installs them under <site-packages>/nvidia/*/lib,
which the dynamic loader does not search, so declaring them alone is not
enough. When they cannot be resolved at all, the ImportError now names
the two ways to provide them instead of just the missing soname.
The wheel verification script could not catch this because it runs in a
CUDA devel image that provides those libraries system-wide. It now also
installs the wheel with its CUDA extra into a clean environment and
requires that the loaded libraries are the pip-provided ones.
Fixes nvidia-isaac#70
Signed-off-by: behnam.asadi <behnam.asadi@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe package declares CUDA 12 and CUDA 13 extras, preloads pip-provided CUDA math libraries during import, reports installation guidance for missing libraries, and verifies wheel behavior through unit tests and Docker checks. ChangesCUDA dependency resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant cuvslam
participant _cuda_libs
participant CUDA libraries
Installer->>cuvslam: Install wheel with cu12/cu13 extra
cuvslam->>_cuda_libs: preload()
_cuda_libs->>CUDA libraries: Discover and load nvidia-* shared objects
cuvslam->>cuvslam: Import libcuvslam.so
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/test/test_cuda_libs.py`:
- Line 19: Update the tests around the module-level cuvslam import and the
import-side-effect assertion to run import cuvslam plus the /proc/self/maps
check in a fresh subprocess, preventing prior initialization from masking
regressions. Load _cuda_libs.py independently for the helper unit tests, and
keep those tests separate from the subprocess-based import validation.
In `@README.md`:
- Around line 127-133: Update the CUDA dependency instructions in README.md to
clearly state that users need either the matching CUDA Toolkit installation or
the wheel’s matching pip extra. Replace the generic package install commands
with the local-wheel installation form already demonstrated by the existing
example near the referenced lines, using the appropriate cu12 or cu13 extra.
In `@scripts/verify_pycuvslam_wheel_in_docker.sh`:
- Around line 112-116: The CUDA extra installation check in the wheel
verification script must not treat “No matching distribution” as a successful
verification. Remove the broad grep-based exit 0 from the pip failure path so
dependency-resolution failures fail the job; handle intentionally unsupported
architectures only through an explicit, narrowly scoped CI or matrix skip before
the pip install check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8d178e7a-55e6-4e38-85ea-1d57c93518d4
📒 Files selected for processing (7)
README.mdpython/CMakeLists.txtpython/__init__.pypython/_cuda_libs.pypython/pyproject.tomlpython/test/test_cuda_libs.pyscripts/verify_pycuvslam_wheel_in_docker.sh
Do not let a failed extra resolution pass wheel verification: "No matching distribution" is what a broken extras declaration looks like too. Decide from the architecture, before the install, that the nvidia-* math library wheels are x86_64-only, and let every pip failure fail the job. Run the import-time preload regression guard in a subprocess. The test module imported cuvslam at module level, which ran preload() before the assertion, so the guard could not fail. Load _cuda_libs.py standalone for the helper tests. State in README.md that a CUDA Toolkit installation and the wheel's cu12/cu13 extra are alternatives, and drop "pip install cuvslam[cu12]", which resolves against PyPI rather than the downloaded wheel.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/test/test_cuda_libs.py`:
- Around line 108-109: Add a scoped Ruff suppression comment for S603 to the
subprocess.run invocation in the import-side-effect probe, documenting that this
intentional call uses fixed values and preventing the lint failure.
In `@README.md`:
- Around line 124-130: Update the prerequisite documentation so the matching
cu12/cu13 pip-extra option is explicitly limited to x86_64 wheels. For supported
aarch64 Jetson wheels, direct users to JetPack or system-provided CUDA instead,
while retaining the CUDA Toolkit alternative and driver requirement where
applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ca75e284-7095-409d-9760-7379c175d471
📒 Files selected for processing (3)
README.mdpython/test/test_cuda_libs.pyscripts/verify_pycuvslam_wheel_in_docker.sh
The nvidia-* CUDA math library wheels are published for aarch64 as well, so "not published there" was the wrong reason to skip the check. The real one is that the aarch64 wheels are SBSA builds while the aarch64 wheel matrix entries are Jetson, where CUDA comes from JetPack. Say that in the script, and point Jetson users in README.md at JetPack rather than at the extra.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
144-146: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the matching CUDA extra here.
pip install "$(echo cuvslam-*.whl)[cu12]"is hardcoded tocu12, so it won’t work forcu13wheels. Show the tag-specific extra instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 144 - 146, Update the README installation example to use the CUDA extra matching the wheel tag, rather than hardcoding [cu12]. Show the tag-specific form so both cu12 and cu13 wheels are installed with their corresponding extras.scripts/verify_pycuvslam_wheel_in_docker.sh (1)
135-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude nvJitLink in the provenance assertion.
The CUDA extras explicitly install
nvidia-nvjitlink-*, but this check only requirescublas,cusolver, andcusparsepaths. Since the Docker image provides system CUDA libraries, a missing pip-provided nvJitLink could be masked and let verification pass incorrectly. Addnvjitlinkto the checked library set or derive the set frompython/_cuda_libs.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_pycuvslam_wheel_in_docker.sh` around lines 135 - 140, Extend the unresolved-library set in the wheel provenance assertion to include nvjitlink alongside cublas, cusolver, and cusparse, or reuse the authoritative library list from _cuda_libs.py. Ensure missing pip-provided nvJitLink is reported when system CUDA libraries mask its absence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@README.md`:
- Around line 144-146: Update the README installation example to use the CUDA
extra matching the wheel tag, rather than hardcoding [cu12]. Show the
tag-specific form so both cu12 and cu13 wheels are installed with their
corresponding extras.
In `@scripts/verify_pycuvslam_wheel_in_docker.sh`:
- Around line 135-140: Extend the unresolved-library set in the wheel provenance
assertion to include nvjitlink alongside cublas, cusolver, and cusparse, or
reuse the authoritative library list from _cuda_libs.py. Ensure missing
pip-provided nvJitLink is reported when system CUDA libraries mask its absence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0b33ed4d-a808-4ece-b9fb-cc1ffed56f3f
📒 Files selected for processing (2)
README.mdscripts/verify_pycuvslam_wheel_in_docker.sh
|
Both outside-diff findings are valid. Addressed, in one case for a different reason than the one given.
Agreed, and the consequence is worse than "could be masked": it is guaranteed to be masked. The stage runs in Taken the second option and derived the set from from cuvslam import _cuda_libs
nvidia_root = _cuda_libs.nvidia_root()
unresolved = [name for name in _cuda_libs.CUDA_COMPONENTS
if os.path.join(nvidia_root, name, "lib") not in maps]
The line already carried So the fix is not a generic placeholder, which is not copy-pasteable, but both commands with the hazard stated: pip install cuvslam-*.whl
# ...or, without a CUDA Toolkit installation, with the CUDA math libraries from pip. Every wheel carries both
# extras, so pip accepts a mismatched one without an error and installs math libraries of the wrong CUDA major:
# pick the line matching the tag of the wheel downloaded above.
pip install "$(echo cuvslam-*.whl)[cu12]" # cu12 wheels
pip install "$(echo cuvslam-*.whl)[cu13]" # cu13 wheelsVerification: |
Verify the pip provenance of every CUDA component the package declares, not just cuBLAS/cuSOLVER/cuSPARSE. The stage runs in a CUDA devel image, so had nvidia-nvjitlink-* been dropped from the extras, cuSOLVER and cuSPARSE would still have loaded by resolving nvJitLink from the image and the check would have passed for a wheel that fails without a toolkit. Deriving the set from _cuda_libs.CUDA_COMPONENTS covers future components without a second edit here, and resolving nvidia_root() the way preload() does asserts against the path production actually uses. Show both install extras in README.md. Every wheel declares both, since a wheel cannot vary its metadata by CUDA major, so a mismatched extra is not an error and installs math libraries of the wrong major silently.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 144-148: Update the README installation example so only the CUDA
extra matching the downloaded wheel tag is executable: comment out the
alternative cu12/cu13 command or replace both with one command that derives the
extra from the wheel tag. Preserve the guidance to select the matching CUDA
major and prevent copying the block from installing both extras.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 49ee8145-6b10-4800-8157-d4bb90f89b84
📒 Files selected for processing (2)
README.mdscripts/verify_pycuvslam_wheel_in_docker.sh
Two active pip commands, one per CUDA major, meant copying the block whole installed both extras and so both sets of math libraries. Reading the extra off the wheel's own +cu12/+cu13 tag leaves one runnable command that cannot be mismatched, using the same idiom the wheel verification script already uses to pick the extra.
|
Valid, and it was a regression I introduced in the previous round: two active Fixed in 087b1b1, but not by commenting both out. That leaves nothing runnable and hands the reader back the same pip install cuvslam-*.whl
# ...or, without a CUDA Toolkit installation, with the CUDA math libraries from pip. The extra has to match the
# wheel's CUDA major: every wheel declares both cu12 and cu13, so pip accepts a mismatched one without an error
# and installs math libraries of the wrong major. Reading the extra off the wheel's own +cu12/+cu13 tag rather
# than typing it keeps the two in step:
wheel=$(echo cuvslam-*.whl)
pip install "$wheel[$(echo "$wheel" | grep -oE '\+cu[0-9]+' | tr -d '+')]"One runnable command, no way to mismatch it, and it is the same idiom Both remaining lines are safe to copy together: the second is a superset of the first, so running both installs the Verification: run against |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 148-149: Update the README installation command around wheel
discovery to require exactly one matching cuvslam wheel before deriving the CUDA
extra. Reject both no-match cases, including the literal glob, and multiple
matches; only then extract the CUDA suffix and pass the single wheel with its
extra to pip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f27807e8-553b-478f-aed9-201c84991a5f
📒 Files selected for processing (1)
README.md
| wheel=$(echo cuvslam-*.whl) | ||
| pip install "$wheel[$(echo "$wheel" | grep -oE '\+cu[0-9]+' | tr -d '+')]" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant README section with line numbers.
sed -n '136,160p' README.md | cat -n
echo
echo '--- shell behavior probe ---'
tmpdir="$(mktemp -d)"
cd "$tmpdir"
# Case 1: no matches
printf 'case 1 (no matches)\n'
shopt -u nullglob
wheel=$(echo cuvslam-*.whl)
printf 'wheel=<%s>\n' "$wheel"
printf 'tags=<%s>\n' "$(grep -oE '\+cu[0-9]+' <<<"$wheel" | tr -d '+')"
# Case 2: one match
printf '\ncase 2 (one match)\n'
touch 'cuvslam-a+cu12.whl'
wheel=$(echo cuvslam-*.whl)
printf 'wheel=<%s>\n' "$wheel"
printf 'tags=<%s>\n' "$(grep -oE '\+cu[0-9]+' <<<"$wheel" | tr -d '+')"
printf 'pip arg=<%s>\n' "$wheel[$(grep -oE '\+cu[0-9]+' <<<"$wheel" | tr -d '+')]"
# Case 3: two matches
printf '\ncase 3 (two matches)\n'
touch 'cuvslam-b+cu13.whl'
wheel=$(echo cuvslam-*.whl)
printf 'wheel=<%s>\n' "$wheel"
printf 'tags raw:\n'
grep -oE '\+cu[0-9]+' <<<"$wheel" | cat -n
printf 'tags stripped:\n'
grep -oE '\+cu[0-9]+' <<<"$wheel" | tr -d '+' | cat -n
printf 'pip arg=<%s>\n' "$wheel[$(grep -oE '\+cu[0-9]+' <<<"$wheel" | tr -d '+')]"Repository: nvidia-isaac/cuVSLAM
Length of output: 1787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '140,155p' README.md | cat -nRepository: nvidia-isaac/cuVSLAM
Length of output: 946
Require a single wheel match before deriving the CUDA extra. wheel=$(echo cuvslam-*.whl) can expand to the literal glob when nothing matches, or to multiple filenames when more than one wheel is present; in the latter case the grep output is concatenated into an invalid pip install argument. Guard for exactly one wheel before building the extra.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 148 - 149, Update the README installation command
around wheel discovery to require exactly one matching cuvslam wheel before
deriving the CUDA extra. Reject both no-match cases, including the literal glob,
and multiple matches; only then extract the CUDA suffix and pass the single
wheel with its extra to pip.
|
I think this PR needs some human review. @slepichev @vikuznetsov-nvidia @hrabeti-nvidia |
1 participant
Fixes #70.
Wheels link against cuBLAS/cuSOLVER/cuSPARSE, exclude them during
auditwheel repair, and neither bundle nor declarethem anywhere, so on a machine without a CUDA Toolkit installation
pip install cuvslam-*.whlsucceeds andimport cuvslamfails withImportError: libcusolver.so.11: cannot open shared object file.What this does
1. Declares the excluded libraries as extras (
python/pyproject.toml)Extras rather than hard dependencies, so the documented CUDA Toolkit prerequisite stays the default and the install
size does not change for existing users —
pip install "cuvslam[cu12]"is opt-in for environments without a toolkit.Happy to make them unconditional dependencies instead if you would rather the wheel be self-contained; that is a
one-line change.
Both extras ship in every wheel, since a wheel cannot vary its metadata by the CUDA major it was built for. From
CUDA 13 on, NVIDIA publishes these packages unsuffixed (
nvidia-cublas13.x, the-cu13names on PyPI are 0.0.1placeholders), hence the different naming and the major-version bounds in the
cu13list.2. Makes the declaration actually work (
python/_cuda_libs.py,python/__init__.py)Declaring the packages is not sufficient on its own: pip installs them into
<site-packages>/nvidia/<component>/lib,which the dynamic loader does not search, and
auditwheel repairoverwrites the RPATH with$ORIGIN/../cuvslam.libs,so an RPATH set at build time would not survive either.
_cuda_libs.preload()therefore loads them by absolute pathwith
RTLD_GLOBALbefore the extension module is imported, which registers their sonames solibcuvslam.soresolvesagainst them — no
LD_LIBRARY_PATHneeded. It is a no-op when thenvidia-*packages are not installed, so a systemCUDA Toolkit keeps working exactly as before.
When the libraries cannot be resolved at all, the
ImportErrornow names both ways to provide them instead of onlythe missing soname:
3. Closes the CI gap (
scripts/verify_pycuvslam_wheel_in_docker.sh)The existing wheel check runs in
cuvslam:local, a CUDA devel image that provides those libraries system-wide, so itcannot distinguish a wheel that declares its CUDA dependencies from one that silently relies on the toolkit being
present. The script now adds a second stage: install the built wheel with the extra matching its
+cu12/+cu13version tag into a clean venv, import it, and require via
/proc/self/mapsthat cuBLAS/cuSOLVER/cuSPARSE were loadedfrom
<site-packages>/nvidia/...rather than from the image. No workflow changes —nightly.ymlalready calls thisscript, so it picks the stage up automatically.
The stage runs on x86_64 only. The
nvidia-*math library wheels are not published for aarch64, where the librariescome from JetPack instead, so the script decides that from
uname -mbefore installing anything and printsSKIPPED:with the reason. Deciding it up front rather than from how pip fails matters: "No matching distribution" is also what a
broken extras declaration looks like, so on x86_64 every resolution failure fails the job.
One note on cost: the stage downloads roughly 700 MB of
nvidia-*wheels per x86_64 wheel-building matrix entry. Easyto drop if you would rather keep nightly lean.
4. Documents it (
README.md) — the install-from-wheels section now states that a CUDA Toolkit installation and thewheel's matching
cu12/cu13extra are the two alternatives for providing the CUDA math libraries, and shows theextra applied to the downloaded wheel file.
Tests
python/test/test_cuda_libs.py(5 tests) covers discovery order, ignoring unrelated components in thenvidianamespace, the no-pip-packages no-op path, termination when libraries are unloadable, and a regression guard that the
CUDA math libraries are in fact loaded once
cuvslamimports.The four helper tests load
_cuda_libs.pystandalone viaimportlibrather than importingcuvslam, and theregression guard runs
import cuvslamplus the/proc/self/mapscheck in a subprocess, so acuvslamalreadyimported by another test cannot mask a regression in import-time preloading.
Verification
Verified against the released v17.0.0 cu12 wheel on a machine with no CUDA math libraries installed at all
(
ldconfig -p | grep -c "libcusolver\|libcublas"→0), Ubuntu 24.04, Python 3.12, RTX 3090:ImportError: libcusolver.so.11: cannot open shared object file— the original report.__init__.py/_cuda_libs.pyin place andpip install nvidia-{cublas,cusolver,cusparse,nvjitlink}-cu12:import cuvslamsucceeds,get_version()→17.0.0+57f42cc, andwarm_up_gpu()runs, so the pip-providedlibraries are usable and not merely loadable.
/proc/self/mapsconfirms cuBLAS, cuBLASLt, cuSOLVER, cuSPARSE and nvJitLink all resolve fromsite-packages/nvidia/*/lib, withlibcudartstill coming from the vendoredcuvslam.libs/.nvidia-*packages absent, the newImportErrormessage is produced.test_map/test_trackingwere not run, theyneed
scipy, which was not installed in the throwaway venv).pip install "<wheel>[cu12]"was exercised against a wheel with the new metadata to confirm the extras resolvethrough a direct wheel path, which is the form the CI stage uses.
pre-commit run --files ...passes on all changed files.The full nightly path (build wheel → new verify stage) has not been run here; it needs the
cuvslam:localimage and aGPU builder.
The review round on top of that was verified separately: the four helper tests pass against a stub
cuvslampackage onPYTHONPATHwhose__init__.pyraises, which is what confirms they no longer import the package, and the subprocessguard fails on that stub with the child's traceback surfaced.
bash -nis clean on the verify script.Summary by CodeRabbit
New Features
Documentation
Tests