-
Notifications
You must be signed in to change notification settings - Fork 770
chore: split frontend + runtime launch message to accurately represent container purpose #4537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…iner purpose Signed-off-by: Tushar Sharma <tusharma@nvidia.com>
Signed-off-by: Tushar Sharma <tusharma@nvidia.com>
WalkthroughMultiple Dockerfiles are refactored to reference launch message files from a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
container/Dockerfile(1 hunks)container/Dockerfile.frontend(1 hunks)container/Dockerfile.sglang(1 hunks)container/Dockerfile.trtllm(1 hunks)container/Dockerfile.vllm(1 hunks)container/launch_message/frontend.txt(1 hunks)container/launch_message/runtime.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-30T20:43:49.632Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 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/launch_message/frontend.txtcontainer/Dockerfilecontainer/Dockerfile.trtllmcontainer/Dockerfile.vllmcontainer/Dockerfile.frontendcontainer/Dockerfile.sglang
📚 Learning: 2025-09-03T01:10:12.599Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 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/Dockerfilecontainer/Dockerfile.trtllmcontainer/Dockerfile.vllmcontainer/Dockerfile.sglang
📚 Learning: 2025-09-16T17:16:07.820Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 3051
File: container/templates/Dockerfile.vllm.j2:221-233
Timestamp: 2025-09-16T17:16:07.820Z
Learning: In the dynamo project, when converting Dockerfiles to Jinja2 templates, the primary goal is backward compatibility - generated Dockerfiles must be identical to the originals. Security improvements or other enhancements that would change the generated output are out of scope for templating PRs and should be addressed separately.
Applied to files:
container/Dockerfile.trtllmcontainer/Dockerfile.vllmcontainer/Dockerfile.frontendcontainer/Dockerfile.sglang
📚 Learning: 2025-08-30T20:43:10.091Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.091Z
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.
Applied to files:
container/Dockerfile.trtllmcontainer/Dockerfile.vllmcontainer/Dockerfile.frontendcontainer/Dockerfile.sglang
📚 Learning: 2025-08-30T20:43:10.091Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 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.trtllmcontainer/Dockerfile.vllmcontainer/Dockerfile.frontendcontainer/Dockerfile.sglang
📚 Learning: 2025-11-14T01:09:35.244Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 4323
File: components/src/dynamo/vllm/main.py:197-205
Timestamp: 2025-11-14T01:09:35.244Z
Learning: In components/src/dynamo/vllm/main.py, keivenchang considers temporary directory cleanup from tempfile.mkdtemp() to be LOW PRIORITY for production because containerized deployment patterns automatically clean up temp directories when containers are destroyed, mitigating resource leak concerns.
Applied to files:
container/Dockerfile.vllm
🪛 LanguageTool
container/launch_message/frontend.txt
[grammar] ~44-~44: Use a hyphen to join words.
Context: ...@@@ @@@ ® Dynamo: A Datacenter Scale Distributed Inference Serving Fram...
(QB_NEW_EN_HYPHEN)
[grammar] ~52-~52: Ensure spelling is correct
Context: ...HTTP API service - Preprocessor - Router - Endpont Picker (EPP) for Gateway API Inference ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (4)
- GitHub Check: trtllm (arm64)
- GitHub Check: operator (arm64)
- GitHub Check: operator (amd64)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (7)
container/Dockerfile.frontend (1)
43-43: Aligns with PR objective to separate frontend launch messaging.The path update correctly references the new frontend-specific launch message file while maintaining the same destination.
container/Dockerfile.sglang (1)
367-368: Consistent with runtime container refactoring.The launch message source correctly references runtime.txt, aligned with other backend runtime Dockerfiles.
container/Dockerfile.trtllm (1)
339-340: Consistent with multi-backend runtime container consolidation.The launch message now correctly sources from runtime.txt, consistent with other backend containers (vllm, sglang).
container/Dockerfile (1)
424-425: Primary dev image correctly updated to use runtime launch message.The base dev entrypoint now sources from runtime.txt, maintaining consistency across the container image family.
container/Dockerfile.vllm (1)
299-300: vLLM runtime stage correctly updated to use runtime.txt.The launch message source now references runtime.txt, aligned with other backend container updates.
container/launch_message/runtime.txt (1)
51-51: Multi-backend runtime command appropriately updated.The brace expansion syntax
{vllm,sglang,trtllm}correctly represents the multiple backend options available in runtime containers. This aligns with the PR's goal of unifying the runtime launch message.container/launch_message/frontend.txt (1)
46-73: Frontend launch message well-structured and distinct from runtime.The new file provides clear separation between frontend container messaging and runtime container messaging. Content is comprehensive, including description, components, benefits, and quick-start examples. This addresses the PR objective of accurate container-purpose representation.
Signed-off-by: Tushar Sharma <tusharma@nvidia.com>
Signed-off-by: Tushar Sharma <tusharma@nvidia.com>
Co-authored-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com> Signed-off-by: Tushar Sharma <tusharma@nvidia.com>
Signed-off-by: Tushar Sharma <tusharma@nvidia.com>
…t container purpose (#4537) Signed-off-by: Tushar Sharma <tusharma@nvidia.com> Co-authored-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
…t container purpose (ai-dynamo#4537) Signed-off-by: Tushar Sharma <tusharma@nvidia.com> Co-authored-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
…t container purpose (ai-dynamo#4537) Signed-off-by: Tushar Sharma <tusharma@nvidia.com> Co-authored-by: Harrison Saturley-Hall <hsaturleyhal@nvidia.com>
Overview:
Resolves (5680875)
Given that we are shipping a new frontend-only container, we need a new launch message to reflect the purpose/usage of the container when accessing via bash. This PR creates a new container/launch_message directory which contains different messages for the various containers that we support. For now, we've created 2 seperate launch messages, one named frontend.txt and one named runtime.txt. We can revisit this later and maintain one launch_message and override accordingly based on container purpose but that's a change for later.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.