revert: restore Dockerfile.vllm and build/run scripts to August 28 vers #2892
Conversation
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
…havior This reverts changes to maintain the same build and run behaviors as before August 28 commit 82bae24. Changes: - Split Dockerfile.vllm dev target into local-dev (Dev Container) and dev (run.sh) - Add comprehensive feature matrix comparing both development targets - Remove --uid/--gid options from build.sh (now handled by local-dev target) - Remove DEV_MODE logic from run.sh (simplified workspace mounting) - Consolidate ENV variables in both targets for better maintainability - Update build.sh to use local-dev target for UID/GID mapping Signed-off-by: Keiven Chang <[email protected]>
keivenchang
requested review from
a team,
alec-flowers,
ishandhanani,
nnshah1,
ptarasiewiczNV,
richardhuo-nv,
rmccorm4 and
tanmayv25
as code owners
Contributor
WalkthroughIntroduces dual development targets in container/Dockerfile.vllm: local-dev (non-root, UID/GID-mapped) and dev (root). Updates container/build.sh to pass host UID/GID only for local-dev. Simplifies container/run.sh by removing --target and dev-mode, refactoring workspace-mount, HF cache, and privileged flag handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant BS as build.sh
participant DK as Docker
participant DF as Dockerfile.vllm
participant IMG as Image
Dev->>BS: ./container/build.sh --target local-dev
BS->>BS: Append USER_UID/USER_GID
BS->>DK: docker build --target local-dev --build-arg UID/GID
DK->>DF: Build stage local-dev
DF-->>IMG: Produce local-dev image (non-root)
Dev->>BS: ./container/build.sh --target dev
BS->>DK: docker build --target dev
DK->>DF: Build stage dev
DF-->>IMG: Produce dev image (root)
sequenceDiagram
autonumber
actor Dev as Developer
participant RS as run.sh
participant DK as Docker
participant C as Container
Dev->>RS: ./container/run.sh [--mount-workspace ...] [--hf-cache ...] [--privileged ...]
RS->>RS: Parse options (no --target, no DEV_MODE)
RS->>RS: If --mount-workspace: add volumes (/workspace,/tmp,/mnt), HF_TOKEN, force -it, default PRIVILEGED=TRUE
RS->>RS: If HF_CACHE set: mount to /root/.cache/huggingface
RS->>RS: PRIVILEGED_STRING = "" if FALSE else "--privileged"
RS->>DK: docker run ... ${PRIVILEGED_STRING} -v ...
DK-->>C: Start container
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
container/Dockerfile.vllm (1)
331-432: local-dev fails if 'ubuntu' user is absent and lacks arg preflight; harden user setup.
- This stage assumes an existing “ubuntu” user; most CUDA base images don’t have it. groupmod/usermod will fail.
- Add an explicit preflight for USER_UID/USER_GID and create the user/group if missing. Also, the COPY of /usr/local/bin from runtime is redundant (this stage is FROM runtime).
Apply:
@@ -FROM runtime AS local-dev +FROM runtime AS local-dev @@ -ENV USERNAME=ubuntu -ARG USER_UID -ARG USER_GID +ENV USERNAME=ubuntu +ARG USER_UID +ARG USER_GID ARG WORKSPACE_DIR=/workspace @@ -COPY --from=runtime /usr/local/bin /usr/local/bin +## Inherits /usr/local/bin from runtime; no need to copy again @@ -RUN apt-get update && apt-get install -y sudo gnupg2 gnupg1 \ - && echo "$USERNAME ALL=(root) NOPASSWD:ALL" > /etc/sudoers.d/$USERNAME \ - && chmod 0440 /etc/sudoers.d/$USERNAME \ - && mkdir -p /home/$USERNAME \ - && groupmod -g $USER_GID $USERNAME \ - && usermod -u $USER_UID -g $USER_GID $USERNAME \ - && chown -R $USERNAME:$USERNAME /home/$USERNAME \ - && rm -rf /var/lib/apt/lists/* \ - && chsh -s /bin/bash $USERNAME +RUN apt-get update && apt-get install -y sudo gnupg2 gnupg1 \ + && test -n "$USER_UID" -a -n "$USER_GID" || (echo "ERROR: USER_UID and USER_GID must be set for local-dev" >&2; exit 1) \ + && if ! id -u "$USERNAME" >/dev/null 2>&1; then \ + groupadd -g "$USER_GID" "$USERNAME" && \ + useradd -m -u "$USER_UID" -g "$USER_GID" -s /bin/bash "$USERNAME"; \ + else \ + groupmod -o -g "$USER_GID" "$USERNAME" && \ + usermod -o -u "$USER_UID" -g "$USER_GID" "$USERNAME"; \ + fi \ + && echo "$USERNAME ALL=(root) NOPASSWD:ALL" > /etc/sudoers.d/$USERNAME \ + && chmod 0440 /etc/sudoers.d/$USERNAME \ + && chown -R $USERNAME:$USERNAME /home/$USERNAME \ + && rm -rf /var/lib/apt/lists/* \ + && chsh -s /bin/bash $USERNAMEThis keeps the “no default USER_UID/GID” policy while preventing fragile builds. Also aligns with the team learning on explicit UID/GID mapping.
🧹 Nitpick comments (5)
container/Dockerfile.vllm (2)
283-321: Sync the feature matrix with actual behavior (WORKDIR, venv).
- Matrix lists Working Directory as “/home/ubuntu/dynamo”, but local-dev sets WORKDIR to $HOME (not “.../dynamo”), and DYNAMO_HOME defaults to /workspace. Clarify either matrix or env to avoid confusion.
- “Python Environment: User-owned venv” is accurate (venv is chowned) but still located at /opt/dynamo/venv. Consider wording “user-writable /opt/dynamo/venv”.
434-508: Dev (root-run) target looks good; mirror env with local-dev where possible.
- Env and toolchain setup are consistent with runtime and local-dev. No blockers.
If desired, set CARGO_TARGET_DIR to ${WORKSPACE_DIR}/target (instead of hardcoding /workspace/target) for symmetry with local-dev.
container/build.sh (1)
469-471: Correct scoping of UID/GID to local-dev; nice. Add help text and guardrails.
- Good: Passing host UID/GID only when TARGET=local-dev matches the intended model.
- Gaps:
- show_help omits --target; either remove parsing or document it to avoid confusion.
- If a non-vLLM Dockerfile lacks a local-dev stage, building with --target local-dev will fail late. Consider an early check or clearer error.
Would you like a follow-up patch to add --target to show_help and emit a friendlier error when the chosen Dockerfile doesn’t define that target?
container/run.sh (2)
231-247: Workspace mounting defaults make sense; ensure target usage is consistently removed or documented.
- Mounts and defaults (HF_CACHE, PRIVILEGED=TRUE, -it) are sensible for local dev via dev target.
- The script still parses/uses --target to shape IMAGE naming elsewhere. If runtime is now target-agnostic, remove that parsing—or re-document it.
253-257: HF cache mount path correct for root-run dev.Mounts to /root/.cache/huggingface. If ENTRYPOINT changes to a non-root user later, consider honoring HF_HOME or mapping to $HOME/.cache/huggingface.
📜 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 (3)
container/Dockerfile.vllm(2 hunks)container/build.sh(1 hunks)container/run.sh(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2822
File: container/Dockerfile.vllm:343-352
Timestamp: 2025-09-03T01:10:12.599Z
Learning: In the dynamo project's local-dev Docker targets, USER_UID and USER_GID build args are intentionally left without default values to force explicit UID/GID mapping during build time, preventing file permission issues in local development environments where container users need to match host user permissions for mounted volumes.
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2797
File: container/Dockerfile:437-449
Timestamp: 2025-08-30T20:43:49.632Z
Learning: In the dynamo project's devcontainer setup, the team prioritizes consistency across framework-specific Dockerfiles (like container/Dockerfile, container/Dockerfile.vllm, etc.) by mirroring their structure, even when individual optimizations might be possible, to maintain uniformity in the development environment setup.
📚 Learning: 2025-09-03T01:10:12.599Z
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2822
File: container/Dockerfile.vllm:343-352
Timestamp: 2025-09-03T01:10:12.599Z
Learning: In the dynamo project's local-dev Docker targets, USER_UID and USER_GID build args are intentionally left without default values to force explicit UID/GID mapping during build time, preventing file permission issues in local development environments where container users need to match host user permissions for mounted volumes.
Applied to files:
container/build.shcontainer/Dockerfile.vllm
📚 Learning: 2025-08-30T20:43:49.632Z
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2797
File: container/Dockerfile:437-449
Timestamp: 2025-08-30T20:43:49.632Z
Learning: In the dynamo project's devcontainer setup, the team prioritizes consistency across framework-specific Dockerfiles (like container/Dockerfile, container/Dockerfile.vllm, etc.) by mirroring their structure, even when individual optimizations might be possible, to maintain uniformity in the development environment setup.
Applied to files:
container/Dockerfile.vllm
📚 Learning: 2025-08-30T20:43:10.091Z
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.091Z
Learning: In the dynamo project, devcontainer.json files use templated container names (like "dynamo-vllm-devcontainer") that are automatically processed by the copy_devcontainer.sh script to generate framework-specific configurations with unique names, preventing container name collisions.
Applied to files:
container/Dockerfile.vllm
⏰ 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). (2)
- GitHub Check: Build and Test - vllm
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
container/run.sh (1)
266-270: Privileged flag handling is clear and explicit.LGTM. The normalized PRIVILEGED_STRING removes ambiguity.
- Remove unused USER_UID and USER_GID variables from build.sh - Remove extra blank line in run.sh Signed-off-by: Keiven Chang <[email protected]>
nnshah1
reviewed
Sep 5, 2025
nnshah1
approved these changes
Sep 5, 2025
Contributor
nnshah1
left a comment
nnshah1
left a comment
There was a problem hiding this comment.
LGTM - we can explore a better / different solution in the futue
nv-tusharma
approved these changes
Sep 5, 2025
keivenchang
deleted the
keivenchang/BUG-5501463__revert-Dockerfile.vllm_build_run_scripts
branch
keivenchang
added a commit
that referenced
this pull request
Sep 5, 2025
…rs (#2892) Signed-off-by: Keiven Chang <[email protected]>
nnshah1
pushed a commit
that referenced
this pull request
Sep 8, 2025
…rs (#2892) Signed-off-by: Keiven Chang <[email protected]> Co-authored-by: Keiven Chang <[email protected]> Signed-off-by: nnshah1 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview:
This PR reverts Dockerfile.vllm and build/run scripts to maintain the same build and run behaviors as before August 28 commit 82bae24. This is to maintain backward compatibility.
Details:
local-dev: For VS Code/Cursor Dev Container plugin use onlydev: For command-line development with run.sh scriptWhere should the reviewer start?
Related Issues:
BUG-5501463