fix: deploy readme changes based on customer feedback #2645
Conversation
athreesh
requested review from
atchernych,
biswapanda,
hhzhang16,
julienmancuso and
mohammedabdulwahhab
WalkthroughDocumentation update to the Dynamo deploy guide: replaces a fixed namespace example with a placeholder, adds a new “Container Images” section covering public images and custom builds, and notes planned defaults for example YAMLs. No code or runtime logic changes. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/guides/dynamo_deploy/README.md (1)
118-126: Align dynamoNamespace across Frontend and Worker.The Frontend uses my-llm while the worker uses dynamo-dev. These must match for service discovery; mixing values can cause the Frontend to miss workers.
Apply:
- dynamoNamespace: my-llm + dynamoNamespace: my-llm @@ - VllmDecodeWorker: # or SGLangDecodeWorker, TrtllmDecodeWorker + VllmDecodeWorker: # or SGLangDecodeWorker, TrtllmDecodeWorker @@ - dynamoNamespace: dynamo-dev + dynamoNamespace: my-llmAlso consider a brief note clarifying that dynamoNamespace is a logical Dynamo namespace (not the Kubernetes metadata.namespace).
🧹 Nitpick comments (6)
docs/guides/dynamo_deploy/README.md (6)
45-45: Use the lowercase resource kind (and likely plural) for kubectl.CRD resource types are conventionally lowercase (and usually pluralized) when used with kubectl. Consider updating this line; verify your CRD’s plural before committing.
Apply this edit if your CRD plural is “dynamographdeployments”:
-kubectl get dynamoGraphDeployment -n ${NAMESPACE} +kubectl get dynamographdeployments -n "${NAMESPACE}"If your CRD registered a different plural or shortname, substitute accordingly (e.g., kubectl get dgds).
52-63: Tighten the “Container Images” edits and resolve CI lint warning.
- The pipeline flagged trailing whitespace in this block (lines 53–63). Run pre-commit locally to ensure all fixes are committed.
- Nice addition, but the current YAML shows three image keys that, if copied verbatim, would be invalid YAML. Suggest a “pick one” snippet with others commented.
You can update the example as follows:
-```yaml -# In your deployment YAML: -image: nvcr.io/nvidia/ai-dynamo/vllm-runtime:0.4.0 # vLLM -image: nvcr.io/nvidia/ai-dynamo/sglang-runtime:0.4.0 # SGLang -image: nvcr.io/nvidia/ai-dynamo/tensorrtllm-runtime:0.4.0 # TensorRT-LLM -``` +```yaml +# In your deployment YAML, set ONE image; comment the others. +# vLLM +image: nvcr.io/nvidia/ai-dynamo/vllm-runtime:0.4.0 +# or SGLang +# image: nvcr.io/nvidia/ai-dynamo/sglang-runtime:0.4.0 +# or TensorRT-LLM +# image: nvcr.io/nvidia/ai-dynamo/tensorrtllm-runtime:0.4.0 +```Also consider specifying the actual field path most users will edit, e.g., spec.services..extraPodSpec.mainContainer.image, to reduce ambiguity.
54-55: Clarify where the image field lives in the CRD.To reduce confusion, reference the precise field path users should edit (it appears to be under extraPodSpec.mainContainer.image in later examples).
Apply:
-Before deploying, you'll need to specify which container images to use in the CRD. You have several options: +Before deploying, you'll need to set the container image at spec.services.<ServiceName>.extraPodSpec.mainContainer.image in your CRD. You have several options:
66-76: Include guidance for private registries/NGC auth and imagePullSecrets.Many users will need to authenticate to nvcr.io or a private registry. Adding a short note and snippet prevents failed pulls.
Apply:
# Tag and push to your registry docker tag dynamo-runtime:latest your-registry/dynamo-runtime:latest docker push your-registry/dynamo-runtime:latest + +## If your registry requires auth (e.g., nvcr.io or a private registry): +# Create a pull secret in the target namespace: +# kubectl create secret docker-registry my-regcred \ +# --docker-server=<registry> --docker-username=<user> --docker-password=<token> \ +# -n "${NAMESPACE}" +# +# Reference it in your CRD: +# extraPodSpec: +# imagePullSecrets: +# - name: my-regcred
78-78: Avoid recommending :latest tags for production.Pinning versions or digests is safer and reproducible; :latest can lead to surprise upgrades and broken rollouts.
Apply:
-> **Note**: We're working to update all example YAMLs to default to public images with `:latest` tags for easier deployment. +> Note: For reliability and security, prefer pinning images to a specific version (e.g., :0.4.0) or digest (e.g., @sha256:...). We'll keep examples pinned and document an upgrade path to newer tags.
130-131: Verify GPU resource key.Kubernetes typically uses nvidia.com/gpu for device plugin resources. If your operator maps gpu: "1" internally, keep it; otherwise, switch to nvidia.com/gpu: "1".
📜 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 (1)
docs/guides/dynamo_deploy/README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/guides/dynamo_deploy/README.md
[grammar] ~66-~66: There might be a mistake here.
Context: ...orRT-LLM ``` Option 2: Build Your Own For customization or private deployments...
(QB_NEW_EN)
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/2645/merge) by athreesh.
docs/guides/dynamo_deploy/README.md
[error] 53-63: Pre-commit trailing-whitespace hook failed. Trailing whitespace detected on lines 53-63 and fixed by the hook.
⏰ 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). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
docs/guides/dynamo_deploy/README.md (2)
39-39: Good change: make namespace explicit and user-provided.Replacing the fixed namespace with a placeholder reduces copy/paste mistakes across clusters and aligns with the “use the platform’s namespace” guidance above.
48-49: Double-check service name in port-forward example.Ensure the Service is actually named agg-vllm-frontend in components/backends/vllm/deploy/agg.yaml; mismatches here are a common stumbling block for first-time users.
| For customization or private deployments, build containers from source: | ||
| ```bash | ||
| # Build from source |
There was a problem hiding this comment.
Should we point to container args to specify framework (vllm, sglang, trtllm) and target (runtime for prod >> I think dev for dev version)
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR has been closed due to inactivity. If you believe this PR is still relevant, please feel free to reopen it with additional context or information. |
Overview:
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit