-
Notifications
You must be signed in to change notification settings - Fork 689
fix: hello world deployment liveness probe and match message responses with readme #2634
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
fix: hello world deployment liveness probe and match message responses with readme #2634
Conversation
2dabc02 to
b09fc30
Compare
|
/ok to test b09fc30 |
WalkthroughThe client request payload in examples/runtime/hello_world/client.py was changed from an index-based string to a fixed string. The deployment YAML adds spec.backendFramework: vllm, adjusts HelloWorldWorker readinessProbe to a no-op command, and removes log piping from the container args. Changes
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
|
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: 0
🧹 Nitpick comments (2)
examples/runtime/hello_world/deploy/hello_world.yaml (2)
60-68: Readiness probe always returns success; consider tying readiness to actual process health.Using
exit 0makes the pod "ready" even when the worker hasn’t started correctly, which can mask startup/config issues. If the worker doesn’t expose a health endpoint (typical for a non-serving worker), a lightweight readiness check that the Python process is running is safer.Apply this diff to make readiness reflect process availability:
- - "exit 0" + - 'ps -ef | grep -q "[p]ython3 hello_world.py" || exit 1'Note: if your base image lacks
ps, usepgrep -finstead, or drop readiness entirely for the worker if it’s not used for traffic gating.
87-87: Log flushing: use unbuffered Python for timely container logs.Without the previous
tee, stdout buffering can delay logs. Running Python unbuffered improves observability.- - python3 hello_world.py + - python3 -u hello_world.py
📜 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 (2)
examples/runtime/hello_world/client.py(1 hunks)examples/runtime/hello_world/deploy/hello_world.yaml(3 hunks)
⏰ 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)
examples/runtime/hello_world/deploy/hello_world.yaml (1)
9-9: CRD enum “vllm” is valid
Verified that both the GraphDeployment and ComponentDeployment CRDs (operator and Helm charts) list “vllm” exactly as a permitted enum value forbackendFramework. No casing or enum mismatches remain—this change can be approved.examples/runtime/hello_world/client.py (1)
43-43: Static prompt matches README — OK.Change aligns with the docs and is fine. Keeping
idxfor error context is still useful even if it’s not in the prompt.
|
@biswapanda Could you update the title? I'm not sure what "hello world yaml and messages" means. The title of the PR becomes the commit message, so it should be something we can read in 3 months and understand what's in the commit. |
fixed the deployment |
Signed-off-by: Hannah Zhang <[email protected]>
Signed-off-by: Jason Zhou <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: nnshah1 <[email protected]>
Overview:
nvbug: https://nvbugspro.nvidia.com/bug/5471412
closes: DYN-926
Summary by CodeRabbit
New Features
Chores