[WIP] add EPD#148
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
whybeyoung
left a comment
whybeyoung
left a comment
There was a problem hiding this comment.
Overall, it needs to be verified successfully in the local Kubernetes environment. In addition, it is recommended that the EPD feature support service discovery in the gateway.
| image: lmsysorg/sglang:dev | ||
| command: | ||
| - python3 | ||
| - -m sglang.launch_server |
There was a problem hiding this comment.
need test? maybe sperate it with newline.
- python3
- -m
- sglang.launch_server
....
| command: | ||
| - python3 | ||
| - -m sglang.launch_server | ||
| - --model-path /models/Qwen2.5-VL-7B-Instruct |
| - --model-path /models/Qwen2.5-VL-7B-Instruct | ||
| - --encoder-only | ||
| - --enable-prefix-mm-cache | ||
| - --port 30002 |
| - name: ENCODER_URLS | ||
| value: "http://s-sglang-vlm-epd-vlm-encoder-0:30002 http://s-sglang-vlm-epd-vlm-encoder-1:30002" | ||
| command: | ||
| - sh |
There was a problem hiding this comment.
It is not very reasonable for sh to act as the No.1 process of a container.
| - name: sglang | ||
| image: lmsysorg/sglang:dev | ||
| env: | ||
| - name: ENCODER_URLS |
There was a problem hiding this comment.
For ENCODER_URLS, It seems unreasonable to configure a static Encoder address in prefill—this won't be the way it's used in production, right? Perhaps we should implement its service discovery automatically in gateway mode instead?
| image: lmsysorg/sglang:dev | ||
| command: | ||
| - python3 | ||
| - -m sglang.launch_server |
| limits: | ||
| nvidia.com/gpu: "1" | ||
|
|
||
| - name: sglang-router |
| image: lmsysorg/sglang:dev | ||
| command: | ||
| - python3 | ||
| - -m sglang_router.launch_router |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Hey @liusy58 - checking in on this KEP. It's been about 5 months since @whybeyoung's review. Are you still planning to continue this work?
A few things that need attention before this can move forward:
Incomplete sections:
- The
Benchmarksection referenced in the TOC is empty. Even for a KEP, expected/target numbers would strengthen the proposal. Test Plan(both Integration Tests and End-to-End Tests) is blank. At minimum, outline what you intend to validate.
Previous review feedback unaddressed:
- whybeyoung asked for local Kubernetes verification and service discovery support in the gateway. No follow-up commits since then.
YAML manifest issue in the encoder role:
command:
- python3
- -m sglang.launch_server # <-- single arg with space
- --encoder-onlyIn a Kubernetes pod spec, each list item is a separate exec argument. -m sglang.launch_server as one string won't work - Python expects -m and sglang.launch_server as two separate args. Either split them or wrap in sh -c like you did for the prefill role.
Minor:
- The decode role is missing
--language-only(or equivalent) in the command - is that intentional? The table says decode uses--disaggregation-mode decodebut the YAML doesn't include it either.
Let us know if you'd like to pick this back up or if priorities have shifted. Happy to re-review once updated.
| - --pd-disaggregation | ||
| - --prefill http://s-sglang-vlm-epd-vlm-prefill:30000 | ||
| - --decode http://s-sglang-vlm-epd-vlm-decode:30001 | ||
| - --port 8000 |
There was a problem hiding this comment.
The table of contents references Benchmark, Integration Tests, and End-to-End Tests sections, but the document body ends after the RBG deployment YAML (line 158) without any of those sections present. Even as a WIP, having stub headings keeps the structure navigable and signals what's still planned.
| - name: vlm-encoder | ||
| replicas: 2 | ||
| template: | ||
| spec: |
There was a problem hiding this comment.
The vlm-encoder, vlm-decode, and sglang-router roles use list-style command entries like - -m sglang.launch_server and - --prefill http://.... In a Kubernetes pod spec each list item becomes a separate exec argument, so Python would receive "-m sglang.launch_server" as one token and fail to recognize the -m flag. The vlm-prefill role already handles this correctly with sh -c "...". Consider applying the same sh -c pattern to all roles, or splitting flags and values into individual list items (e.g., - -m / - sglang.launch_server).
|
|
||
| ```yaml | ||
| apiVersion: workloads.x-k8s.io/v1alpha1 | ||
| kind: RoleBasedGroup |
There was a problem hiding this comment.
Compared to KEP-74 (Mooncake Integration), this manifest omits readinessProbes and model volume definitions. Adding at least a TCP socket readinessProbe and a PVC volume mount for the model path would make the example deployment-ready and consistent with existing KEPs.
| - --pd-disaggregation | ||
| - --prefill http://s-sglang-vlm-epd-vlm-prefill:30000 | ||
| - --decode http://s-sglang-vlm-epd-vlm-decode:30001 | ||
| - --port 8000 |
There was a problem hiding this comment.
The KEP template and sibling KEPs include an Alternatives section explaining why other approaches were ruled out. For EPD this could briefly cover why a two-tier PD split (without separate encoders) was insufficient, strengthening the motivation.
4 participants
Ⅰ. Motivation
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.