[TRTLLMINF-419][infra] Clean up the devel image build - #19152
EmmaQiaoCh wants to merge 7 commits into
Conversation
…TC dynamically The DLFW base image ships NVRTC at the version we ask for but without libnvrtc_static.a, so the version guard in install_cuda_libs.sh skipped the reinstall and left the CUDA::nvrtc_static target missing. Every build that hit that was then taught to link NVRTC dynamically instead. Reinstall cuda-nvrtc-dev unconditionally so the static library is present, and drop the three workarounds that existed only because it was not: - docker/Makefile no longer injects NVRTC_DYNAMIC_LINKING=ON into BUILD_WHEEL_ARGS - jenkins/Build.groovy's micro-benchmark build no longer passes it - the cpp test fixtures no longer request nvrtc_dynamic_linking build_wheel.py still forces dynamic linking when BOLT is enabled; that is an independent requirement (static NVIDIA libraries lack --emit-relocs) and is left alone. Also stop naming specific releases in the pin comments, so a version bump only has to touch the variables. Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
…ges too install_cuda_toolkit.sh only handled Rocky Linux, so the Ubuntu CI image kept whatever CUDA its public base tag carried while the libraries moved on. That is why the SBSA Ubuntu 24.04 image build was disabled. Add the Ubuntu path and re-enable that build. Two details it needs: - the aarch64 run file has an _sbsa suffix, and the Ubuntu 24.04 image is built for both architectures, so pick the file name from uname -m - the DLFW image is Ubuntu based as well, and its CUDA_DRIVER_VERSION is an internal build that is not published, so the Rocky guard would fail to match and wipe a perfectly good toolkit. Match on the toolkit version alone, which is all the run file replaces. The purge takes cuda-keyring with it; install_cuda_libs.sh runs next and puts it back along with cuDNN, NCCL, cuBLAS and NVRTC. The comments describe the situation rather than this particular release: a DLFW bump regularly lands before nvcr.io publishes a matching rockylinux/ubuntu tag, and only the version variables should need editing when it does. Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
It was added while chasing a singleton MPI_Comm_spawn failure, but it never fixed one: setting it inside a CI pod left the spawn failing identically, and the same image on bare metal spawns fine without it. All it changes is PRRTE's own install-dir lookup, which shows up as `prte --version` failing to find its help files. Open MPI itself is unaffected -- the base image already exports OPAL_PREFIX and puts /usr/local/mpi/bin on PATH. The variable is not set anywhere else, so this restores the previous behaviour exactly. PMIX_HOSTNAME, which is what actually fixes the spawn, is untouched. Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
… image Six packages were pinned in both requirements.txt and constraints.txt. Because setup.py appends constraints.txt to the wheel's install_requires, the wheel carried two specifiers for the same package and pip silently intersected them. protobuf is where that went wrong: requirements.txt asked for >=5.27.2 with a comment about etcd-sdk-python, while the binding constraint was the CUTLASS DSL libraries needing 6.x. Reading either file alone gave the wrong answer. Keep each pin in one place. Runtime dependencies live in requirements.txt; constraints.txt keeps the base-image vulnerability fixes, which are not TensorRT-LLM dependencies and have nowhere else to go. starlette stays there even though requirements.txt also lists it, because the entry is the GHSA-82w8-qh3p-5jfq workaround. requirements.txt now states the real protobuf bound, so the wheel metadata is unchanged. This also changes what the devel image contains, which is the point: Dockerfile.multi installs constraints.txt with `pip3 install -r`, not `-c`, so every line was a package installed into the image. quack-kernels and torch-c-dlpack-ext declare an unbounded `torch` dependency, which is how a stray public torch and its matching triton ended up preinstalled in images built on a base without torch -- a version that satisfies nothing we ask for and drifts with whatever PyPI publishes that day. install_fa4.sh goes for the same reason. flash-attn-4 has been a requirements.txt entry since it stopped being vendored, the package is a 0.4 MB pure-Python wheel so there is nothing to prebuild, and every consumer already resolves it: the release image installs the wheel with its dependencies, and CI installs requirements-dev.txt, whose first line includes requirements.txt. Installing it separately only duplicated the version and, because that pip invocation saw flash-attn-4's unbounded dependency ranges on their own, would have pulled the just-removed packages back in and downgraded the pinned CUTLASS DSL to the latest non-prerelease. The base image's numpy is still uninstalled, and now nothing puts it back while the image is built, so the released devel image no longer carries a numpy old enough to trip a vulnerability scan. Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
|
/bot run --stage-list "Build-Docker-Images" --disable-fail-fast |
|
PR_Github #73258 [ run ] triggered by Bot. Commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe change removes obsolete runtime and FlashAttention setup, updates protobuf constraints, changes CUDA installation for Ubuntu and Rocky Linux, removes NVRTC linking overrides, and re-enables the Ubuntu SBSA CI image. ChangesCUDA image updates
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant DockerBuild
participant install_cuda_toolkit.sh
participant UbuntuApt
participant CUDARunfile
participant install_cuda_libs.sh
DockerBuild->>install_cuda_toolkit.sh: configure CUDA installation
install_cuda_toolkit.sh->>install_cuda_toolkit.sh: check_cuda_toolkit_version
install_cuda_toolkit.sh->>UbuntuApt: purge existing CUDA packages
install_cuda_toolkit.sh->>CUDARunfile: install the configured toolkit
install_cuda_libs.sh->>UbuntuApt: reinstall cuda-nvrtc-dev
install_cuda_libs.sh->>UbuntuApt: install cuda-compat when libcuda.so.1 is absent
Merge Risk: ⚪ Minimal · up to The updated CUDA installation supports the re-enabled SBSA image, with no concrete merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
The run-file CUDA reinstall purges cuda-compat, and --toolkit does not bring it back, leaving no libcuda.so.1 under /usr/local for the NIXL build to find. Restore it from the public CUDA repo, as the RockyLinux path already does. Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
|
/bot run --stage-list "Build-Docker-Images" --disable-fail-fast |
|
PR_Github #73258 [ run ] completed with state
|
|
PR_Github #73277 [ run ] triggered by Bot. Commit: |
|
PR_Github #73277 [ run ] completed with state
|
|
/bot run --stage-list "Build-Docker-Images" --disable-fail-fast |
|
PR_Github #73421 [ run ] triggered by Bot. Commit: |
Pick up the images rebuilt for this branch. The SBSA Ubuntu 24.04 wheel image is left on its old tag until its build succeeds. Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
8b7cfd1 to
c06c12a
Compare
|
/bot run |
|
/bot run |
|
PR_Github #73466 [ run ] triggered by Bot. Commit: |
|
PR_Github #73421 [ run ] completed with state |
|
PR_Github #73466 [ run ] completed with state
|
The four *_ALLOW_RUN_AS_ROOT variables were set on the tritondevel stage only, so the release image shipped without them. The NIXL bindings install_nixl.sh builds were unreachable for a different reason: ninja installs them outside site-packages under the name nixl_cu13, while consumers import nixl. Anyone using ModelExpress therefore had to pull the backend from PyPI, whose bundled UCX segfaults alongside the one torch already links -- the crash L0_Test.groovy had been working around per stage. Move the variables to the devel stage, install the dispatching shim, and export PYTHONPATH next to the LD_LIBRARY_PATH the script already writes. The per-stage workarounds are dropped; the bare-metal sanity stages keep theirs because they run on a stock image that never saw Dockerfile.multi. Requires an image rebuild: without one the ModelExpress stages fail to import nixl and test_mpi_session's spawn dies in PRRTE. Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
|
/bot run --stage-list "Build-Docker-Images" --disable-fail-fast |
|
PR_Github #73561 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #73603 [ run ] triggered by Bot. Commit: |
|
PR_Github #73561 [ run ] completed with state |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
A few things to settle before merge.
Description. Both the Description and Test Coverage sections are empty, and the auto-generated text contradicts the diff — it says the SBSA Ubuntu 24.04 wheel image "keeps its prior tag until its build succeeds", but [jenkins/current_image_tags.properties:20](https://github.com/NVIDIA/TensorRT-LLM/pull/19152/files#diff-5b8a6aba346304942b78d7fbd55b59dd3d1783ad8fed38b6c4fc447cd7d5ac97R20) bumps it to cuda-13.4.1-devel-ubuntu24.04-…-19152. It also doesn't mention the changes to docker/common/install_nixl.sh, jenkins/L0_Test.groovy, or the MPI env move in Dockerfile.multi. Please write the human sections; this PR changes published-image content and a user-facing dependency floor, so a future reader needs more than a file list.
User-visible changes worth a doc/release note. protobuf>=5.27.2 → >=6.30.2,<7 in requirements.txt drops protobuf 5.x for everyone installing the wheel, not just CI. And the MPI root-execution env now lands in the release/tritonrelease/NGC images (see inline). Neither is called out anywhere.
Verification. The image-tag bump proves the image builds ran, but several behavior changes here are only exercised by L0: the removal of NVRTC_DYNAMIC_LINKING=ON from docker/Makefile, jenkins/Build.groovy, and tests/integration/defs/cpp/conftest.py depends entirely on the new unconditional cuda-nvrtc-dev reinstall actually delivering libnvrtc_static.a in every image, and the -ModelExpress- stages now depend on the image's PYTHONPATH rather than withEnv. Please confirm a full pipeline (including the C++ build stages and a ModelExpress stage) ran green on the images this PR pins, and say so in the PR.
Confirmed clean: no dangling references to the deleted install_fa4.sh or to PRTE_PREFIX remain, and flash-attn-4 / the CuTe DSL stack are still declared in requirements.txt, so the wheel and the test-time requirements-dev.txt install still bring them in.
| # Remove any leftover namespace dirs or dist-info that pip missed | ||
| rm -rf $(python3 -c "import site; print(site.getsitepackages()[0])")/nvidia_cutlass_dsl* && \ | ||
| pip3 install -r /tmp/constraints.txt "nvidia-cutlass-dsl[cu13]==4.8.0.dev0" && \ | ||
| pip3 install -r /tmp/constraints.txt && \ |
There was a problem hiding this comment.
Removing the pins from constraints.txt leaves the pip3 uninstall on line 76 uncompensated for numpy. Previously constraints.txt carried numpy>=2.0.0,<2.4, so this line put numpy back; now nothing does, and the devel stage ends with torch/scipy/pandas installed but numpy purged. That stage is a published artifact (ngc-devel_%: STAGE = devel in docker/Makefile), so the public devel image ships a broken environment. CI and the wheel stage are unaffected (both install requirements-dev.txt, which pulls requirements.txt), which is why this wouldn't show up in a green pipeline.
If the uninstall existed only so the wheel's pin could win at install time, drop numpy (and, if the same reasoning applies, the nvidia-cutlass-dsl* entries) from the uninstall list on line 76. If the devel image is intentionally dependency-free now, say so in a comment here.
| # usually not published, so the Rocky check above would miss and wipe a good toolkit. Match | ||
| # on the toolkit version alone: that is all the run file replaces. | ||
| check_cuda_toolkit_version() { | ||
| if [ -n "$CUDA_VERSION" ] && [ "$(echo "$CUDA_VERSION" | cut -d'.' -f1-3)" = "$CUDA_VER" ]; then |
There was a problem hiding this comment.
The guard is exact equality, so a DLFW image newer than CUDA_VER doesn't skip — it takes the Ubuntu reinstall path and downgrades a good toolkit. That was harmless before this PR (Ubuntu never reached reinstall_*), but it is now destructive on exactly the case the file header calls a no-op: "the checks below skip the reinstall once the base image already ships it". A >=-style comparison, or gating the Ubuntu path on an explicit signal from the Makefile targets that need it, would match the stated intent.
Related: the check reads the CUDA_VERSION env var, which the run-file install doesn't update. An image built through reinstall_ubuntu_cuda still reports CUDA_VERSION=13.3.1 while carrying 13.4.1, so re-running this script against that image purges and reinstalls again, and anything downstream reading CUDA_VERSION gets the wrong answer. Worth exporting the new version at the end of the reinstall.
| TRT_LLM_VERSION=${TRT_LLM_VER} | ||
|
|
||
| # Allow MPI/PRTE to run as root inside the container | ||
| ENV OMPI_ALLOW_RUN_AS_ROOT=1 \ |
There was a problem hiding this comment.
Moving this block from tritondevel up to devel changes what ships publicly: release (and therefore tritonrelease and the NGC release images) is FROM ${DEVEL_IMAGE}, so released containers now default to permitting MPI/PRTE to run as root. That's a behavior change for users, not just CI. If it's intended, it belongs in the PR description and the container docs; if only the CI images need it, keeping it in tritondevel and adding it to the CI image path preserves the old release-image default.
| // sees, so the pod keeps its own for logging and for port sectioning in | ||
| // getHostNodeName(). Kept out of runLLMTestlistOnPlatformImpl: the SLURM | ||
| // path runs that on the compute node, where one shared name would break | ||
| // locality. The ALLOW_RUN_AS_ROOT pair now comes from the image |
There was a problem hiding this comment.
The PYTHONPATH that -ModelExpress- stages needed moves from a Jenkins withEnv to an export in the image's ${ENV} shell-init file (install_nixl.sh). Those propagate differently: withEnv applies to every process the step launches, while the init-file export only reaches shells that actually source it (bash via BASH_ENV, not a plain sh -e step or a direct exec). If any step in the ModelExpress path doesn't go through bash, import nixl._api silently resolves to the pip shim without the image-built backend.
The image-tag bump shows the image builds succeeded; it doesn't show L0 did. Can you confirm a ModelExpress stage passed on the pipeline that produced the 202609132200-19152 images?
|
|
||
| echo "export LD_LIBRARY_PATH=/opt/nvidia/nvda_nixl/lib/${ARCH_NAME}:/opt/nvidia/nvda_nixl/lib64:\$LD_LIBRARY_PATH" >> "${ENV}" | ||
| # ninja installs the bindings outside site-packages, so the shim needs PYTHONPATH. | ||
| echo "export PYTHONPATH=/opt/nvidia/nvda_nixl/lib/python3/dist-packages\${PYTHONPATH:+:\$PYTHONPATH}" >> "${ENV}" |
There was a problem hiding this comment.
Exporting this globally (not just for the ModelExpress stages, as the removed L0_Test.groovy block did) means the image-built bindings now shadow the pip-installed nixl-cu13 in every stage. NIXL_VERSION here (v1.4.0) and requirements-dev.txt:68 (nixl-cu13==1.4.0) agree today, but they'll now skew silently if either moves — the pip package will be installed and simply never used. Worth a comment pointing at the other pin, or deriving one from the other.
| @@ -1,10 +0,0 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Removal of this file approved from my end; the FA2/FA4 coexistence fix this originated from was fixed in 26.06 release: https://gitlab-master.nvidia.com/dl/dgx/pytorch_3rd_party/-/merge_requests/8
4 participants
Dev Engineer Review
cuda-compatwhenlibcuda.so.1is absent.requirements.txt. FlashAttention 4 installation and redundant constraints are removed.PRTE_PREFIXconfiguration is removed.QA Engineer Review
tests/integration/defs/cpp/conftest.pyremoves explicit dynamic NVRTC linking from both C++ fixtures. No test-list files changed, so the fixtures are not newly listed intest-db/orqa/. Verify C++ integration builds with static NVRTC on supported architectures. Coverage: needs follow-up.Per-File QA Perspective
constraints.txt: Verify runtime dependency resolution after pins move torequirements.txt.docker/Dockerfile.multi: Verify builds without the obsolete constraints installation and FlashAttention 4 setup.docker/Makefile: Verify wheel builds without forcedNVRTC_DYNAMIC_LINKING=ON.docker/common/install_base.sh: Verify MPI and PRTE discovery withoutPRTE_PREFIX.docker/common/install_cuda_libs.sh: Verifycuda-compat,cuda-nvrtc-dev, andlibcuda.so.1availability.docker/common/install_cuda_toolkit.sh: Verify Ubuntu and Rocky Linux installation paths for x86_64 and aarch64.docker/common/install_fa4.sh: Verify no build or runtime step requires the removed installer.jenkins/Build.groovy: Verify x86_64 debug wheel builds without dynamic NVRTC linking.jenkins/BuildDockerImage.groovy: Verify the SBSA Ubuntu 24.04 stage, arm64 target, Python version, and tag behavior.requirements.txt: Verify protobuf resolution with>=6.30.2,<7.security_scanning/pyproject.toml: Verify security-scan dependency resolution with the updated protobuf range.tests/integration/defs/cpp/conftest.py: Verify both C++ fixture configurations and static NVRTC integration coverage.jenkins/current_image_tags.properties: Verify CI uses rebuilt image tags and preserves the pending SBSA Ubuntu 24.04 wheel tag.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.