chore: Fix cuda lock in trtllm dockerfile#3684
Conversation
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
WalkthroughAdds a version constraint for cuda-python (>=12, <13) to the Dockerfile.trtllm before TensorRT-LLM installation in both build paths. This pins cuda-python to ensure compatibility with tensorrt-llm 1.0.0rc6 without modifying existing conditional logic or error handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (1)
container/Dockerfile.trtllm (1)
198-199: Optional: Consider consolidating RUN commands for Docker build efficiency.The cuda-python installation is currently a separate RUN layer. If docker build layer caching is a concern, consider merging it with the subsequent large RUN block (lines 201–235) to reduce the final image size and rebuild time:
-# NOTE: locking cuda-python version to <13 to avoid breaks with tensorrt-llm 1.0.0rc6. -RUN uv pip install "cuda-python>=12,<13" - # Note: TensorRT needs to be uninstalled before installing the TRTLLM wheel # because there might be mismatched versions of TensorRT between the NGC PyTorch # and the TRTLLM wheel. RUN [ -f /etc/pip/constraint.txt ] && : > /etc/pip/constraint.txt || true && \ # Clean up any existing conflicting CUDA repository configurations and GPG keys rm -f /etc/apt/sources.list.d/cuda*.list && \ rm -f /usr/share/keyrings/cuda-archive-keyring.gpg && \ rm -f /etc/apt/trusted.gpg.d/cuda*.gpg && \ + # Install cuda-python with version lock to avoid breaks with tensorrt-llm 1.0.0rc6 + uv pip install "cuda-python>=12,<13" && \ if [ "$HAS_TRTLLM_CONTEXT" = "1" ]; then \This is optional and depends on your layer-caching strategy, but keeping related pip installs together can improve build performance and readability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
container/Dockerfile.trtllm(1 hunks)
⏰ 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). (6)
- GitHub Check: trtllm (amd64)
- GitHub Check: vllm (amd64)
- GitHub Check: vllm (arm64)
- GitHub Check: sglang
- GitHub Check: trtllm (arm64)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
container/Dockerfile.trtllm (2)
198-199: Clear and well-placed cuda-python version lock.The constraint
>=12,<13aligns with the CUDA 12.9.1 runtime image (line 9) and is positioned correctly before TensorRT-LLM installation. The comment adequately explains the motivation (compatibility with tensorrt-llm 1.0.0rc6).
198-235: Verify that unconditional cuda-python installation is intentional.Line 199 installs cuda-python outside the conditional block (line 209), meaning it executes regardless of whether
HAS_TRTLLM_CONTEXTis set to "1" or "0". If TensorRT-LLM installation is optional based on this flag, confirm whether cuda-python should also be conditional. If it's required by other dependencies in both branches (lines 217–224 and 233–234), this unconditional placement is correct.
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Overview:
Fix cuda lock in trtllm dockerfile, by bringing back initial lock
Details:
Add
pip install "cuda-python>=12,<13"Where should the reviewer start?
container/Dockerfile.trtllm
Summary by CodeRabbit