-
Notifications
You must be signed in to change notification settings - Fork 752
fix: Update tensorrt_llm to 1.0.0rc6 #2606
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
WalkthroughDocumentation references to experimental TensorRT-LLM build requirements for MTP/VSWA were removed across trtllm docs. The container build script updated the default experimental TensorRT-LLM commit and default wheel to 1.0.0rc6. The Python optional dependency for trtllm was bumped to tensorrt-llm==1.0.0rc6. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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: 2
🧹 Nitpick comments (1)
container/build.sh (1)
101-103: Defaulting to a pip wheel conflicts with earlier “build from source” guidance and ABI note.Comments above state the default should be option 1 (local wheel/build from source) due to ABI incompatibility between upstream wheels and NGC PyTorch, yet the logic defaults to installing a versioned pip wheel when neither a wheel nor commit is provided. If TENSORRTLLM_INDEX_URL remains the public PyPI (line 98), this default can be brittle.
Two options:
- Keep default = build from source (safer with NGC PyTorch):
-DEFAULT_TENSORRTLLM_PIP_WHEEL="tensorrt-llm==1.0.0rc6" +DEFAULT_TENSORRTLLM_PIP_WHEEL="" +# When empty, logic falls back to commit-based local wheel build.
- Or, if defaulting to a wheel is intended now, update the surrounding comments to explicitly state “default is option 2 (wheel)” and consider switching TENSORRTLLM_INDEX_URL to a vetted internal index.
Also consider following through on the TODO (lines 99–101) to install ai-dynamo[trtllm] in Dockerfile.trtllm so one source of truth drives the version. I can help wire this up in a follow-up.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
components/backends/trtllm/README.md(0 hunks)components/backends/trtllm/deploy/README.md(0 hunks)components/backends/trtllm/gemma3_sliding_window_attention.md(0 hunks)container/build.sh(2 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (3)
- components/backends/trtllm/deploy/README.md
- components/backends/trtllm/README.md
- components/backends/trtllm/gemma3_sliding_window_attention.md
⏰ 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). (4)
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (2)
pyproject.toml (1)
52-52: Bump to tensorrt-llm 1.0.0rc6 — looks good.Aligns with the PR objective to move to rc6. No functional concerns here.
container/build.sh (1)
92-96: Verify DEFAULT_EXPERIMENTAL_TRTLLM_COMMIT — it doesn’t match the DeepGEMM SBSA fix commit cited in the PR.PR description references upstream fix commit 0ff8df95b7ccf0412b32be7befddbec3503115b6 (“[fix] DeepGEMM installation on SBSA”), but DEFAULT_EXPERIMENTAL_TRTLLM_COMMIT is set to a16ba6445c61ed70e7aadfe787d6f316bb422652 (a docs-only change). If users pass --use-default-experimental-tensorrtllm-commit, they won’t pick up the DeepGEMM fix.
If the intent is to default to the DeepGEMM fix, change to the cited commit:
-DEFAULT_EXPERIMENTAL_TRTLLM_COMMIT="a16ba6445c61ed70e7aadfe787d6f316bb422652" +DEFAULT_EXPERIMENTAL_TRTLLM_COMMIT="0ff8df95b7ccf0412b32be7befddbec3503115b6"Please confirm the desired commit/tag for experimental builds. Reference: DeepGEMM SBSA fix commit, and the currently configured commit. (github.com)
nv-kmcgill53
left a 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.
why do we need to remove the MTP sections from the docs? Should I trust coderabbit when it says they are outdated?
indrajit96
left a 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.
LGTM from Multimodal side instructions.
These docs are outdated. MTP support should available to the trtllm version we are using. |
Signed-off-by: Hannah Zhang <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: nnshah1 <[email protected]>
Overview:
There are issues with DeepGEMM on SBSA with VSWA. Upgrading the version can help.
NVIDIA/TensorRT-LLM@0ff8df9
Summary by CodeRabbit