Repository navigation
feat support max_total_num_tokens in getserverinforesponse to keep align with get_server_info in HTTP server - #817
Conversation
Summary of ChangesHello, 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 enhances the gRPC server's information reporting by introducing a new field to its server info response. This addition ensures consistency with the HTTP server's capabilities, providing a more comprehensive view of the server's token capacity across different interfaces. The update facilitates better resource management and client-side decision-making by making crucial configuration details accessible via gRPC. Highlights
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. Footnotes
|
📝 WalkthroughWalkthroughAdded an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
Hi @Huixxi, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
…ign with get_server_info in HTTP server Signed-off-by: 桓希 <huxiguo.hxg@taobao.com>
![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
The pull request introduces the max_total_num_tokens field to the GetServerInfoResponse protobuf message, which is a positive step towards aligning the gRPC server information with the HTTP server's capabilities. However, for this feature to be fully functional and provide accurate data, the corresponding Python servicer's GetServerInfo method needs to be updated to populate this newly added field. Without this update, gRPC clients will receive a default value (0), potentially leading to misleading server information.
| // Server metadata | ||
| string server_type = 8; // "grpc" | ||
| google.protobuf.Timestamp start_time = 9; | ||
| int32 max_total_num_tokens = 10; |
There was a problem hiding this comment.
The addition of max_total_num_tokens to GetServerInfoResponse is a good step towards aligning gRPC server information with the HTTP server. However, please ensure that the corresponding Python servicer's GetServerInfo method (in grpc_servicer/smg_grpc_servicer/sglang/servicer.py) is updated to populate this new field with the correct value. Currently, gRPC clients will receive the default value (0), which might lead to incorrect information being reported.
81708c0 to
b3b73d1
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the SGLang scheduler gRPC protobuf to include max_total_num_tokens in GetServerInfoResponse, aligning the gRPC server-info payload with the token-capacity information available elsewhere (e.g., loads snapshots / HTTP server info).
Changes:
- Add
max_total_num_tokensfield toGetServerInfoResponseinsglang_scheduler.proto.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81708c0573
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Server metadata | ||
| string server_type = 8; // "grpc" | ||
| google.protobuf.Timestamp start_time = 9; | ||
| int32 max_total_num_tokens = 10; |
There was a problem hiding this comment.
max_total_num_tokens before advertising it
The schema now exposes max_total_num_tokens, but the only SGLang gRPC implementation I inspected still never sets it: grpc_servicer/smg_grpc_servicer/sglang/servicer.py:491-500 builds GetServerInfoResponse without this field. Because proto3 defaults unset int32 fields to 0, every GetServerInfo reply from the gRPC server will report a bogus token limit until the servicer is updated, which breaks any client that starts using this new field for capacity/context-length decisions.
Useful? React with 👍 / 👎.
| // Server metadata | ||
| string server_type = 8; // "grpc" | ||
| google.protobuf.Timestamp start_time = 9; | ||
| int32 max_total_num_tokens = 10; |
There was a problem hiding this comment.
This API change is not propagated to the repository's Go SDK. bindings/golang/internal/proto/sglang_scheduler.pb.go:2820-2931 still defines GetServerInfoResponse with fields only through start_time, so Go clients built from this commit will silently drop field 10 on decode and have no accessor for it. If this proto stays source-controlled, the generated Go files need to be refreshed in the same change.
Useful? React with 👍 / 👎.
…ign with get_server_info in HTTP server 2 Signed-off-by: 桓希 <huxiguo.hxg@taobao.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6b65b6bed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sglang_version=sglang.__version__, | ||
| server_type="grpc", | ||
| start_time=start_timestamp, | ||
| max_total_num_tokens=self.scheduler_info.get("max_total_num_tokens", 0), |
There was a problem hiding this comment.
If the scheduler bootstrap dict does not contain max_total_num_tokens—a case the same startup path already anticipates in grpc_servicer/smg_grpc_servicer/sglang/server.py:81-85 by falling back to server_args.context_length or 8192—GetServerInfo will now advertise 0 for the new field. Any client that starts using this field for capacity/context-length decisions will treat that worker as having no usable context, even though model_info.max_context_length from the same process remains non-zero.
Useful? React with 👍 / 👎.
|
@slin1237 hi~ whenever you have some time, could you please take a look at this PR? Thanks! |
Description
Problem
Add the max_total_num_tokens keyword in gRCP mode's GetServerInfoResponse to keep align with the origin get_server_info in HTTP server's response.
Solution
Changes
Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit