fix: small qol improvements to devcontainer #1427
Conversation
alec-flowers
requested review from
a team,
ishandhanani,
nnshah1,
ptarasiewiczNV,
richardhuo-nv,
rmccorm4 and
tanmayv25
as code owners
alec-flowers
changed the title
WalkthroughThe updates enhance the development environment configuration and documentation. The README gains sections on development flow and documentation. The devcontainer settings are improved for Rust development and streamline Python linting. The post-create script now uses Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 1
🧹 Nitpick comments (4)
container/Dockerfile.vllm (1)
311-311: Pinmaturininstallation to improve build reproducibility
Installingmaturin[patchelf]without a version can lead to unexpected upgrades. Consider adding--no-cache-dirand pinning to a known version to ensure consistent builds.- RUN uv pip install maturin[patchelf] + RUN uv pip install --no-cache-dir maturin[patchelf]==<desired_version>.devcontainer/README.md (2)
56-71: Specify fenced code block languages and correct typo
The new “Development Flow” section is clear, but the fenced blocks lack language identifiers and there’s a spelling error in “propogate”. Addbashafter the opening backticks and correct “propogate” → “propagate”. Consider adding commas in compound sentences (e.g., after “GitHub”).@@ -60,7 +60,7 @@ - ``` + ```bash cd /home/ubuntu/dynamo && cargo build --locked --profile dev@@ -68,7 +68,7 @@
@@ -66,7 +66,7 @@
- If you make changes to rust code and want to propogate to python bindings then can use [maturin]...
- If you make changes to Rust code and want to propagate to Python bindings, then use [maturin]...
<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [uncategorized] ~58-~58: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short). Context: ...t Flow If you make changes to rust code and you want to compile, use [cargo build](... (COMMA_COMPOUND_SENTENCE) --- [uncategorized] ~64-~64: Possible missing comma found. Context: ...profile dev ``` Before pushing code to github remember to run `cargo fmt` and `cargo ... (AI_HYDRA_LEO_MISSING_COMMA) --- [uncategorized] ~66-~66: Possible missing comma found. Context: ...st code and want to propogate to python bindings then can use [maturin](https://www.matu... (AI_HYDRA_LEO_MISSING_COMMA) </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 60-60: Fenced code blocks should have a language specified null (MD040, fenced-code-language) --- 68-68: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `99-105`: **Add language tag to the code block** The “Documentation” section’s code block should declare its language for proper syntax highlighting. ```diff @@ -102,4 +102,4 @@ - ``` + ```bash cd /home/ubuntu/dynamo/.build/target/doc && python3 -m http.server 8000 - ``` + ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
102-102: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
.devcontainer/devcontainer.json (1)
45-52: Tunerust-analyzerperformance settings
Allocating 4 GB and enablingcheckOnSaveandautoimportgreatly improves the Rust DX. Verify that this memory limit fits within container resource quotas, and consider exposing feature flags via"rust-analyzer.cargo.features".
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.devcontainer/README.md(2 hunks).devcontainer/devcontainer.json(1 hunks).devcontainer/post-create.sh(1 hunks)container/Dockerfile.vllm(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.devcontainer/README.md
[uncategorized] ~58-~58: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...t Flow If you make changes to rust code and you want to compile, use [cargo build](...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~64-~64: Possible missing comma found.
Context: ...profile dev ``` Before pushing code to github remember to run cargo fmt and `cargo ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~66-~66: Possible missing comma found.
Context: ...st code and want to propogate to python bindings then can use [maturin](https://www.matu...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~106-~106: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... vscode will automatically port-forward and you can check them out in your browser....
(COMMA_COMPOUND_SENTENCE)
🪛 markdownlint-cli2 (0.17.2)
.devcontainer/README.md
60-60: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
68-68: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
102-102: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (2)
.devcontainer/post-create.sh (2)
61-63: Adoptmaturin developfor Python bindings
Switching tomaturin developaligns with the updated flow. Includingretryis helpful for flaky network installs. Optionally, document why--releaseisn’t used here if production-grade bindings are needed.
70-84: Robust virtual-env activation logic
The conditional block correctly avoids duplicate entries, handles deactivation, and uses a single-quoted heredoc to prevent expansion. The grep guard ensures idempotency.
Co-authored-by: Ryan McCormick <rmccormick@nvidia.com> Signed-off-by: Alec <35311602+alec-flowers@users.noreply.github.com>
Co-authored-by: Ryan McCormick <rmccormick@nvidia.com> Signed-off-by: Alec <35311602+alec-flowers@users.noreply.github.com>
Co-authored-by: Ryan McCormick <rmccormick@nvidia.com> Signed-off-by: Alec <35311602+alec-flowers@users.noreply.github.com>
Co-authored-by: Ryan McCormick <rmccormick@nvidia.com> Signed-off-by: Alec <35311602+alec-flowers@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
.devcontainer/README.md (6)
58-59: Improve clarity and capitalization in Rust instruction
The sentence can be streamlined and capitalize “Rust”.- If you make changes to rust code and you want to compile, use [cargo build](https://doc.rust-lang.org/cargo/commands/cargo-build.html). This will update rust binaries such as dynamo-run. + If you make changes to Rust code and want to compile, use [cargo build](https://doc.rust-lang.org/cargo/commands/cargo-build.html). This will update Rust binaries such as dynamo-run.🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...t Flow If you make changes to rust code and you want to compile, use [cargo build](...(COMMA_COMPOUND_SENTENCE)
60-63: Specify language for fenced code block
Addbashafter the opening backticks to enable proper syntax highlighting.- ``` - cd ~/dynamo && cargo build --locked --profile dev - ``` + ```bash + cd ~/dynamo && cargo build --locked --profile dev + ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
60-60: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
64-64: Capitalize GitHub and add missing comma
Improve readability by capitalizing “GitHub” and adding a comma.- Before pushing code to github remember to run `cargo fmt` and `cargo clippy` + Before pushing code to GitHub, remember to run `cargo fmt` and `cargo clippy`🧰 Tools
🪛 LanguageTool
[uncategorized] ~64-~64: Possible missing comma found.
Context: ...profile dev ``` Before pushing code to github remember to runcargo fmtand `cargo ...(AI_HYDRA_LEO_MISSING_COMMA)
66-70: Fix spelling, capitalization, punctuation, and specify code block language
- Correct “propogate” → “propagate”
- Capitalize “Rust” and “Python”
- Add missing commas
- Add
bashto code fence- If you make changes to rust code and want to propogate to python bindings then can use [maturin](https://www.maturin.rs/#usage) (pre-installed). This will update the python bindings with your new rust changes. - - ``` - cd ~/dynamo/lib/bindings/python && maturin develop - ``` + If you make changes to Rust code and want to propagate to Python bindings, you can use [maturin](https://www.maturin.rs/#usage) (pre-installed). This will update the Python bindings with your new Rust changes. + + ```bash + cd ~/dynamo/lib/bindings/python && maturin develop + ```🧰 Tools
🪛 LanguageTool
[uncategorized] ~66-~66: Possible missing comma found.
Context: ...st code and want to propogate to python bindings then can use [maturin](https://www.matu...(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
68-68: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
102-104: Specify language for documentation code block
Addbashafter the opening backticks for the doc serve example.- ``` - cd ~/dynamo/.build/target/doc && python3 -m http.server 8000 - ``` + ```bash + cd ~/dynamo/.build/target/doc && python3 -m http.server 8000 + ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
102-102: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
106-106: Capitalize and punctuate VS Code reference
Use the correct “VS Code” casing and add a comma for clarity.- vscode will automatically port-forward and you can check them out in your browser. + VS Code will automatically port-forward, and you can check them out in your browser.🧰 Tools
🪛 LanguageTool
[uncategorized] ~106-~106: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... vscode will automatically port-forward and you can check them out in your browser....(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.devcontainer/README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
.devcontainer/README.md
[uncategorized] ~58-~58: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...t Flow If you make changes to rust code and you want to compile, use [cargo build](...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~64-~64: Possible missing comma found.
Context: ...profile dev ``` Before pushing code to github remember to run cargo fmt and `cargo ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~66-~66: Possible missing comma found.
Context: ...st code and want to propogate to python bindings then can use [maturin](https://www.matu...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~106-~106: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... vscode will automatically port-forward and you can check them out in your browser....
(COMMA_COMPOUND_SENTENCE)
🪛 markdownlint-cli2 (0.17.2)
.devcontainer/README.md
60-60: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
68-68: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
102-102: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.devcontainer/README.md (1)
58-63: Acknowledge cargo build instructions implementedThis aligns with previous guidance to build Rust binaries via
cargo build --locked --profile dev.🧰 Tools
🪛 LanguageTool
[style] ~58-~58: Consider shortening or rephrasing this to strengthen your wording.
Context: ...on github. ## Development Flow If you make changes to Rust code and want to compile, use [car...(MAKE_CHANGES)
🧹 Nitpick comments (3)
.devcontainer/README.md (3)
64-65: Capitalize GitHub and add missing commaAdd a comma after “GitHub” and correct its capitalization:
-Before pushing code to Github, remember to run `cargo fmt` and `cargo clippy` +Before pushing code to GitHub, remember to run `cargo fmt` and `cargo clippy`
66-71: Fix typo in “propogate” and improve clarityCorrect “propogate” → “propagate” and streamline the instruction:
-If you make changes to Rust code and want to propogate to Python bindings then can use [maturin](https://www.maturin.rs/#usage) (pre-installed). This will update the Python bindings with your new Rust changes. +After updating Rust code, run [maturin](https://www.maturin.rs/#usage) in the Python bindings directory to propagate changes: + +```bash +cd /home/ubuntu/dynamo/lib/bindings/python && maturin develop +```🧰 Tools
🪛 LanguageTool
[style] ~66-~66: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cargo fmtandcargo clippyIf you make changes to Rust code and want to propogate to Pyth...(MAKE_CHANGES)
[uncategorized] ~66-~66: Possible missing comma found.
Context: ...st code and want to propogate to Python bindings then can use [maturin](https://www.matu...(AI_HYDRA_LEO_MISSING_COMMA)
102-105: Specify language for fenced code block and unify pathAdd a language specifier (
bash) and use the absolute path to match other examples:-``` -cd ~/dynamo/.build/target/doc && python3 -m http.server 8000 -``` +```bash +cd /home/ubuntu/dynamo/.build/target/doc && python3 -m http.server 8000 +```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
102-102: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.devcontainer/README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
.devcontainer/README.md
[style] ~58-~58: Consider shortening or rephrasing this to strengthen your wording.
Context: ...on github. ## Development Flow If you make changes to Rust code and want to compile, use [car...
(MAKE_CHANGES)
[style] ~66-~66: Consider shortening or rephrasing this to strengthen your wording.
Context: ... cargo fmt and cargo clippy If you make changes to Rust code and want to propogate to Pyth...
(MAKE_CHANGES)
[uncategorized] ~66-~66: Possible missing comma found.
Context: ...st code and want to propogate to Python bindings then can use [maturin](https://www.matu...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~106-~106: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... VSCode will automatically port-forward and you can check them out in your browser....
(COMMA_COMPOUND_SENTENCE)
🪛 markdownlint-cli2 (0.17.2)
.devcontainer/README.md
102-102: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - vllm
alec-flowers
force-pushed
the
aflowers/dev-container-improvements
branch
from
0080652 to
8bc6843
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.devcontainer/README.md (4)
20-20: Add comma for compound sentence.Use a comma before “and” to separate the independent clauses for clarity:
- Warning: devcontainers is an experimental feature and we are not testing in CI. + Warning: devcontainers is an experimental feature, and we are not testing in CI.🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...devcontainers is an experimental feature and we are not testing in CI. Please submit...(COMMA_COMPOUND_SENTENCE)
54-54: Unify warning style.Consider using a blockquote for this note to match the existing warning on line 20:
- If `post-create.sh` fails, you can try to debug or [submit](https://github.com/ai-dynamo/dynamo/issues) an issue on GitHub. + > If `post-create.sh` fails, you can try to debug or [submit](https://github.com/ai-dynamo/dynamo/issues) an issue on GitHub.
66-66: Add comma and clarity.Insert a comma after “bindings” and add “you” for smoother flow:
- If you make changes to Rust code and want to propagate to Python bindings then can use [maturin] + If you make changes to Rust code and want to propagate to Python bindings, then you can use [maturin]🧰 Tools
🪛 LanguageTool
[style] ~66-~66: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cargo fmtandcargo clippyIf you make changes to Rust code and want to propagate to Pyth...(MAKE_CHANGES)
[uncategorized] ~66-~66: Possible missing comma found.
Context: ...st code and want to propagate to Python bindings then can use [maturin](https://www.matu...(AI_HYDRA_LEO_MISSING_COMMA)
106-106: Add comma for readability.Use a comma before “and” to separate the independent clauses:
- VSCode will automatically port-forward and you can check them out in your browser. + VSCode will automatically port-forward, and you can check them out in your browser.🧰 Tools
🪛 LanguageTool
[uncategorized] ~106-~106: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... VSCode will automatically port-forward and you can check them out in your browser....(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.devcontainer/README.md(3 hunks)
🧰 Additional context used
🪛 LanguageTool
.devcontainer/README.md
[uncategorized] ~20-~20: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...devcontainers is an experimental feature and we are not testing in CI. Please submit...
(COMMA_COMPOUND_SENTENCE)
[style] ~58-~58: Consider shortening or rephrasing this to strengthen your wording.
Context: ...on GitHub. ## Development Flow If you make changes to Rust code and want to compile, use [car...
(MAKE_CHANGES)
[style] ~66-~66: Consider shortening or rephrasing this to strengthen your wording.
Context: ... cargo fmt and cargo clippy If you make changes to Rust code and want to propagate to Pyth...
(MAKE_CHANGES)
[uncategorized] ~66-~66: Possible missing comma found.
Context: ...st code and want to propagate to Python bindings then can use [maturin](https://www.matu...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~106-~106: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... VSCode will automatically port-forward and you can check them out in your browser....
(COMMA_COMPOUND_SENTENCE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - vllm
Overview
Small quality of life improvements to the devcontainer setup to enhance the development experience for both Rust and Python development workflows.
Details
Documentation Improvements
.devcontainer/README.mdwith clear instructions for:DevContainer Configuration Enhancements
Build Process Improvements
maturin developinstead ofuv pip install -e .for Python bindingsVIRTUAL_ENVis set but activation script hasn't been sourcedContainer Updates
maturin[patchelf]to ensure proper shared library linkingThese changes provide a smoother development experience with better tooling support, clearer documentation, and more robust build processes.
Summary by CodeRabbit
Documentation
Chores