-
Notifications
You must be signed in to change notification settings - Fork 692
fix: prevent crash looping hello world #2625
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: prevent crash looping hello world #2625
Conversation
b2f76b8 to
d65ae5f
Compare
|
/ok to test d65ae5f |
nealvaidya
left a 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.
LGTM. In future we can replace client with an http listener
yeah a simple fastapi service would be better. this is just a quick fix for now |
WalkthroughThe example client now runs an infinite loop issuing successive streaming requests. Each iteration increments an index embedded in the query string, opens a new stream, prints all received data chunks, then sleeps for one second before repeating. Previously, it performed a single streaming generate call. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant Runtime as Runtime Service
loop Repeat every 1s
rect rgba(200,230,255,0.25)
Note over Client: Build request with incremented idx
Client->>Runtime: Open streaming request "Query[idx] world,sun,moon,star"
activate Runtime
Runtime-->>Client: Stream chunk(s)
Runtime-->>Client: [end of stream]
deactivate Runtime
Note over Client: Print all received data
end
Client->>Client: sleep(1s)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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: 1
🧹 Nitpick comments (2)
examples/runtime/hello_world/client.py (2)
41-42: Parameterize request cadence to reduce log spam and allow tuning.Hardcoding
sleep(1)is fine for a demo, but making it configurable via an env var improves UX in clusters with stricter rate limits.Add this outside the shown block:
# imports import os # at top-level # before the loop, near idx initialization period = float(os.getenv("HELLO_WORLD_PERIOD_SEC", "1.0"))Then change the sleep to:
await asyncio.sleep(period)
38-40: Ensure the generated stream is always closed on errorsOur inspection of the Python bindings shows that
client.generate(...)returns a bare async generator (no__aenter__/__aexit__support), so usingasync withwon’t work. To avoid leaking the underlying TCP/NATS connection if an exception occurs during iteration, wrap the loop in atry/finallyand callawait stream.aclose().• examples/runtime/hello_world/client.py (around lines 38–41)
Suggested diff:
- stream = await client.generate(f"Query[{idx}] world,sun,moon,star") - async for response in stream: - print(response.data()) + stream = await client.generate(f"Query[{idx}] world,sun,moon,star") + try: + async for response in stream: + print(response.data()) + finally: + # ensure the async generator is closed to free network resources + await stream.aclose()
📜 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)
examples/runtime/hello_world/client.py(1 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). (2)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - dynamo
Signed-off-by: Hannah Zhang <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: nnshah1 <[email protected]>
Overview:
Prevent crashloops in helloworld k8s example
nvbug: https://nvbugspro.nvidia.com/bug/5471412
closes: DYN-926
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit