Repository navigation
chore(deps): bump SGLang to 0.5.16 - #1970
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR upgrades SGLang Docker and CI versions to ChangesSGLang version update
LoadSnapshot migration
gRPC request compatibility
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GetLoads
participant GrpcRequestManager
participant LoadSnapshotReader
Client->>GetLoads: request load sections and optional dp_rank
GetLoads->>GrpcRequestManager: get_loads(include, dp_rank)
GrpcRequestManager->>LoadSnapshotReader: read_all()
LoadSnapshotReader-->>GrpcRequestManager: LoadSnapshot list
GrpcRequestManager-->>GetLoads: filtered snapshots
GetLoads-->>Client: protobuf load response
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean dependency bump — version strings are consistent, removed workarounds are confirmed fixed upstream in 0.5.16, and mooncake tracks the upstream CI pin. No issues found.
Note: the PR description is missing the ## Changes header per the template (see comment).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
029c09d to
5af047b
Compare
github-actions
Bot
added
dependencies
| # 0.5.13 is the first release with the shmem load snapshots that replaced | ||
| # the WatchLoadUpdateReq piggyback path (removed upstream in v0.5.14). | ||
| sglang = ["sglang>=0.5.13"] | ||
| sglang = ["sglang>=0.5.16"] |
There was a problem hiding this comment.
🟡 Nit: The comment still explains the old >=0.5.13 floor (shmem load snapshots replacing WatchLoadUpdateReq), but the floor is now >=0.5.16 because the GetLoadsReqInput/Output IPC was removed upstream. Consider updating the comment to reflect the current reason.
| # 0.5.13 is the first release with the shmem load snapshots that replaced | |
| # the WatchLoadUpdateReq piggyback path (removed upstream in v0.5.14). | |
| sglang = ["sglang>=0.5.13"] | |
| sglang = ["sglang>=0.5.16"] | |
| # 0.5.16 removed the GetLoadsReqInput/Output IPC the servicer previously | |
| # used; the port to LoadSnapshot snapshot readers requires this floor. | |
| sglang = ["sglang>=0.5.16"] |
![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: 5af047bfa1
ℹ️ 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".
|
|
||
| def _convert_loads_to_protobuf( | ||
| result: GetLoadsReqOutput, | ||
| result: LoadSnapshot, |
There was a problem hiding this comment.
Now that result is a LoadSnapshot, the disaggregation branch below still reads result.disaggregation.prefill_prealloc_queue_reqs. In SGLang 0.5.16 the snapshot's DisaggregationMetrics exposes prefill_bootstrap_queue_reqs instead, so any GetLoads request that includes disaggregation metrics (the default all does) will raise AttributeError and return an INTERNAL error instead of load data whenever PD/disaggregation metrics are present.
Useful? React with 👍 / 👎.
5af047b to
c99cbf4
Compare
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
c99cbf4 to
b06f4bc
Compare
c28ce8c to
2a8afcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@grpc_servicer/smg_grpc_servicer/sglang/servicer.py`:
- Around line 100-101: Update the LoadSnapshot-to-SchedulerLoad mapping in the
surrounding servicer method to read the disaggregation requests from
result.prefill_bootstrap_queue_reqs instead of
result.prefill_prealloc_queue_reqs. Preserve the existing response construction
and other field mappings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b6f08e6-98bf-4ba3-a1c0-fadb7676f392
📒 Files selected for processing (7)
.github/workflows/release-sglang-docker.ymlgrpc_servicer/pyproject.tomlgrpc_servicer/smg_grpc_servicer/sglang/request_manager.pygrpc_servicer/smg_grpc_servicer/sglang/servicer.pygrpc_servicer/smg_grpc_servicer/sglang/utils.pygrpc_servicer/tests/test_token_id_array.pyscripts/ci_install_sglang.sh
| result: LoadSnapshot, | ||
| ) -> sglang_scheduler_pb2.SchedulerLoad: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n "prefill_(prealloc|bootstrap)_queue_reqs" \
grpc_servicer/smg_grpc_servicer/sglang/servicer.py
curl -fsSL \
https://raw.githubusercontent.com/sgl-project/sglang/v0.5.16/python/sglang/srt/managers/load_snapshot.py |
rg -n "prefill_(prealloc|bootstrap)_queue_reqs"Repository: lightseekorg/smg
Length of output: 299
🏁 Script executed:
sed -n '140,160p' grpc_servicer/smg_grpc_servicer/sglang/servicer.pyRepository: lightseekorg/smg
Length of output: 1231
Use prefill_bootstrap_queue_reqs here — result.disaggregation is mapped with prefill_prealloc_queue_reqs, but SGLang v0.5.16 exposes prefill_bootstrap_queue_reqs on LoadSnapshot. Disaggregation responses will fail with AttributeError until this field name is updated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@grpc_servicer/smg_grpc_servicer/sglang/servicer.py` around lines 100 - 101,
Update the LoadSnapshot-to-SchedulerLoad mapping in the surrounding servicer
method to read the disaggregation requests from
result.prefill_bootstrap_queue_reqs instead of
result.prefill_prealloc_queue_reqs. Preserve the existing response construction
and other field mappings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a8afcfc39
ℹ️ 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".
| tokenized_req.need_wait_for_mm_inputs = bool(encode_req.need_wait_for_mm_inputs) | ||
| tokenized_req.num_items_assigned = encode_req.num_items_assigned |
There was a problem hiding this comment.
When encoder disaggregation uses the gRPC servicer with dynamic encoder URLs, SGLang 0.5.16 freezes the selected encoder list on encode_req.encoder_urls, and the scheduler later indexes num_items_assigned against tokenized_req.encoder_urls (falling back to the current list only for legacy callers). This copy only forwards the wait flag and assignment counts, so if an encoder registers or unregisters between dispatch and scheduler processing, the counts can be applied to a different URL ordering, sending image shards to the wrong or missing encoders and causing the request to fail or wait until timeout. Copy encode_req.encoder_urls onto tokenized_req here as well.
Useful? React with 👍 / 👎.
c1487d4 to
74a2c53
Compare
0.5.16 pins kernels<0.15 and nvidia-cutlass-dsl 4.6.0 itself, so the CI
kernels band pin and the cutlass#3259 forced reinstall are dropped.
Mooncake follows upstream 0.5.16 CI to 0.3.11.post1; release-docker base
images move to lmsysorg/sglang:v0.5.16.
0.5.16 also drops the legacy get_loads IPC (sglang#30525), so the gRPC
servicer's load bridge is ported to the published LoadSnapshot reader:
the request manager reads per-dp-rank snapshots (TokenizerManager role)
instead of round-tripping GetLoadsReq through a communicator, include-
section filtering moves gateway-side, and the sglang extra floor rises
to >=0.5.16.
0.5.16 further rewrites the IPC structs as kw-only msgspec Structs:
input_embeds and token_type_ids become required on
TokenizedGenerateReqInput, TokenizedEmbeddingReqInput renames
image_inputs to mm_inputs, the scheduler consumes mm_inputs via
MultimodalInputs.from_processor_output (so the servicer now hands it a
MultimodalProcessorOutput), slotted structs reject undeclared attributes
(need_wait_for_image was never a real field; the EPD hook now sets
need_wait_for_mm_inputs), and Req concatenates origin_input_ids with
array("q") output_ids, making the array token-id contract mandatory —
to_token_id_array always emits array("q") and its legacy-list env toggle
is removed. 0.5.16's SamplingParams.normalize also rejects string
stop sequences when called without a tokenizer; the scheduler matches
them with its own tokenizer (skip_tokenizer_init is never set here), so
the servicer now loads the model tokenizer exactly as TokenizerManager
does and passes it to normalize instead of tokenizer=None. ProfileReq's type field is renamed
req_type.
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
74a2c53 to
05c0cfd
Compare
|
|
||
| def _convert_loads_to_protobuf( | ||
| result: GetLoadsReqOutput, | ||
| result: LoadSnapshot, |
There was a problem hiding this comment.
🔴 Important: The type changed from GetLoadsReqOutput → LoadSnapshot, but the disaggregation mapping below (line 152) still reads result.disaggregation.prefill_prealloc_queue_reqs. SGLang 0.5.16 renamed this field to prefill_bootstrap_queue_reqs on DisaggregationMetrics. Any GetLoads request that includes disaggregation metrics will raise AttributeError.
Fix on line 152:
prefill_prealloc_queue_reqs=result.disaggregation.prefill_bootstrap_queue_reqs,
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05c0cfdacc
ℹ️ 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".
| fail-fast: false | ||
| matrix: | ||
| base_image: ${{ github.event_name == 'push' && fromJSON('["lmsysorg/sglang:v0.5.12.post1"]') || fromJSON(format('["{0}"]', inputs.base_image_ref || 'lmsysorg/sglang:v0.5.12.post1')) }} | ||
| base_image: ${{ github.event_name == 'push' && fromJSON('["lmsysorg/sglang:v0.5.16"]') || fromJSON(format('["{0}"]', inputs.base_image_ref || 'lmsysorg/sglang:v0.5.16')) }} |
There was a problem hiding this comment.
For push and default manual runs this matrix value is passed as BASE_IMAGE_REF to _build-engine-image.yml, but the current lmsysorg/sglang tag list publishes v0.5.16 as suffixed CUDA/ROCm runtime tags such as v0.5.16-cu129-runtime/v0.5.16-cu130-runtime, not the bare v0.5.16 tag. These release runs will fail when Docker tries to pull the base image, so the default should point at a published runtime tag.
Useful? React with 👍 / 👎.
1 participant
Description
Problem
CI and release images pin SGLang 0.5.12.post1; upstream is at 0.5.16. Two of our installer workarounds were explicitly waiting on a release that has now shipped — and 0.5.16 makes two breaking changes the gRPC servicer was built on: it removes the legacy
get_loadsIPC (sglang#30525), which the first e2e run surfaced as every gRPC worker dying at import, and it rewrites the scheduler IPC structs as kw-only msgspec Structs, which the second e2e run surfaced as warmup requests failing construction (Missing required argument 'input_embeds').Solution
scripts/ci_install_sglang.sh:sglang[all]==0.5.16. Dropped two now-obsolete workarounds, both confirmed against the 0.5.16 wheel metadata: the forcednvidia-cutlass-dsl-libs-cu13==4.5.2reinstall (0.5.16 pinsnvidia-cutlass-dsl[cu13]==4.6.0, which carries the cutlass#3259 fix the comment was waiting for) and thekernels>=0.14.1,<0.15band pin (0.5.16 pinskernels<0.15,>=0.14.1itself). Torch stays 2.11.0/CUDA-13, so the nvcc-13 setup is unchanged.mooncake-transfer-engine-cuda13==0.3.11.post1, tracking upstream v0.5.16 CI (scripts/ci/cuda/ci_install_dependency.sh); tracking links updated.release-sglang-docker.yml: base image matrix/defaults/examples →lmsysorg/sglang:v0.5.16(tag verified on Docker Hub).LoadSnapshot: schedulers now publish per-dp-rank snapshots (SHM, zmq for multi-node DP) unconditionally, so the request manager creates acreate_load_snapshot_reader(..., caller="TokenizerManager")andget_loadsreads published snapshots instead of round-trippingGetLoadsReqInput/Outputthrough a communicator.LoadSnapshotcarries the identical fields the proto conversion already reads, soGetLoadsRPC wire behavior is unchanged; include-section filtering (previously scheduler-side) now happens in the manager with the same validation error. Theget_loads_communicatoris removed (including from the public communicator set — the 0.5.16 HTTP sidecar's /v1/loads uses snapshot readers too). Reader is closed on shutdown.TokenizedGenerateReqInputconstruction now passes the newly requiredinput_embeds/token_type_ids;TokenizedEmbeddingReqInput'simage_inputsis renamedmm_inputs; the scheduler consumesmm_inputsviaMultimodalInputs.from_processor_output(...), so_parse_mm_inputsreturns aMultimodalProcessorOutputinstead of a pre-builtMultimodalInputs; slotted structs reject undeclared attributes, so the EPD encode hook sets the realneed_wait_for_mm_inputsfield (need_wait_for_imagewas never declared — the old assignment was a silent no-op that msgspec turns into anAttributeError); andReqnow concatenatesorigin_input_idswitharray("q")output ids, making the array token-id contract mandatory —to_token_id_arrayalways emitsarray("q")and its legacy-list env toggle (SGLANG_GRPC_TOKEN_ID_ARRAY) is removed. Additionally, 0.5.16'sSamplingParams.normalizerejects string stop sequences when called without a tokenizer. The scheduler matches stop strings with its own tokenizer (skip_tokenizer_initis never set in this deployment), so the servicer now loads the model tokenizer exactly as sglang'sTokenizerManagerdoes and passes it tonormalize— engine-side early stopping andfinish_reason="stop"semantics are preserved (andstop_str_max_lenis now computed in real tokens rather than the tokenizer-less character approximation); andProfileReq.typeis renamedreq_type. Wire format is unaffected: 0.5.16 defaults to pickle IPC (SGLANG_USE_PICKLE_IPC=True), matching the manager's existingsend_pyobj.grpc_servicer's sglang extra floor rises to>=0.5.16(the old IPC no longer exists, and the previous>=0.5.13floor was above what CI installed anyway).Breaking change
smg-grpc-servicernow requires SGLang >= 0.5.16 and no longer runs against older engines; upgrade SGLang and the servicer together (pip enforces the floor). Suggested release-note entry:Test Plan
bash -nclean; 0.5.16 wheel metadata verified for the dropped workarounds;v0.5.16image tag verified; grpc_servicer python suite green after the port (77 passed, 3 skipped); everyTokenizedGenerateReqInput/TokenizedEmbeddingReqInputconstruction site statically checked against the 0.5.16 wheel's struct definitions (no missing required fields, no unknown kwargs).Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
/v1/loadsbehavior.GetLoadsendpoint documentation and response mapping accordingly.