fix: copy whole workspace for pre-merge vllm tests #2146
Conversation
nv-anants
requested review from
rmccorm4,
tanmayv25,
richardhuo-nv,
ptarasiewiczNV,
ishandhanani,
alec-flowers,
nnshah1 and
a team
as code owners
WalkthroughThis change updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~6 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🔭 Outside diff range comments (1)
container/Dockerfile.vllm (1)
467-469:COPYinside aRUNblock is invalid Docker syntax – build will fail
COPY(and all other build-stage directives) must be a top-level instruction. Embedding it inside a shellif …; thenin aRUNlayer will cause Docker to error-out during parsing.RUN if [ "$ARCH" = "arm64" ]; then \ COPY --from=base /opt/vllm /opt/vllm; \ fiThe conditional copy can be expressed safely by splitting the stage or by copying unconditionally and relying on build-args for correctness.
- RUN if [ "$ARCH" = "arm64" ]; then \ - COPY --from=base /opt/vllm /opt/vllm; \ - fi +# Copy vllm artifacts only for arm64 builds +ARG ARCH +COPY --from=base /opt/vllm /opt/vllm +RUN if [ "$ARCH" != "arm64" ]; then rm -rf /opt/vllm; fiWithout this fix the image will not build.
🧹 Nitpick comments (1)
container/Dockerfile.vllm (1)
486-488: Whole-workspace copy bloats the runtime image & risks overwriting earlier layersPulling the entire
/workspace/fromci_minimummakes the runtime image several GB larger, ships all sources, build caches (target/,.mypy_cache/, etc.) and can mask/overwrite previously copied artefacts such aswheelhouse/andmetrics.If the sole goal is to run mypy/pre-merge tests, consider:
- Creating a separate “ci-runtime” stage extending
runtimejust for CI.- Copying only what static analysis needs, e.g.
COPY --from=ci_minimum /workspace/{deploy,components,tests} /workspace/- Using a
.dockerignoreinci_minimumto strip large build artefacts before they reach this stage.These changes keep the production runtime slim while still unblocking CI.
Is there any production workflow depending on the enlarged image size that we should account for?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
container/Dockerfile.vllm(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nnshah1
PR: ai-dynamo/dynamo#1444
File: tests/fault_tolerance/scenarios.py:57-57
Timestamp: 2025-07-01T15:39:56.789Z
Learning: The fault tolerance tests in tests/fault_tolerance/ are designed to run only in the mounted container environment, so hardcoded absolute paths with `/workspace/` prefix are intentional and should not be changed to relative paths.
container/Dockerfile.vllm (1)
Learnt from: nnshah1
PR: #1444
File: tests/fault_tolerance/scenarios.py:57-57
Timestamp: 2025-07-01T15:39:56.789Z
Learning: The fault tolerance tests in tests/fault_tolerance/ are designed to run only in the mounted container environment, so hardcoded absolute paths with /workspace/ prefix are intentional and should not be changed to relative paths.
⏰ 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: Build and Test - vllm
Overview:
Copy whole dynamo repo into runtime container to run all pre-merge and mypy CI tests
Details:
ref: OPS-440
Summary by CodeRabbit