Repository navigation
Conversation
- Add --connection-mode {grpc,http} argument (default: grpc)
- sglang supports both grpc and http modes
- vllm and trtllm only support grpc (error if http requested)
- Move health_check and worker_url to base WorkerLauncher class
- Add grpcio and grpcio-health-checking dependencies
- Remove unused dependencies (uvicorn, fastapi, aiohttp, orjson)
- Update README with serve command documentation
github-actions
Bot
added
documentation
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughRefactors Python bindings: README expanded; pyproject switches HTTP stack to gRPC deps; serve logic updated to accept a connection mode and host/port/data-parallel-size args, converting health_check and worker_url to concrete, arg-aware implementations and propagating connection_mode through worker launch and tests. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI / User"
participant Orch as "ServeOrchestrator"
participant Launcher as "WorkerLauncher"
participant Worker as "Worker (gRPC / HTTP)"
participant Health as "Health Endpoint"
CLI->>Orch: parse args (--connection-mode, --host, --port, --data-parallel-size)
Orch->>Launcher: launch worker(s) with args, host, port
Launcher->>Worker: start process (backend-specific)
par health-check
Launcher->>Health: perform health_check(args, host, port)
Health-->>Launcher: healthy / unhealthy
end
Launcher-->>Orch: worker URLs (built using args.connection_mode)
Orch-->>CLI: report ready / worker URLs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
I think vLLM and TrtLLM may not have the standard grpc health check implemented yet |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the smg serve command to use a unified --connection-mode argument, which is a great improvement for consistency. The changes correctly move shared logic for health checks and worker URL generation into the base WorkerLauncher class, reducing code duplication. The argument parsing is also made more robust. My review includes a couple of suggestions for the README.md file to improve the accuracy and completeness of the documentation for both users and contributors.
yes, the code fall back to channel being ready |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@bindings/python/pyproject.toml`:
- Around line 30-34: The pyproject declares Python 3.8 but the unpinned grpcio
and grpcio-health-checking now require Python >=3.9; either update the package
metadata to drop 3.8 (set requires-python to ">=3.9" and remove 3.8 from
classifiers) OR keep 3.8 support by pinning the dependencies in the dependencies
list to versions below 1.71.0 (e.g., change "grpcio" and
"grpcio-health-checking" entries to version-constrained strings like
grpcio<1.71.0 and grpcio-health-checking<1.71.0) so installs on Python 3.8
succeed.
In `@bindings/python/README.md`:
- Around line 38-48: Update the --connection-mode table entry to correctly state
backend restrictions: clarify that `--connection-mode` supports `grpc` (default)
and `http` for the `sglang` backend, but `vllm` and `trtllm` backends enforce
`grpc`-only (attempting `http` will error); reference the option names
`--connection-mode`, `--backend`, and backend values `sglang`, `vllm`, `trtllm`
so readers can see which backends allow HTTP vs which require gRPC.
In `@bindings/python/src/smg/serve.py`:
- Around line 319-335: The current CLI flag definition for "--host" sets
default="0.0.0.0" which exposes the router; change the default to "127.0.0.1"
(or "localhost") in the add_argument call that defines "--host" so the service
binds to localhost by default, and update the corresponding help string to
instruct users how to opt into public binding (e.g., "--host 0.0.0.0") if
required; locate the "--host" add_argument invocation in serve.py and modify the
default and help text accordingly.
- Drop Python 3.8 support (grpcio 1.71.0+ requires Python ≥3.9) - Fix README: clarify vllm/trtllm only support grpc (not sglang) - Default --host to 127.0.0.1 for security (avoid exposing router) - Update tests for new API: - dp_size → data_parallel_size - grpc_mode → connection_mode - health_check/worker_url now take args as first parameter - Default connection mode is grpc
…#335) Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
2 participants
Summary
--connection-mode {grpc,http}argument (default:grpc) for unified connection mode across backendshealth_checkandworker_urlto baseWorkerLauncherclass to reduce duplicationgrpcioandgrpcio-health-checkingdependencies for proper gRPC health checksuvicorn,fastapi,aiohttp,orjson)smg servecommand documentationTest plan
smg serve --backend sglangwith default grpc modesmg serve --backend sglang --connection-mode httpsmg serve --backend vllm(grpc only)--backend vllm --connection-mode httpSummary by CodeRabbit
Documentation
New Features
Chores
Tests