[TRTLLM-14027][infra] Remove --trt_root and stop installing the TensorRT SDK into images - #16608
Conversation
2c77371 to
173f8d9
Compare
📝 WalkthroughWalkthroughThe change removes TensorRT-specific build arguments, installation logic, packaging entries, integration parameters, and compilation documentation. Docker installation now uses CUDA library components without TensorRT version handling. ChangesTensorRT installation removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (1)
docker/common/install.sh (1)
14-14: 🧹 Nitpick | 🔵 TrivialLGTM! The
--tensorrt→--cuda_libsreplacement is consistent across the default, argument parser,--allaggregate, and install conditional.Based on learnings, ensure the
CUBLAS_VERvalue passed through toinstall_cuda_libs.shaligns with the upstream PyTorch container (e.g., cuBLAS 13.2.0.9 for CUDA 13.1 configs), even if the major version appears higher than the CUDA version. Verify the pinned version ininstall_cuda_libs.shand the Docker Makefile build args.Also applies to: 37-40, 55-58, 95-102
🤖 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 `@docker/common/install.sh` at line 14, Align the CUBLAS_VER value propagated to install_cuda_libs.sh with the upstream PyTorch container versions, including cuBLAS 13.2.0.9 for CUDA 13.1 configurations. Verify and update the pinned version in install_cuda_libs.sh and the corresponding Docker Makefile build arguments while preserving the existing cuda_libs option flow.Source: Learnings
🤖 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 @.claude/skills/exec-slurm-compile/scripts/compile.sh:
- Line 22: Synchronize the documented default flags with the actual command:
update the default command in the compile script to pass --no-venv alongside -a
"100-real" and --nvtx, keeping the comment and command consistent.
---
Nitpick comments:
In `@docker/common/install.sh`:
- Line 14: Align the CUBLAS_VER value propagated to install_cuda_libs.sh with
the upstream PyTorch container versions, including cuBLAS 13.2.0.9 for CUDA 13.1
configurations. Verify and update the pinned version in install_cuda_libs.sh and
the corresponding Docker Makefile build arguments while preserving the existing
cuda_libs option flow.
🪄 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: CHILL
Plan: Enterprise
Run ID: a828864f-9272-4ca5-8876-fa176b807eb3
📒 Files selected for processing (10)
.claude/skills/exec-local-compile/SKILL.md.claude/skills/exec-slurm-compile/SKILL.md.claude/skills/exec-slurm-compile/scripts/compile.shdocker/Dockerfile.multidocker/Makefiledocker/common/install.shdocker/common/install_cuda_libs.shscripts/build_wheel.pysetup.pytests/integration/defs/cpp/conftest.py
💤 Files with no reviewable changes (6)
- setup.py
- .claude/skills/exec-slurm-compile/SKILL.md
- tests/integration/defs/cpp/conftest.py
- docker/common/install_cuda_libs.sh
- docker/Makefile
- scripts/build_wheel.py
173f8d9 to
4f186fc
Compare
|
/bot run --stage-list "Build-Docker-Images" --disable-fail-fast |
|
PR_Github #60322 [ run ] triggered by Bot. Commit: |
juney-nvidia
left a comment
juney-nvidia
left a comment
There was a problem hiding this comment.
Approved from OSS compliance perspective
|
PR_Github #60322 [ run ] completed with state
|
|
/bot run --stage-list "Build-Docker-Images" --disable-fail-fast |
|
PR_Github #60495 [ run ] triggered by Bot. Commit: |
|
PR_Github #60495 [ run ] completed with state
|
Adopt the tritondevel images built by PR NVIDIA#16608 CI (build 48820, commit 4f186fc), which no longer install the TensorRT SDK. The images were retagged and pushed manually (timestamp 202607211045); the now-inaccurate trt10.16.1.11 segment is dropped from the canonical tag names since the devel images no longer contain TensorRT. scripts/rename_docker_images.py carries the new prefix forward automatically on future bumps. Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
4f186fc to
f52fedf
Compare
TensorRT is no longer required to build TensorRT-LLM (the legacy TensorRT backend was fully removed; FindTensorRT.cmake and all nvinfer dependencies are gone, and CMake no longer consumes TensorRT_ROOT). The --trt_root flag was kept for one cycle as a documented no-op; remove it now as promised in the PR NVIDIA#16369 review: - scripts/build_wheel.py: drop the argparse flag, the trt_root parameter, and the dead -DTensorRT_ROOT forwarding (previously appended on every build since the default was never None). - tests/integration/defs/cpp/conftest.py: drop the trt_root kwarg from the build_trt_llm() call. - .claude/skills/exec-local-compile, exec-slurm-compile: remove the flag from documented build commands, flag tables, and compile.sh defaults. Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
…ges and wheels TensorRT-LLM no longer links or builds against TensorRT (the legacy engine backend is removed; nvinfer dependencies and FindTensorRT.cmake are gone), but the devel image still downloaded and installed the full TensorRT SDK, and the wheel still shipped a TRT installer script. - docker/common/install_tensorrt.sh -> install_cuda_libs.sh: the script was load-bearing beyond TensorRT - it installs the pinned cuDNN/NCCL/cuBLAS (+NVRTC) stacks - so it is renamed and only the install_tensorrt() function, the TRT_VER pin, and the TRT tarball download are removed. - docker/common/install.sh: the --tensorrt option becomes --cuda_libs and no longer forwards TRT_VER. - docker/Dockerfile.multi: drop ARG TRT_VER and switch the install.sh call to --cuda_libs. - docker/Makefile: drop the TRT_VERSION variable and its build-arg. - scripts/build_wheel.py + setup.py: stop copying install_tensorrt.sh into the package and shipping it as package_data (tensorrt_llm/scripts/ install_tensorrt.sh in the wheel); the build-time copy would also have broken against the renamed source. Stale copies in existing build trees are still cleaned by the pre-existing clear_folder step. NOTE for mergers: this changes devel-image content and therefore requires a devel image rebuild plus a jenkins/current_image_tags.properties bump, coordinated with the docker/infra owners. Existing pinned images keep working (their pre-installed TensorRT is unused but harmless). Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
Adopt the tritondevel images built by PR NVIDIA#16608 CI (build 48820, commit 4f186fc), which no longer install the TensorRT SDK. The images were retagged and pushed manually (timestamp 202607211045); the now-inaccurate trt10.16.1.11 segment is dropped from the canonical tag names since the devel images no longer contain TensorRT. scripts/rename_docker_images.py carries the new prefix forward automatically on future bumps. Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
…SanityCheck The sanity check's final steps invoked the removed TensorRT engine flow (generate_checkpoint_config.py + trtllm-build + examples/run.py). Keep the function (it is currently disabled at its call site) but comment out those steps, with a note to switch to PyTorch backend test samples when the check is re-enabled. Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
f52fedf to
0adadfb
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #60610 [ run ] triggered by Bot. Commit: |
|
PR_Github #60610 [ run ] completed with state
|
|
/bot run --only-multi-gpu-test --disable-fail-fast |
|
PR_Github #60812 [ run ] triggered by Bot. Commit: |
|
PR_Github #60812 [ run ] completed with state |
|
/bot skip --comment "multi-test passed and single-GPU tests are passed as well" |
|
PR_Github #60848 [ skip ] triggered by Bot. Commit: |
|
PR_Github #60848 [ skip ] completed with state |
merge first since the comment is resolved.
…rRT SDK into images (NVIDIA#16608) Signed-off-by: Wanli Jiang <35160485+Wanli-Jiang@users.noreply.github.com>
7 participants
Summary by CodeRabbit
Build & Installation
Documentation
Compatibility
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.