build: apply trtllm image floors to the system-site install - #13256
Merged
pvijayakrish merged 3 commits intoAug 14, 2026
Conversation
requirements.trtllm.txt floors seven packages that the upstream tensorrt-llm/release base bundles at older patch releases. That file's header says naming a package there is enough to refresh the bundled copy in place. For these seven it is not: those installs run with VIRTUAL_ENV set, so they land in /opt/dynamo/venv while the base image's copy stays on disk under dist-packages, where the image inventory reads. Shipped images disagreed with the declared floors on every one: pillow floor >=12.3.0 bundled 12.2.0 mistune floor >=3.3.0 bundled 3.2.1 tornado floor >=6.5.6 bundled 6.5.5 jupyter-server floor >=2.20.0 bundled 2.18.2 jupyterlab floor >=4.5.10 bundled 4.5.7 gitpython floor >=3.1.58 bundled 3.1.50 soupsieve floor >=2.8.4 bundled 2.8.3 aiohttp was the only floor in that file that landed, and it landed because of the explicit system-interpreter install already in this Dockerfile rather than because of the floor. Install the seven through the system interpreter the same way, and mirror them in the pre_runtime whiteout so the squash COPY does not ship a renamed old dist-info beside the new one. pillow and gitpython also drop their import packages (PIL, git), which the whiteout covers. The guard normalizes distribution names before comparing, since the on-disk directories do not agree on spelling: jupyter_server-*.dist-info and GitPython-*.dist-info both appear. It requires exactly one dist-info per package at or above the floor, so a venv-only install, a missed whiteout, or an old copy surviving beside the new one fails the build instead of showing up in an image inventory later. Named packages only, no re-solve, matching the existing narrow-by-design rule for this file. Signed-off-by: Dan Gil <dagil@nvidia.com>
Collaborator
Author
|
/ok to test 3cb0c54 |
github-actions
Bot
added
backend::trtllm
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cb0c54b7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
|
🎯 Code Coverage (details) 🔗 Commit SHA: a4dd3ca | Docs | Datadog PR Page | Give us feedback! |
Three defects in the system-site floor change, all found in review. The GitPython whiteout used `GitPython-*`. Shell globs are case-sensitive and the base image stores the directory lowercase -- `gitpython-3.1.50.dist-info`, recorded for both architectures in container/compliance/base_sboms. The pattern matched nothing, so the overlay re-added the old metadata beside the upgraded copy and the image kept reporting 3.1.50. The Pillow whiteout used `PIL.libs`. auditwheel names the vendored directory after the distribution, not the import package, so the real path is `pillow.libs` -- the convention every sibling Dockerfile already follows (av.libs, decord.libs, opencv_python_headless.libs). `pillow-*` does not cover it either, since the separator is a dot. The base image's hash-named bundled libraries therefore survived next to the upgraded ones. Neither defect could fail the build. The in-stage assertion runs inside runtime_full, before `COPY --from=runtime_full / /`, and a whiteout mistake only produces a duplicate after that overlay. Add a post-overlay guard running the same normalized dist-info comparison at the point where the duplicate can exist, mirroring what DALI already does for exactly this reason. Also add --no-deps to the install. Without it pip's default only-if-needed strategy upgrades a transitive dependency whenever a newly selected release wants one the base image does not satisfy, which re-solves part of upstream's graph and strands that dependency's old metadata -- the whiteout names only the seven packages. These are patch-level bumps within one major, so the base image's existing dependencies satisfy them. Signed-off-by: Dan Gil <dagil@nvidia.com>
Collaborator
Author
|
/ok to test 7139833 |
pvijayakrish
left a comment
pvijayakrish
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the current file state at 7139833 against the wheels on PyPI, the checked-in base SBOMs, and the base image config; the guard one-liner was exercised against fixtures. The mechanism is right, and the round-2 fixes (lowercase [Gg]it[Pp]ython-*, pillow.libs, --no-deps, post-overlay guard) all check out. Inline comments below on the gaps that remain.
Two PR-level items:
-
Forward-port: main carries the same seven floors (and the same "refresh in place" comments in requirements.trtllm.txt), has the aiohttp system-site block but not this one, and pins
tensorrt-llm/release:1.3.0rc23, whose baseline SBOM still bundles all seven below the floors (pillow 12.2.0, mistune 3.2.1, tornado 6.5.5, jupyter-server 2.18.2, jupyterlab 4.5.7, gitpython 3.1.50, soupsieve 2.8.3) — the mismatch table in this PR's comment applies verbatim on main. I couldn't find a main companion PR. What's the forward-port plan? -
PR description: the guard-validation table describes the first commit (it predates the post-overlay guard), and "current patch releases" undersells what actually installs today (jupyterlab 4.6.3, soupsieve 2.9.2 — minor jumps). Since this body reads as the design doc for the pattern, worth refreshing before merge.
…loads Review findings on the system-site floor mechanism. An open floor does not stay on the patch line. `jupyterlab>=4.5.10` resolves to 4.6.3, a minor jump that adds a hard runtime dependency on jupyter-builder which the base image does not bundle -- and with --no-deps that dependency is simply absent, so `jupyter lab` raises ImportError from a top-level import. Every floor now carries an upper bound at the next minor, which makes --no-deps sound and keeps each package on the line its fix shipped in, so the caps cost nothing. jupyterlab also installs ~400 wheel data files under /usr/local/share/jupyter/lab, most of them content-hashed static bundles renamed every release. The in-stage upgrade replaces them, then the overlay re-adds the base image's copies beside the new ones -- the same defect class as the pillow.libs whiteout, one directory over. Whited out. The dist-info comparison cannot see that class at all: a package can carry exactly one correct dist-info while stale hash-named payload files ship. pillow 12.2.0 and 12.3.0 each vendor 18 differently-hashed shared objects and share one path. Add a payload probe that diffs pillow.libs against the entries RECORD says the installed wheel owns, so a survivor from the base image fails the build. This is what the DALI guard does -- it inspects libraries, not version strings. Carry aiohttp in both guard dicts. It is managed by the same mechanism and had only an in-stage guard, which by this file's own argument cannot observe a whiteout mistake. The dicts now hold ranges rather than floors, so the aiohttp upper bound is expressed the same way as the rest. Add --no-cache-dir to both installs. The base image sets no PIP_NO_CACHE_DIR, so these RUNs left wheel blobs in /root/.cache/pip and the squash COPY carried them into the shipped image; extending the whiteout cannot fix that, since the overlay re-adds runtime_full's copy. Correct requirements.trtllm.txt. Its header claimed a floor there installs over the bundled copy, which is the assumption that produced this bug -- #13195 raised floors in that file and none reached the image. It now states that a floor governs the venv copy only, and that a base-bundled package must also be added to the system-interpreter install and the whiteout. Signed-off-by: Dan Gil <dagil@nvidia.com>
Collaborator
Author
|
/ok to test a4dd3ca |
Contributor
pvijayakrish
approved these changes
Aug 14, 2026
dagil-nvidia
added a commit
that referenced
this pull request
Aug 15, 2026
…rence, recipe ID fix Fix the helm-docs source template's docs link (the generated README was fixed but generate-helm-docs regenerated the old link, dirtying the operator check). Adopt main's refreshed vLLM benchmark sampling-flag reference (#13036 — same shipped flag set, code-verified prose) and the Qwen3.8 recipe model-ID correction (#13265). Triaged all main docs commits since the last snapshot point: #13192 and #11723 docs excluded (features absent from 1.4.0); release-side #13280/#13256 have no docs impact (AIC-core wheel is a Spica dependency; planner shim wording verified still correct). Signed-off-by: Dan Gil <dagil@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
container/deps/requirements.trtllm.txtfloors seven packages that the upstreamtensorrt-llm/releasebase bundles at older patch releases. That file's header states that naming a package there is enough to refresh the bundled copy in place. For these seven it is not.Those installs run with
VIRTUAL_ENVset, so they land in/opt/dynamo/venv. The base image's copy stays on disk under/usr/local/lib/python3.12/dist-packages, which is where an image inventory reads. The venv is--system-site-packages, so the newer copy shadows the old one at import time and the mismatch is invisible from inside the container.The declared floors and the shipped versions disagreed on every one:
>=12.3.0>=3.3.0>=6.5.6>=2.20.0>=4.5.10>=3.1.58>=2.8.4aiohttpis floored in the same file and is the only one that landed — because of the explicit system-interpreter install already in this Dockerfile, not because of the floor.How
Install the seven through
/usr/bin/python3 -m pip --break-system-packages, mirroring the existingaiohttpblock, and mirror them in thepre_runtimewhiteout so the squashCOPYcannot ship a renamed olddist-infobeside the new one.pillowandgitpythonalso drop import packages under different names (PIL,git); the whiteout covers those.Named packages only, no re-solve, matching the narrow-by-design rule this file already follows.
Guard
The build fails rather than deferring the problem to an inventory afterwards. The guard normalizes distribution names before comparing, because the on-disk directories do not agree on spelling —
jupyter_server-*.dist-infoandGitPython-*.dist-infoboth appear. It requires exactly onedist-infoper package, at or above the floor.Exercised against five states before commit:
Both architectures render cleanly via
container/render.py(linux/amd64,linux/arm64, CUDA 13.1) and the guard compiles as rendered.Risk
Scoped to the trtllm runtime image. No dependency re-solve, no change to any other image, no change to
requirements.trtllm.txtitself — the floors there already declare these versions; this makes the image match them.