Skip to content

Conversation

@keivenchang
Copy link
Contributor

@keivenchang keivenchang commented Oct 24, 2025

Overview:

Standardize Dev Container (devcontainer) configuration to use /workspace as the working directory instead of /home/ubuntu/dynamo, for consistency. This simplifies environment variable management and allows pytests (which currently HARD CODES /workspace) to run inside Dev Containers.

Details:

  • Changed workspaceFolder and workspaceMount from /home/ubuntu/dynamo to /workspace across all devcontainer.json files
  • Removed duplicate environment variable definitions (DYNAMO_HOME, CARGO_HOME, RUSTUP_HOME, CARGO_TARGET_DIR) from devcontainer.json, now set by Dockerfile.local_dev
  • Updated post-create.sh to use WORKSPACE_DIR from Dockerfile instead of hardcoded paths
  • Updated rust-analyzer targetDir to /workspace/target for consistency
  • Enhanced container/README.md with detailed environment variable documentation showing how variables flow through build stages

Where should the reviewer start?

Review .devcontainer/devcontainer.json.j2 to see the simplified configuration, then container/README.md for the documentation of environment variables across build stages.

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

/coderabbit profile chill

Summary by CodeRabbit

  • Chores

    • Standardized development container configurations to use unified workspace paths across all variants.
    • Updated build directory and environment variable references for consistency.
    • Improved shell script logic for enhanced path handling.
  • Documentation

    • Enhanced container setup documentation with feature comparison matrix, workflow examples, and environment variable diagrams covering all supported development configurations.

…onfig

- Change devcontainer paths from /home/ubuntu/dynamo to /workspace
- Remove duplicate environment variable definitions
- Simplify post-create.sh to use WORKSPACE_DIR from Dockerfile
- Use standard Docker conventions throughout

Signed-off-by: Keiven Chang <[email protected]>
@keivenchang keivenchang requested review from a team as code owners October 24, 2025 05:34
@keivenchang keivenchang self-assigned this Oct 24, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 24, 2025

Walkthrough

Dev container and build configurations migrated from hardcoded /home/ubuntu/dynamo paths to /workspace. Environment variables removed from containerEnv blocks. Post-create script refactored to use WORKSPACE_DIR. Dockerfile reordered for early HOME definition. Documentation expanded with feature matrix and diagrams.

Changes

Cohort / File(s) Summary
Dev container JSON configurations
.devcontainer/devcontainer.json.j2, .devcontainer/sglang/devcontainer.json, .devcontainer/trtllm/devcontainer.json, .devcontainer/vllm/devcontainer.json
Updated terminal and workspace paths from /home/ubuntu/dynamo to /workspace; Rust analyzer target directory updated to /workspace/target; post-create script path updated to /workspace/.devcontainer/post-create.sh; removed containerEnv block defining DYNAMO_HOME, CARGO_HOME, RUSTUP_HOME, and CARGO_TARGET_DIR
Post-create initialization script
.devcontainer/post-create.sh
Replaced DYNAMO_HOME references with WORKSPACE_DIR; added explicit non-root execution checks and user verification; introduced retry/error handling wrapper; updated pre-commit, cargo target, and installation paths to use WORKSPACE_DIR; expanded .bashrc configuration with environment hygiene checks
Local development Dockerfile
container/Dockerfile.local_dev
Moved ENV HOME definition earlier in build sequence to ensure HOME is set before path-dependent operations; removed redundant later declaration
Container documentation
container/README.md
Added feature matrix comparing development workflows; introduced Dockerfile stage flow and environment variable diagrams; added Key Insights section; expanded usage guidelines and examples for all three workflows

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Changes primarily consist of repetitive path migrations across multiple configuration files and straightforward script refactoring. Although affecting multiple files, the pattern is consistent and the logic density is low. Dockerfile change is minimal, and documentation additions are informational.

Poem

🐰 From dynamo's old nest to workspace bright and new,
We hoppy-footed devs take paths that sparkle true,
With WORKSPACE_DIR as guide, through containers we bound,
Environment clean, the perfect dev ground! 🏗️

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "refactor: Dev Container (devcontainer) to standardize paths to /workspace (for pytest compatibility)" accurately captures the main objective of the changeset. The raw summary confirms that the primary change across all modified files is the standardization of paths from /home/ubuntu/dynamo to /workspace in devcontainer configuration files (.devcontainer/devcontainer.json.j2, sglang/devcontainer.json, trtllm/devcontainer.json, vllm/devcontainer.json), the post-create.sh script, and supporting documentation. The title is specific, concise, and clearly communicates the refactoring objective without vague language or unnecessary details.
Description Check ✅ Passed The pull request description follows the required template structure and includes all critical sections. The Overview section clearly explains the purpose of standardizing the dev container configuration to use /workspace instead of /home/ubuntu/dynamo for consistency and pytest compatibility. The Details section comprehensively lists the specific changes across all affected files, including devcontainer.json files, post-create.sh, Dockerfile.local_dev, and README.md documentation. The "Where should the reviewer start" section appropriately directs reviewers to key files. The only gap is that the Related Issues section is empty with no issue references provided, though this is a non-critical section since the PR content is otherwise well-documented and clear.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@keivenchang keivenchang changed the title refactor: standardize paths to /workspace and simplify devcontainer refactor: Dev Container (devcontainer) to standardize paths to /workspace (for pytest compatibility) Oct 24, 2025
"rust-lang.rust-analyzer"
],
"settings": {
// Disable automatic copying of .gitconfig to avoid errors
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto-generated file, no need to look at it.

"rust-lang.rust-analyzer"
],
"settings": {
// Disable automatic copying of .gitconfig to avoid errors
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto-generated file, no need to look at it.

"rust-lang.rust-analyzer"
],
"settings": {
// Disable automatic copying of .gitconfig to avoid errors
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto-generated file, no need to look at it.

"rust-lang.rust-analyzer"
],
"settings": {
// Disable automatic copying of .gitconfig to avoid errors
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the source template file. Do review it.

"workspaceMount": "source=${localWorkspaceFolder},target=/workspace,type=bind,consistency=cached",
"userEnvProbe": "interactiveShell",
"postCreateCommand": "/bin/bash /home/ubuntu/dynamo/.devcontainer/post-create.sh", // Runs cargo build and pip installs packages
"containerEnv": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all set in Dockerfile.*, so no need to override it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant