feat: devcontainer with framework-specific configurations #2797
Conversation
keivenchang
requested review from
a team,
alec-flowers,
ishandhanani,
nnshah1,
ptarasiewiczNV,
richardhuo-nv,
rmccorm4 and
tanmayv25
as code owners
WalkthroughAdds a non-root “local-dev” stage across multiple Dockerfiles for Dev Container workflows, updates .devcontainer docs and naming, and expands .gitignore to exclude generated devcontainer subdirs. Standardizes container user/env/tooling setup, and documents a script-driven generation flow for per-framework devcontainer configs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Repo as Repo (.devcontainer)
participant Script as copy_devcontainer.sh
participant FS as Workspace FS
participant IDE as IDE Dev Containers
Dev->>Script: Run generation (optional: --dry-run / --force)
Script->>Repo: Read base devcontainer configs
Script->>FS: Create .devcontainer/<framework>/devcontainer.json
Note right of FS: Generated structure per framework<br/>(e.g., trtllm, sglang, vllm, none)
Dev->>IDE: Open folder in Dev Container
IDE->>FS: Discover .devcontainer/*/devcontainer.json
IDE-->>Dev: Present selectable Dev Container entries
sequenceDiagram
autonumber
participant Docker as Docker Build
participant Base as Base Stage
participant Local as local-dev Stage
participant User as Non-root user (ubuntu)
Docker->>Base: Build base image
Docker->>Local: FROM base AS local-dev
Local->>Local: Install dev tools, maturin
Local->>Local: Configure Rust env (RUSTUP_HOME/CARGO_HOME)
Local->>Local: Copy venv/binaries with correct ownership
Local->>User: Create UID/GID user, sudo (where applicable)
Local->>Local: Set PYTHONPATH, CARGO_TARGET_DIR, history/cache
Note over Local,User: Stage ready for Dev Container attach
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
container/Dockerfile.sglang (3)
269-318: User may not exist; add group/user creation before sudoers and chown.Same issue as TRTLLM:
ubuntulikely doesn’t exist. Creating the user is required for Dev Containers to work.Apply:
-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 \ - && 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 \ + && groupadd --gid $USER_GID $USERNAME || true \ + && useradd --uid $USER_UID --gid $USER_GID -s /bin/bash -m $USERNAME || true \ + && echo "$USERNAME ALL=(root) NOPASSWD:ALL" > /etc/sudoers.d/$USERNAME \ + && chmod 0440 /etc/sudoers.d/$USERNAME \ + && rm -rf /var/lib/apt/lists/*
321-324: Do not chown/usr/local/bintoubuntu.Keep root ownership for system binaries.
Apply:
-COPY --from=base --chown=$USERNAME:$USERNAME /usr/local/bin /usr/local/bin +COPY --from=base /usr/local/bin /usr/local/bin
330-331: Fix PYTHONPATH to point at the mounted workspace under the dev user.In Dev Containers, the project mounts at
$HOME/dynamo. Current/workspace/...paths won’t resolve in local-dev sessions.Apply:
-ENV PYTHONPATH=/workspace/dynamo/components/planner/src:/workspace/examples/sglang:$PYTHONPATH +ENV PYTHONPATH=$HOME/dynamo/components/planner/src:$HOME/dynamo/examples/sglang:$PYTHONPATHcontainer/Dockerfile.vllm (2)
358-359: Avoid copying and chowning /usr/local/bin wholesaleCopying /usr/local/bin from runtime and changing ownership is redundant (FROM runtime already contains it) and risks permission drift on system tools. Remove this COPY.
Apply this diff:
-COPY --from=runtime --chown=$USERNAME:$USERNAME /usr/local/bin /usr/local/bin
356-357: Install Python tooling as the non-root dev user to avoid permission issuesYou chown the venv to the dev user, then install maturin as root, which reintroduces root-owned files into the venv. Install after switching to USER.
Apply this diff:
-COPY --from=runtime --chown=$USER_UID:$USER_GID ${VIRTUAL_ENV} ${VIRTUAL_ENV} -RUN chown $USERNAME:$USERNAME ${VIRTUAL_ENV} -# so we can use maturin develop -RUN uv pip install maturin[patchelf] -USER $USERNAME +COPY --from=runtime --chown=$USER_UID:$USER_GID ${VIRTUAL_ENV} ${VIRTUAL_ENV} +RUN chown $USERNAME:$USERNAME ${VIRTUAL_ENV} +USER $USERNAME +# so we can use maturin develop (writeable by $USERNAME) +RUN uv pip install --no-cache-dir maturin[patchelf]Also applies to: 361-363
🧹 Nitpick comments (8)
.gitignore (1)
94-96:.devcontainer/*/is too broad; it may ignore future repo-owned subfolders.This pattern will hide any immediate subdirectory under
.devcontainer(including ones we might want to check in later). Prefer explicitly ignoring generated folders and allowlisted exceptions if needed.Apply one of:
-# Auto-generated custom devcontainer.json directories -.devcontainer/*/ +# Auto-generated framework-specific devcontainer directories +.devcontainer/trtllm/ +.devcontainer/sglang/ +.devcontainer/none/Or keep the broad ignore but whitelist known repo-owned subdirs:
# Auto-generated custom devcontainer.json directories .devcontainer/*/ +!.devcontainer/scripts/ +!.devcontainer/assets/container/Dockerfile.trtllm (1)
571-599: Duplicate/conflicting CARGO_TARGET_DIR declarations.You set
CARGO_TARGET_DIRtwice (/opt/dynamo/targetthen$HOME/dynamo/.build/target). Keep the final user-scoped path and drop the earlier one to avoid confusion.Apply:
-ENV RUSTUP_HOME=/usr/local/rustup \ - CARGO_HOME=/usr/local/cargo \ - CARGO_TARGET_DIR=/opt/dynamo/target \ - PATH=/usr/local/cargo/bin:$PATH +ENV RUSTUP_HOME=/usr/local/rustup \ + CARGO_HOME=/usr/local/cargo \ + PATH=/usr/local/cargo/bin:$PATHcontainer/Dockerfile.sglang (1)
303-307: Minor: unify Rust target dir with other frameworks.For consistency and cleaner host mounts, prefer
$HOME/dynamo/.build/target.Apply:
-ENV RUSTUP_HOME=/usr/local/rustup \ - CARGO_HOME=/usr/local/cargo \ - CARGO_TARGET_DIR=/opt/dynamo/target \ - PATH=/usr/local/cargo/bin:$PATH +ENV RUSTUP_HOME=/usr/local/rustup \ + CARGO_HOME=/usr/local/cargo \ + CARGO_TARGET_DIR=/home/${USERNAME}/dynamo/.build/target \ + PATH=/usr/local/cargo/bin:$PATH.devcontainer/README.md (4)
226-237: Clarify path to the generator script.The script lives in
.devcontainer/, but the examples use./copy_devcontainer.sh. Make it explicit to reduce confusion when run from repo root.Apply:
-mkdir -p .devcontainer/jensen_dev -cp .devcontainer/devcontainer.json .devcontainer/jensen_dev/devcontainer.json +mkdir -p .devcontainer/jensen_dev +cp .devcontainer/devcontainer.json .devcontainer/jensen_dev/devcontainer.jsonAnd in the usage section below, prefer:
-./copy_devcontainer.sh +./.devcontainer/copy_devcontainer.sh
253-264: Use explicit script path and add dry-run/force examples accordingly.Align usage with actual script location.
Apply:
-./copy_devcontainer.sh +./.devcontainer/copy_devcontainer.sh @@ -./copy_devcontainer.sh --dryrun +./.devcontainer/copy_devcontainer.sh --dryrun @@ -./copy_devcontainer.sh --force +./.devcontainer/copy_devcontainer.sh --force
266-277: Add language to fenced “Generated Structure” block (MD040).Markdown lint: specify a language, e.g.,
text.Apply:
-``` +```text .devcontainer/ ├── devcontainer.json # Original VLLM configuration ├── copy_devcontainer.sh # Original generation script @@--- `247-252`: **Tighten wording and punctuation in “Available Frameworks”.** Minor grammar/style nits flagged by tooling. Use consistent punctuation and casing. Apply: ```diff -- **VLLM**: VLLM framework (✅ **Verified and stable** - tested by many users) -- **TRTLLM**: TensorRT-LLM framework (🔄 **In development** - being worked on) -- **SGLANG**: SGLang framework (🔄 **In development** - being worked on) -- **NONE**: No specific framework (🔄 **In development** - being worked on) +- **VLLM**: VLLM framework (✅ Verified and stable — tested by many users). +- **TRTLLM**: TensorRT-LLM framework (🔄 In development). +- **SGLANG**: SGLang framework (🔄 In development). +- **NONE**: No specific framework (🔄 In development).container/Dockerfile (1)
389-417: Minor: add noninteractive to apt install to avoid promptsSafer in CI/builders.
Apply this diff:
-RUN apt-get update -y && \ - apt-get install -y --no-install-recommends \ +RUN apt-get update -y && \ + DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
📜 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 (7)
.devcontainer/README.md(2 hunks).devcontainer/devcontainer.json(1 hunks).gitignore(1 hunks)container/Dockerfile(1 hunks)container/Dockerfile.sglang(3 hunks)container/Dockerfile.trtllm(1 hunks)container/Dockerfile.vllm(2 hunks)
🧰 Additional context used
🪛 LanguageTool
.devcontainer/README.md
[grammar] ~247-~247: There might be a mistake here.
Context: ...I frameworks. #### Available Frameworks - VLLM: VLLM framework (✅ **Verified and...
(QB_NEW_EN)
[grammar] ~248-~248: There might be a mistake here.
Context: ...ied and stable** - tested by many users) - TRTLLM: TensorRT-LLM framework (🔄 **I...
(QB_NEW_EN)
[grammar] ~249-~249: There might be a mistake here.
Context: ...🔄 In development - being worked on) - SGLANG: SGLang framework (🔄 **In deve...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ...🔄 In development - being worked on) - NONE: No specific framework (🔄 **In d...
(QB_NEW_EN)
[grammar] ~251-~251: There might be a mistake here.
Context: ...🔄 In development - being worked on) #### Usage ```bash # Generate framework-spec...
(QB_NEW_EN)
[grammar] ~279-~279: There might be a mistake here.
Context: ...nfig ``` #### Building Framework Images Before using framework-specific configur...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.devcontainer/README.md
267-267: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (7)
.devcontainer/devcontainer.json (1)
7-7: Confirm image/user alignment with VLLM local-dev image.
remoteUser: "ubuntu"requires that the image contains anubuntuuser with a valid home dir and shell. Please verify the VLLM local-dev stage actually creates this user (groupadd/useradd + sudoers) like the other Dockerfiles intend. Otherwise, Dev Containers will fail to start..devcontainer/README.md (1)
92-101: Step 1 still reads VLLM-only; align with new multi-framework flow.Since TRTLLM/SGLANG local-dev stages now exist, add a brief note or link to “Building Framework Images” so users pick the correct
--framework.container/Dockerfile.vllm (2)
283-289: Clear, consistent stage naming and purpose docsRenaming to “Dev Container Local Development” and clarifying the PURPOSE block improves discoverability for devcontainer users.
371-374: Confirm devcontainer override to avoid Nvidia entrypoint conflictsIf this image is used by VS Code/Cursor devcontainers, ensure devcontainer.json sets "overrideCommand": true (or equivalent) so the Nvidia entrypoint doesn’t interfere with shell startup.
Also applies to: 377-379
container/Dockerfile (3)
360-362: Explicit empty CMD is fineKeeping CMD [] here is harmless and keeps behavior consistent with the entrypoint.
446-448: Verify PYTHONPATH path matches devcontainer mountPYTHONPATH references $HOME/dynamo/components/planner/src. If the repo mounts to /workspaces/dynamo (common in VS Code), this may not resolve. Confirm devcontainer.json uses the same path or adjust to a relative workspace folder.
458-460: Devcontainer entrypoint behaviorAs with vLLM, ensure devcontainer.json sets "overrideCommand": true to get an interactive shell despite the Nvidia entrypoint.
| "updateRemoteUserUID": true, // Updates the UID of the remote user to match the host user, avoids permission errors | ||
| "image": "dynamo:latest-vllm-local-dev", // Use the latest VLLM local dev image | ||
| "runArgs": [ | ||
| "--name", "dynamo-vllm-devcontainer", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove fixed --name to avoid container name collisions.
Hard-coding --name dynamo-vllm-devcontainer will clash when multiple Dev Containers are opened, and overrides the extension’s unique naming. Let the Dev Containers extension manage names, or template with a unique suffix generated by your script.
Apply:
"runArgs": [
- "--name", "dynamo-vllm-devcontainer",
"--gpus=all",📝 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.
| "--name", "dynamo-vllm-devcontainer", | |
| "runArgs": [ | |
| "--gpus=all", |
🤖 Prompt for AI Agents
.devcontainer/devcontainer.json around line 12: remove the hard-coded "--name",
"dynamo-vllm-devcontainer" entry so the Dev Containers extension can manage
container names (to avoid collisions); either delete that flag entirely from the
"runArgs" array or replace it with a templated/unique name generated by your
devcontainer script if you truly need a custom name, ensuring multiple instances
won’t conflict.
There was a problem hiding this comment.
This will be auto generated with the copy_devcontainer.sh script
There was a problem hiding this comment.
🧩 Analysis chain
@keivenchang Thanks for the clarification! I can see from the copy_devcontainer.sh script that it automatically generates framework-specific container names by replacing the template values. The script handles the unique naming to prevent collisions between different framework configurations, which addresses the original concern about hard-coded names.
The automation approach makes sense for maintaining multiple framework-specific devcontainer configurations while avoiding manual duplication.
🏁 Script executed:
#!/bin/bash
# Check the copy_devcontainer.sh script to understand how container naming works
cat .devcontainer/copy_devcontainer.sh
Length of output: 107
@keivenchang Thank you for the clarification about the auto-generation! However, I wasn't able to locate the copy_devcontainer.sh script in the current repository state to verify how the container naming automation works.
Could you confirm if this script is part of the changes being introduced in this PR? If it's included in the PR but not yet visible in my current view, that would explain the automation mechanism you're referring to for generating unique framework-specific container names.
The approach of using a script to automatically generate framework-specific configurations with unique names definitely addresses the container name collision concern.
🏁 Script executed:
#!/bin/bash
# Search for the copy_devcontainer.sh script across the repository
find . -name "copy_devcontainer.sh" -type f 2>/dev/null
# Also search for any script files in .devcontainer directory
find .devcontainer -type f 2>/dev/null
# Check if there are any shell scripts that might be related
find . -name "*.sh" -type f | grep -i devcontainer 2>/dev/null
Length of output: 241
@keivenchang Thank you for the clarification! I understand that the copy_devcontainer.sh script (mentioned in the PR objectives) will handle the auto-generation of framework-specific container names from this template.
Since the script will automatically process the hard-coded name to create unique framework-specific variations, this addresses the container name collision concern. The template approach with automated processing makes sense for maintaining multiple devcontainer configurations efficiently.
✏️ Learnings added
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.059Z
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.
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.059Z
Learning: In the dynamo project's devcontainer setup, hard-coded container names in devcontainer.json files serve as templates that are automatically processed by the copy_devcontainer.sh script to generate framework-specific configurations with unique names, preventing container name collisions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # Much better than chown -R $USERNAME:$USERNAME /opt/dynamo/venv (~10min on my cpu) | ||
| COPY --from=base --chown=$USER_UID:$USER_GID ${VIRTUAL_ENV} ${VIRTUAL_ENV} | ||
| RUN chown $USERNAME:$USERNAME ${VIRTUAL_ENV} | ||
| COPY --from=base --chown=$USERNAME:$USERNAME /usr/local/bin /usr/local/bin | ||
|
|
||
| # so we can use maturin develop | ||
| RUN uv pip install maturin[patchelf] | ||
|
|
||
| USER $USERNAME | ||
| ENV HOME=/home/$USERNAME | ||
| ENV PYTHONPATH=$PYTHONPATH:$HOME/dynamo/components/planner/src | ||
| ENV CARGO_TARGET_DIR=$HOME/dynamo/.build/target | ||
| WORKDIR $HOME |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Install maturin after switching to USER to keep venv writeable by dev user
Same rationale as vLLM Dockerfile.
Apply this diff:
-COPY --from=base --chown=$USER_UID:$USER_GID ${VIRTUAL_ENV} ${VIRTUAL_ENV}
-RUN chown $USERNAME:$USERNAME ${VIRTUAL_ENV}
-# so we can use maturin develop
-RUN uv pip install maturin[patchelf]
-USER $USERNAME
+COPY --from=base --chown=$USER_UID:$USER_GID ${VIRTUAL_ENV} ${VIRTUAL_ENV}
+RUN chown $USERNAME:$USERNAME ${VIRTUAL_ENV}
+USER $USERNAME
+# so we can use maturin develop
+RUN uv pip install --no-cache-dir maturin[patchelf]Also applies to: 441-444
🤖 Prompt for AI Agents
In container/Dockerfile around lines 437 to 449 (also affecting 441-444): the
RUN uv pip install maturin[patchelf] is executed as root before switching to the
non-root USER, causing the virtualenv to be owned by root and not writable by
the dev user; move the maturin installation step to after the USER $USERNAME and
ENV HOME setup so the install runs as the non-root user (or alternatively chown
the virtualenv and related files to $USERNAME before switching), ensuring the
venv is writable by the dev user and keeping the rest of the file paths and ENV
settings unchanged.
There was a problem hiding this comment.
Not applicable-- just mirroring the Dockerfile.vllm file
There was a problem hiding this comment.
@keivenchang I understand you're maintaining consistency by mirroring the Dockerfile.vllm structure. Thanks for the clarification!
🐰
✨ Got it! Consistency across frameworks
makes perfect sense for maintenance
✏️ Learnings added
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2797
File: container/Dockerfile:437-449
Timestamp: 2025-08-30T20:43:49.593Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.059Z
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.
Learnt from: keivenchang
PR: ai-dynamo/dynamo#2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.059Z
Learning: In the dynamo project's devcontainer setup, hard-coded container names in devcontainer.json files serve as templates that are automatically processed by the copy_devcontainer.sh script to generate framework-specific configurations with unique names, preventing container name collisions.
…ev stages - Add framework-specific devcontainer configurations (VLLM, TRTLLM, SGLANG) - Implement copy_devcontainer.sh script for dynamic devcontainer setup - Add local-dev stages across all Dockerfiles with development tools - Update devcontainer documentation with framework usage guide - Add gitignore patterns for auto-generated devcontainer directories - Update container naming and user setup for development environments Signed-off-by: Keiven Chang <[email protected]>
keivenchang
force-pushed
the
keivenchang/devcontainer-add-sglang-trtllm-frameworks
branch
from
9c8212f to
def73cd
Compare
keivenchang
deleted the
keivenchang/devcontainer-add-sglang-trtllm-frameworks
branch
Overview:
Add framework-specific devcontainer configurations and automation script for VLLM, TRTLLM, and SGLANG frameworks to streamline developer setup across different AI frameworks.
Details:
Where should the reviewer start?
.devcontainer/copy_devcontainer.sh- Review the automation script for framework config generation.devcontainer/README.md- Check the updated documentation for framework usagecontainer/Dockerfile.*- Verify the new local-dev stages in each framework DockerfileRelated Issues:
Summary by CodeRabbit