-
Notifications
You must be signed in to change notification settings - Fork 161
Upgrade TensorRT-LLM docker to 1.1.0RC2 #327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Warning Rate limit exceeded@cjluo-nv has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughUpdated the Docker base image tag in Changes
Sequence Diagram(s)(No sequence diagram provided — changes are configuration and documentation edits without control-flow modifications.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docker/Dockerfile (4)
3-7
: Scope PIP extra index to the commands that need it.TRT‑LLM no longer requires
--extra-index-url
to install, so keepingPIP_EXTRA_INDEX_URL
globally may unintentionally affect laterpip install -r requirements.txt
. Prefer passing the flag only where needed. (nvidia.github.io)Apply:
-ARG PIP_EXTRA_INDEX_URL="https://pypi.nvidia.com" -ENV PIP_EXTRA_INDEX_URL=$PIP_EXTRA_INDEX_URL \ - PIP_NO_CACHE_DIR=off \ +ARG PIP_EXTRA_INDEX_URL="https://pypi.nvidia.com" +ENV PIP_NO_CACHE_DIR=off \ PIP_CONSTRAINT= \ TORCH_CUDA_ARCH_LIST="8.0 8.6 8.7 8.9 9.0 10.0+PTX"Then pass it explicitly where required (see comment on Line 22).
22-23
: Pin ModelOpt to a compatible range to avoid surprise upgrades.Recent TRT‑LLM releases updated the required ModelOpt version; suggest pinning to a compatible minor (e.g.,
~=0.27
) to keep builds reproducible. (nvidia.github.io)-RUN pip install -U "nvidia-modelopt[all,dev-test]" +RUN pip install -U "nvidia-modelopt[all,dev-test]~=0.27" ${PIP_EXTRA_INDEX_URL:+--extra-index-url "$PIP_EXTRA_INDEX_URL"}
9-11
: Minor image‑size hardening.Use
--no-install-recommends
and clean apt cache to reduce layer size.-RUN apt-get update && \ - apt-get install -y libgl1 && \ - rm -rf /var/lib/apt/lists/* +RUN apt-get update && \ + apt-get install -y --no-install-recommends libgl1 && \ + rm -rf /var/lib/apt/lists/* /var/cache/apt/*
34-34
: Avoid 0777; consider a non‑root user.0777 is overly permissive. Prefer creating a user and chowning the workspace, or at least 0775.
Example:
+# Create non-root user +RUN useradd -m -u 1000 -s /bin/bash appuser && \ + mkdir -p /workspace && chown -R appuser:appuser /workspace -RUN chmod -R 777 /workspace +USER appuser
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker/Dockerfile
(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). (3)
- GitHub Check: linux
- GitHub Check: build-docs
- GitHub Check: code-quality
🔇 Additional comments (3)
docker/Dockerfile (3)
15-15
: Symlink source may have changed across base tags.Ensure
/app/tensorrt_llm
exists in1.1.0rc2.post2
; otherwise the symlink breaks. The verification script in Line 1 checks this.
18-19
: Double‑check hard‑coded TRT library/bin paths.Validate
/usr/local/tensorrt/targets/x86_64-linux-gnu/{lib,bin}
exist in the new base; these paths have shifted in some images. The Line 1 script covers this.
1-1
: Confirm tag availability and decide between rc2 vs rc3.
Docker unavailable here (docker: command not found); verification couldn't complete. Confirm nvcr.io/nvidia/tensorrt-llm/release:1.1.0rc2.post2 exists on NGC and decide whether to upgrade to 1.1.0rc3 or keep rc2.
Location: docker/Dockerfile line 1
Signed-off-by: Chenjie Luo <[email protected]>
5c3b19d
to
8751fdc
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
==========================================
- Coverage 73.86% 73.86% -0.01%
==========================================
Files 172 172
Lines 17409 17409
==========================================
- Hits 12860 12859 -1
- Misses 4549 4550 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Chenjie Luo <[email protected]> Signed-off-by: Ye Yu <[email protected]>
What does this PR do?
Upgrade TensorRT-LLM 1.1.0RC2 for the following modelopt release.
Summary by CodeRabbit