Repository navigation
fix(docker): shadow distro-owned PyYAML on TRT-LLM bases before smg install - #1666
Conversation
…nstall The SMG+TRTLLM image build fails on the NGC 1.3.0rc18 base: it ships a debian-installed PyYAML 6.0.1 without a pip RECORD file, and the smg wheel install (--force-reinstall, dependency set includes pyyaml) dies with "uninstall-no-record-file" when pip tries to remove it. Pre-install a pip-managed PyYAML with --ignore-installed, scoped to ENGINE=trtllm: the fresh copy shadows the distro one on sys.path and in pip's metadata view, so install-smg.sh can upgrade pyyaml through its normal uninstall/reinstall path. The sglang/vllm/tgl builds keep their exact pip behavior. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
📝 WalkthroughWalkthroughThe Dockerfile now conditionally installs a pip-managed ChangesTensorRT-LLM PyYAML Installation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Dockerfile to conditionally install pyyaml with pip when the ENGINE is set to trtllm to bypass a dependency uninstallation issue. The reviewer suggests using python3 -m pip instead of calling pip directly to ensure robustness and use of the correct Python interpreter.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # then upgrade pyyaml normally. Scoped to trtllm so the other engine bases | ||
| # keep their exact pip behavior. | ||
| RUN if [ "${ENGINE}" = "trtllm" ]; then \ | ||
| pip install --no-cache-dir --ignore-installed pyyaml; \ |
There was a problem hiding this comment.
Using python3 -m pip is more robust than calling pip directly. It ensures that the package is installed using the correct Python 3 interpreter and avoids potential issues where the pip executable is not in the PATH or points to a different Python installation.
python3 -m pip install --no-cache-dir --ignore-installed pyyaml; \
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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
🤖 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 `@docker/engine.Dockerfile`:
- Around line 65-67: The Dockerfile RUN that executes pip install
--ignore-installed pyyaml when ENGINE=trtllm leaves PyYAML unpinned and
build-time dependent; update the build to pin a specific PyYAML version (or use
a pip constraints file) so the resolved version is reproducible: either add a
pinned spec (e.g., pyyaml==X.Y.Z) in the RUN that installs it, add the same
fixed version into bindings/python/pyproject.toml dependencies, or supply a
constraints file referenced by pip; alternatively, add a clear comment in the
RUN and documentation explaining that drifting versions are intentional and
acceptable—reference the RUN block in docker/engine.Dockerfile,
scripts/installation/install-smg.sh (which currently doesn’t touch PyYAML), and
bindings/python/pyproject.toml when making the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b02c16a0-ffe6-4a71-bcfd-4534f2de44fd
📒 Files selected for processing (1)
docker/engine.Dockerfile
1 participant
Description
The
build (nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc18)dry-run on #1665 fails ininstall-smg.sh:The rc18 NGC base ships a debian-owned PyYAML (rc10 did not). The smg wheel install uses
--force-reinstall, which reinstalls the dependency set including pyyaml, and pip cannot uninstall a RECORD-less distro package.Changes
One conditional step in
docker/engine.Dockerfile, scoped toENGINE=trtllm, beforeinstall-smg.sh:pip install --ignore-installed pyyamlinstalls a pip-managed copy that shadows the distro one on sys.path and in pip's metadata view, so the smg install can then upgrade pyyaml through its normal uninstall/reinstall path.Deliberately NOT a blanket
--ignore-installedin the sharedinstall-smg.sh: that would make pip blind to every installed distribution on all engine bases and risks hybrid installs / stomping engine-pinned deps on the sglang/vllm images that build fine today.Test Plan
build (…rc18)dry-run should go green.Summary by CodeRabbit