[MISC] Add routerconfig to runtimes and add pd runtimes#78
Conversation
YouNeedCryDear
requested review from
CatherineSue,
slin1237 and
zhyncs
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @YouNeedCryDear, 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 expands the available model serving runtimes by adding new configurations that support prefill-decode disaggregation for various large language models. It also standardizes and completes the router configurations across all relevant runtimes, ensuring consistent and updated deployment practices. Additionally, some minor Go module dependency cleanup was performed.
Highlights
- New Prefill-Decode Runtimes: Introduced several new
ClusterServingRuntimeconfigurations (e.g.,llama-3-1-405b-instruct-fp8-pd-rt.yaml,mistral-7b-instruct-pd-rt.yaml) specifically designed for prefill-decode disaggregation. These runtimes separate the prefill and decode stages of LLM inference into distinct engine and decoder components, managed by a dedicated router. - Router Image Version Standardization: Updated the
sgl-routerimage version across all existing and newly added runtime configurations to a specific release tag:ghcr.io/moirai-internal/sgl-router:0.1.4.30f2a44. This ensures consistency and stability in the router deployments. - Router Configuration for Existing Runtimes: Added the
routerConfigsection to existingmistral-7b-instruct-rt.yamlandmixtral-8x7b-instruct-rt.yamlruntime definitions, which previously lacked this configuration. This enables these runtimes to properly utilize the router for model serving. - Go Module Dependency Optimization: Cleaned up the Go module dependencies by removing several unused indirect dependencies from
go.modandgo.sum, contributing to a leaner build and reduced dependency footprint.
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 is currently in preview and 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 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 pull request adds several new ClusterServingRuntime configurations for prefill/decode disaggregated setups and updates existing runtimes. The main changes involve pinning router image versions and adding new runtime definitions. My review focuses on ensuring consistency, maintainability, and production-readiness of the new configuration files. Key issues identified include the use of development image tags, inconsistencies in naming conventions and command-line arguments, and significant code duplication in the new runtime definitions.
| periodSeconds: 6 | ||
| initialDelaySeconds: 60 | ||
| timeoutSeconds: 30 | ||
| decoderConfig: |
There was a problem hiding this comment.
The decoderConfig section is almost a complete duplicate of the engineConfig section, with the main difference being the --disaggregation-mode argument. This much duplication can make maintenance difficult. Consider using a tool like Kustomize overlays to define a common base and patch the differences for engine and decoder. This comment applies to all new pd-rt.yaml files.
There was a problem hiding this comment.
Will consider later.
| nvidia.com/gpu: 2 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /health |
There was a problem hiding this comment.
The readiness probe uses /health. For consistency with other new runtime configurations in this PR (e.g., llama-3-1-405b-instruct-fp8-pd-rt.yaml), consider using /health_generate as the health endpoint. This change should also be applied to the liveness and startup probes in this file, and all probes in the decoderConfig for consistency.
path: /health_generate
There was a problem hiding this comment.
will change later.
CatherineSue
left a comment
CatherineSue
left a comment
There was a problem hiding this comment.
Have we tested all these?
| image: ghcr.io/moirai-internal/sgl:dev2 | ||
| image: lmsysorg/sglang:v0.4.8.post1-cu126 | ||
| env: | ||
| - name: SGLANG_MOONCAKE_TRANS_THREAD |
There was a problem hiding this comment.
I think this should be SGLANG_DISAGGREGATION_THREAD_POOL_SIZE in the main branch (or in the lmsysorg/sglang:v0.4.8.post1-cu126)? @whybeyoung @slin1237
YouNeedCryDear
force-pushed
the
feature/add-router-pd-to-rt
branch
from
9ef4ede to
81c7945
Compare
YouNeedCryDear
force-pushed
the
feature/add-router-pd-to-rt
branch
from
81c7945 to
1bb5c6c
Compare
405B failed because of container issue. 90B and 11B failed potentially because of encoder-decoder model architecture. The rest of the models are tested successfully. @CatherineSue |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request adds routerconfig to runtimes and adds pd runtimes. The changes are consistent, updating images to specific versions and using public images where appropriate. The main area for improvement is to use a specific version tag instead of a floating tag for the image to ensure reproducibility and prevent unexpected changes.
slin1237
force-pushed
the
feature/add-router-pd-to-rt
branch
2 times, most recently
from
556e9f0 to
7a98c3c
Compare
slin1237
force-pushed
the
feature/add-router-pd-to-rt
branch
from
7a98c3c to
6fdd662
Compare
* clean up deps * update router image * add router config to sglang runtimes * add pd runtimes * add decoderconfig to pd runtimes * add -pd as suffix to runtime names * use --model-path for consistancy * replace --tp with --tp-size for consistency * use latest sgl image in runtimes * fix image repo * fix ome manager img name * cleanup dep * add pd to runtime kustomize
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add routerconfig to runtimes and add pd runtimes
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?