[None][fix] Remove the wheel from intermediate docker storage #7175
Conversation
Signed-off-by: Martin Marciniszyn Mehringer <[email protected]>
📝 WalkthroughWalkthroughUpdates Docker release image stage to install the built wheel via BuildKit bind-mount from the wheel stage instead of copying, removing manual cleanup. Adds IMAGE_NAME property to Jenkins image tags configuration; existing tags unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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. ✨ Finishing Touches🧪 Generate unit tests
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
|
|
/bot run --skip-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docker/Dockerfile.multi (1)
134-135: Make the mount read-only and harden pip install to avoid accidental network hits.Two small robustness tweaks:
- Add
,roto the bind mount to prevent accidental writes to the mounted build directory.- Consider
--no-indexsopipnever reaches out to the network in the release stage.Apply this minimal diff:
-RUN --mount=type=cache,target=/root/.cache/pip --mount=type=bind,from=wheel,source=/src/tensorrt_llm/build,target=/tmp/wheel \ - pip install /tmp/wheel/tensorrt_llm*.whl +RUN --mount=type=cache,target=/root/.cache/pip \ + --mount=type=bind,from=wheel,source=/src/tensorrt_llm/build,target=/tmp/wheel,ro \ + pip install --no-index /tmp/wheel/tensorrt_llm*.whl
📜 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 (2)
docker/Dockerfile.multi(1 hunks)jenkins/current_image_tags.properties(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-20T15:04:42.848Z
Learnt from: dbari
PR: NVIDIA/TensorRT-LLM#7095
File: docker/Dockerfile.multi:168-168
Timestamp: 2025-08-20T15:04:42.848Z
Learning: In docker/Dockerfile.multi, wildcard COPY for benchmarks (${CPP_BUILD_DIR}/benchmarks/*Benchmark) is intentionally used instead of directory copy because the benchmarks directory contains various other build artifacts during C++ builds, and only specific benchmark executables should be copied to the final image.
Applied to files:
docker/Dockerfile.multi
📚 Learning: 2025-08-01T15:14:45.673Z
Learnt from: yibinl-nvidia
PR: NVIDIA/TensorRT-LLM#6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.
Applied to files:
docker/Dockerfile.multi
📚 Learning: 2025-08-21T00:16:56.427Z
Learnt from: farshadghodsian
PR: NVIDIA/TensorRT-LLM#7101
File: docs/source/blogs/tech_blog/blog9_Deploying_GPT_OSS_on_TRTLLM.md:36-36
Timestamp: 2025-08-21T00:16:56.427Z
Learning: TensorRT-LLM container release tags in documentation should only reference published NGC container images. The README badge version may be ahead of the actual published container versions.
Applied to files:
jenkins/current_image_tags.properties
⏰ 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). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (3)
docker/Dockerfile.multi (1)
134-135: Nice swap to BuildKit bind-mount; trims image layers and avoids wheel cleanup.This RUN with a mount from the wheel stage installs the wheel without copying artifacts into the final image. Good use of cache mounts for pip as well.
jenkins/current_image_tags.properties (2)
14-14: Adding IMAGE_NAME looks good and aligns with existing tags.The value matches the registry prefix used by the downstream
LLM_*_DOCKER_IMAGEtags, and keeps the file sh-parseable as required by the header notes.
14-14: Downstream IMAGE_NAME usage verified; no drift riskAll references to Docker image names have been checked:
- Jenkins test and merge pipelines (e.g. L0_Test.groovy, L0_MergeRequest.groovy) consume the fully qualified
LLM_DOCKER_IMAGE/LLM_SBSA_DOCKER_IMAGEvariables, not the rawIMAGE_NAMEproperty.- The
BuildDockerImage.groovyscript defines its own staging registry in a hard-codedIMAGE_NAME = "urm.nvidia.com/sw-tensorrt-docker/tensorrt-llm-staging", so it remains unaffected by changes tojenkins/current_image_tags.properties.- The
docker/Makefiletargets underdocker/correctly sourceIMAGE_NAMEfromjenkins/current_image_tags.propertiesfor local builds, which is the intended behavior.No conflicting definitions or silent mismatches were found. Updating
IMAGE_NAMEinjenkins/current_image_tags.propertiesto the new base (urm.nvidia.com/sw-tensorrt-docker/tensorrt-llm) aligns with existing conventions and does not require further downstream adjustments.
|
PR_Github #16207 [ run ] triggered by Bot |
|
PR_Github #16207 [ run ] completed with state |
|
/bot run --stage-list "Build-Docker-Images" |
|
PR_Github #16405 [ run ] triggered by Bot |
|
I suggest doing the same change for line 175 By the way, I started a build just for the docker images, because they were not built in the previous one. I'll check the result afterwards. |
We use the |
Oops, yes, you're right, there is no copy there. By the way, I also found a 1.4GB waste in our base image |
|
PR_Github #16405 [ run ] completed with state |
|
/bot run --stage-list "Build-Docker-Images" |
|
PR_Github #16536 [ run ] triggered by Bot |
|
PR_Github #16536 [ run ] completed with state |
|
/bot run --stage-list "Build-Docker-Images" |
|
PR_Github #16574 [ run ] triggered by Bot |
|
PR_Github #16574 [ run ] completed with state |
|
/bot run --stage-list "Build-Docker-Images" |
|
PR_Github #16657 [ run ] triggered by Bot |
|
PR_Github #16657 [ run ] completed with state |
|
/bot skip --comment "ucxx builds are randomly failing independent of the changes here" |
|
PR_Github #16710 [ skip ] triggered by Bot |
|
PR_Github #16710 [ skip ] completed with state |
4 participants
Summary by CodeRabbit
NB: The
IMAGE_NAMEfixes build problems fortrtllm_buildtarget in Makefile.