[None][doc] Note runtime-dependency requirement for the kimi_k3 Slurm container image - #17456
[None][doc] Note runtime-dependency requirement for the kimi_k3 Slurm container image#17456brnguyen2 wants to merge 1 commit into
Conversation
The kimi_k3 prerequisites are performed inside a container, but the Slurm scripts start a fresh container from --image and mount only the repository and the checkpoint. Nothing dependency-shaped carries over: the repository-root .venv-3.12 is the build venv created by build_wheel.py, and the editable install is --no-deps. Passing a build/devel image therefore fails every rank with a bare ModuleNotFoundError for a runtime dependency. Document that --image must be a release-style TensorRT-LLM container, and note why the scripts export PYTHONPATH for anyone adapting them. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
WalkthroughThe Kimi K3 README expands Slurm prerequisites. It documents required TensorRT-LLM runtime dependencies, the build environment limitation, the related ChangesKimi K3 runtime setup
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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 `@examples/kimi_k3/README.md`:
- Around line 38-43: Update the container-boundary description in the README to
state that packages installed into repository-mounted paths, including
`$REPO/.venv-3.12`, `$HOME`, or paths from `EXTRA_MOUNTS`, carry into the Slurm
container, while packages installed only in the preparation container’s base
environment do not. Replace the inaccurate claim that nothing installed during
build preparation carries over and preserve the surrounding explanation of the
virtual environment and editable install.
🪄 Autofix
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: db1b56e4-42dd-4fcb-a684-eee167ddef36
📒 Files selected for processing (1)
examples/kimi_k3/README.md
| scripts start a fresh container from that image and mount only the | ||
| repository and the checkpoint, so nothing installed while preparing the | ||
| build carries over: the repository-root `.venv-3.12` is the build | ||
| environment created by `build_wheel.py` (it contains Conan and pip, not | ||
| PyTorch or Transformers), and `pip install --no-deps -e .` installs | ||
| `tensorrt_llm` alone. With an image that lacks the dependencies, every rank |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the container-boundary description.
The Slurm script mounts $REPO and uses $REPO/.venv-3.12/bin. Packages installed into that repository-mounted virtual environment therefore carry into the Slurm container. This includes the packages installed at Lines 51-64. The script also mounts $HOME and supports EXTRA_MOUNTS.
Replace “nothing installed while preparing the build carries over” with a distinction between packages in mounted paths and packages installed only in the preparation container’s base environment.
Suggested wording
- image. The Slurm scripts start a fresh container from that image and mount only the
- repository and the checkpoint, so nothing installed while preparing the build carries over:
+ image. The Slurm scripts start a fresh container from that image. They mount the
+ repository, including `.venv-3.12`, and the checkpoint, so packages installed in
+ that virtual environment are available. Packages installed only in the preparation
+ container's base environment are not:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scripts start a fresh container from that image and mount only the | |
| repository and the checkpoint, so nothing installed while preparing the | |
| build carries over: the repository-root `.venv-3.12` is the build | |
| environment created by `build_wheel.py` (it contains Conan and pip, not | |
| PyTorch or Transformers), and `pip install --no-deps -e .` installs | |
| `tensorrt_llm` alone. With an image that lacks the dependencies, every rank | |
| scripts start a fresh container from that image. They mount the | |
| repository, including `.venv-3.12`, and the checkpoint, so packages installed in | |
| that virtual environment are available. Packages installed only in the preparation | |
| container's base environment are not: | |
| repository-root `.venv-3.12` is the build environment created by `build_wheel.py` | |
| (it contains Conan and pip, not PyTorch or Transformers), and `pip install | |
| --no-deps -e .` installs `tensorrt_llm` alone. With an image that lacks the dependencies, every rank |
🤖 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 `@examples/kimi_k3/README.md` around lines 38 - 43, Update the
container-boundary description in the README to state that packages installed
into repository-mounted paths, including `$REPO/.venv-3.12`, `$HOME`, or paths
from `EXTRA_MOUNTS`, carry into the Slurm container, while packages installed
only in the preparation container’s base environment do not. Replace the
inaccurate claim that nothing installed during build preparation carries over
and preserve the surrounding explanation of the virtual environment and editable
install.
|
/bot run |
|
PR_Github #64914 [ run ] triggered by Bot. Commit: |
|
PR_Github #64914 [ run ] completed with state |
2 participants
Dev Engineer Review
examples/kimi_k3/README.mdto clarify container and environment requirements.PYTHONPATH="$REPO"requirement for Slurm script execution.QA Engineer Review
No test changes.
Description
The
examples/kimi_k3prerequisites are performed inside a TensorRT-LLMcontainer, but the Slurm launchers start a fresh container from
--imageand mount only the repository and the checkpoint. Nothing dependency-shaped
carries over from the container where the prerequisites were run:
.venv-3.12created bybuild_wheel.pyis a buildvirtual environment (Conan, pip, and their helpers; no PyTorch or
Transformers), so it does not carry the runtime;
pip install --no-deps -e .deliberately installs onlytensorrt_llm.So the runtime dependencies must come from the image itself. A multi-GPU
Slurm submission using a build image failed on every rank with a bare
ModuleNotFoundError: No module named 'transformers'raised from insidetensorrt_llm/llmapi/llm.py— nothing in the message points at the image.This adds a short note to the Prerequisites section stating that
--imagemust be a release-style TensorRT-LLM container that already provides the
runtime dependencies, and explaining why. It also documents why the scripts
export
PYTHONPATH="$REPO":python3 <script>puts the script's directoryon
sys.path, not the repository root, so a launcher adapted without thatexport fails with
No module named 'tensorrt_llm'even with a correct image.Documentation only; no code changes.
Note: PR #17455 also edits
examples/kimi_k3/README.md(run/examples area).This change is confined to Prerequisites, but a textual conflict is possible;
whichever merges second will resolve it.
Test Coverage
Documentation-only change; no tests apply.
PR Checklist