Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @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 significantly refactors the SGLang router's worker management by introducing a dedicated Data Parallel (DP) worker abstraction. This change aims to decouple DP-aware logic from the core router, making the system more modular and extensible. It allows any worker type to be DP-aware and centralizes the handling of data parallel ranks within the new DPAwareWorker and updated WorkerFactory.
Highlights
- Enhanced Worker Trait: The
Workertrait has been extended with new methods (is_dp_aware,base_url,dp_rank,dp_size,prepare_request,endpoint_url,can_handle) to support Data Parallel (DP) awareness, providing default implementations for non-DP-aware workers. - New DPAwareWorker Implementation: A new
DPAwareWorkerstruct has been introduced, which wraps existing worker types (Regular, Prefill, Decode) and automatically injectsdata_parallel_rankinto requests, centralizing DP-specific logic. - WorkerFactory Enhancements: The
WorkerFactorynow includes asynchronous methods (create_dp_aware,create_dp_aware_regular_workers,create_dp_aware_prefill_workers,create_dp_aware_decode_workers) for creating DP-aware workers, and the generalcreate_workersmethod has been updated to utilize these new factory methods.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This is an excellent refactoring that introduces a DPAwareWorker abstraction to encapsulate data-parallel logic, which is a great improvement for modularity and maintainability. The changes are well-structured and the addition of comprehensive unit tests is fantastic.
My review focuses on a few areas for further improvement, mainly around performance and code duplication. I've suggested parallelizing worker creation, reusing HTTP clients to improve efficiency, and refactoring some repetitive logic. Overall, great work!
There was a problem hiding this comment.
While the current check for dp_size overflow is correct, using usize::try_from is a more idiomatic and robust way to handle fallible numeric conversions in Rust. It clearly expresses the intent of a conversion that might fail and is safer across different architectures (e.g., 32-bit vs. 64-bit).
usize::try_from(dp_size).map_err(|_| WorkerError::InvalidConfiguration {
message: format!("dp_size {} is too large for this architecture", dp_size),
})|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
1 similar comment
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Dose it support kv cache aware? |
Motivation
The current SGLang router implementation has DP-aware (Data Parallel aware) logic throughout the Router code. To enable DP Aware routing in PD mode. A abstraction is required. The DP-aware functionality allows workers to handle data-parallel routing by injecting
data_parallel_rankinto requests, but this logic is currently hardcoded in the router layer.This PR introduces a clean Worker abstraction that encapsulates DP-aware functionality, allowing:
Modifications
1. Worker Trait
is_dp_aware()- Check if worker supports data-parallel routingbase_url()- Get base URL without DP rank suffixdp_rank()/dp_size()- Get DP configurationprepare_request()- Transform requests for DP-aware routingendpoint_url()- Get the actual endpoint URL for requests2. New DPAwareWorker Implementation
DPAwareWorkerstruct that wraps any worker type with DP-aware capabilitiesdata_parallel_rankinto requests@dp_ranksuffix for identification3. WorkerFactory
create_dp_aware()- Create individual DP-aware worker of any typecreate_dp_aware_regular_workers()- For regular routercreate_dp_aware_prefill_workers()- For PD router prefill workerscreate_dp_aware_decode_workers()- For PD router decode workerscreate_workers()to use the new DP-aware factory methodsChecklist