Cuda driver improvements/fixes - #833
Conversation
WalkthroughNormalize GPU capability syntax in compose files, add a Vulkan ICD bind-mount, and switch Dockerfile GPU setup from Vulkan/driver installs to installing the CUDA toolkit via the cuda-keyring package; also add wget and curl to base images. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
build/docker-compose.dev.cuda.yml (1)
25-27: Vulkan ICD mount mirrors unresolved concern from related file.This Vulkan ICD mount is identical to the one added in build/docker-compose.carla.cuda.yaml (lines 16–18). Please address the clarification needed there, as it applies here as well.
build/docker/agent-ros2/Dockerfile (1)
35-47: Address the same CUDA keyring hardcoding and error-handling concerns as carla/Dockerfile.This block is identical to build/docker/carla/Dockerfile lines 28–40, which has unresolved concerns:
- Hardcoded
ubuntu2404in URL (line 41); ifBASE_IMAGEchanges, this breaks.- Hardcoded keyring version
1.1-1without version management.- No
wgeterror handling; silent failures may occur.libxext6necessity unclear (see related comment in carla/Dockerfile review).Apply the same solutions and error handling suggested for carla/Dockerfile to maintain consistency and robustness.
🧹 Nitpick comments (1)
build/docker-compose.dev.cuda.yml (1)
24-24: GPU capabilities formatting inconsistency risk.This file applies the same
[ gpu ]formatting as build/docker-compose.carla.cuda.yaml. If this is a linting requirement, ensure the change is also applied to any other Compose files with GPU definitions to maintain uniformity across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build/docker-compose.carla.cuda.yaml(1 hunks)build/docker-compose.dev.cuda.yml(1 hunks)build/docker/agent-ros2/Dockerfile(2 hunks)build/docker/carla/Dockerfile(2 hunks)
🔇 Additional comments (6)
build/docker-compose.carla.cuda.yaml (2)
14-14: Clarify the GPU capabilities formatting change.The change from
[gpu]to[ gpu ]is syntactically valid YAML in both forms. If this formatting is required for tooling compliance, please confirm the linting rule or tool version enforcing it; otherwise, it risks inconsistency if not applied uniformly across all Compose files.
16-18: The review mischaracterizes the technical relationship between Vulkan ICD and CUDA driver isolation.The claim that mounting
/usr/share/vulkan/icd.d/nvidia_icd.json"directly contradicts the isolation goal" is technically incorrect. The Vulkan ICD JSON is a configuration file that names the vendor library; the nvidia-container-toolkit mounts the host driver library so the ICD can point to a working vendor library from the host. CUDA binaries remain from the image and rely on host driver compatibility, while the toolkit ensures driver libraries are visible in the container.This is the standard, supported design for exposing host NVIDIA hardware while keeping CUDA binaries inside the container. There is no contradiction—the Vulkan ICD mount is a legitimate part of the host driver injection strategy.
However, the code comment itself already notes "This should not be necessary" with a reference to the toolkit's GitHub issue, suggesting the modern toolkit may auto-inject this. That's a separate, valid question about redundancy, but it's not an isolation violation.
Likely an incorrect or invalid review comment.
build/docker/carla/Dockerfile (2)
13-13: ✓ LGTM: wget and curl addition supports CUDA keyring download.The tools are appropriately added to support the external CUDA keyring installation that follows.
40-40: Clarify the necessity and scope of libxext6 installation.Line 40 introduces
libxext6(X11 client library), which appears to be a new dependency for the CUDA toolkit setup. Confirm whether this is:
- A required dependency of
cuda-toolkitthat's being explicitly declared for clarity.- A workaround for a missing dependency in the CUDA tooling.
- Intended for GPU graphics rendering support.
Additionally, verify that this library is only installed for the
cudaflavour and not unintentionally pulled into thegpuorrocmbuilds.build/docker/agent-ros2/Dockerfile (2)
17-17: ✓ LGTM: wget and curl addition mirrors carla/Dockerfile.The tools are appropriately added to support the external CUDA keyring installation.
33-33: Verify NVIDIA_REQUIRE_DRIVER constraint against carla/Dockerfile.This file specifies
ENV NVIDIA_REQUIRE_DRIVER="cuda>=12.0 driver>=${NVIDIA_DRIVER_VERSION}"(line 33), whereas carla/Dockerfile specifies onlyENV NVIDIA_REQUIRE_DRIVER="driver>=${NVIDIA_DRIVER_VERSION}"(line 26).Verify that the
cuda>=12.0constraint is intentional and aligns with thecuda-toolkitpackage selected. Ensure consistency or document the difference.
|
@samuel-ibele could you quickly describe what you tested in a comment here? Thank you. |
|
Tested the CUDA driver fixes on several computers with working and non-working setups. |
Description
Makes sure that no cuda driver binaries are installed into the container
-> the binaries from the host have to be used.
Might fix some driver incompatibility issues that started occurring in arlab.
Type of change
Does this PR introduce a breaking change?
No
Draft statusChecklist:
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.