Repository navigation
Bump sglang to v0.5.20 - #3321
Conversation
Zhichenzzz
added
run-ci-fsdp
run-ci-megatron
run-ci-image
run-ci-sglang
run-ci-lora
run-ci-true-on-policy
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Run 35427410660 —
Next: new run on 03d7bcd667; GPU stages start once |
03d7bcd to
5a2b2da
Compare
|
@claude review always |
Zhichenzzz
added
the
run-on-blackwell
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
examples/infra_features/random_async/sglang_config_qwen3_5_35b_1p1d.yaml— This example's decode-engine override silently stops taking effect after the sglang v0.5.20 bump, so anyone running this config gets the default cuda-graph max batch size instead of 128 with no error. The yaml still setscuda_graph_max_bs: 128underoverrides:, which flows straight into ServerArgs kwargs in miles/backends/sglang_utils/sglang_engine.py, but v0.5.20 renamed the field tocuda_graph_max_bs_decode(mirroring the--sglang-cuda-graph-max-bs->--sglang-cuda-graph-max-bs-decodeCLI rename this PR applies everywhere else). Becausecuda_graph_max_bsis no longer inmsgspec.structs.fields(ServerArgs), sglang_engine.py'sunused_keyscompat fallback (lines 180-191) silently pops it and only logs at info level. …Extended reasoning...
…Fix: update every raw ServerArgs-field override (not just CLI-flag docs/scripts) to the new field name, i.e. rename this key to
cuda_graph_max_bs_decode, and audit other yaml/json sglang override configs for the same stale-name-silently-dropped pattern.The PR's own rename sweep updated every
--sglang-cuda-graph-max-bsCLI usage across docs/scripts/tests to--sglang-cuda-graph-max-bs-decode, and examples/infra_features/p2p_weight_transfer/run.py:967 emits the new flag name too. But examples/infra_features/random_async/sglang_config_qwen3_5_35b_1p1d.yaml:36 sets the raw ServerArgs field namecuda_graph_max_bs(not a CLI flag) under the decode server_group'soverrides:. This dict is documented in sglang_config.py:30 as 'ServerArgs field overrides' and is merged intokwargsin sglang_engine.py's_compute_server_argsviakwargs.update(sglang_overrides)(line ~178). The subsequent loop at lines 180-186 iteratesmsgspec.structs.fields(ServerArgs)and discards recognized names fromunused_keys; since the real field is nowcuda_graph_max_bs_decode,cuda_graph_max_bs…Verification: normal (regression introduced by this bump). examples/infra_features/random_async/sglang_config_qwen3_5_35b_1p1d.yaml:36 still sets
cuda_graph_max_bs: 128in the decode server_groupoverrides:. That dict flows into kwargs via sglang_engine.py:177 (kwargs.update(sglang_overrides)); then _compute_server_args iteratesmsgspec.structs.fields(ServerArgs)and any override key not matching a…
All CPU lanes green on 0b09c13f6d (run 35428928664): Three more v0.5.20 API shifts surfaced after the first comment, each its own commit:
On the two review findings:
Two sweeps run to stop discovering these one CI round at a time, both clean:
The CPU suite also runs clean against the v0.5.20 tree on an H200 devbox (v0.5.20 source, sgl-kernel 0.4.7), which is where these were triaged before pushing. |
Zhichenzzz
added
the
run-on-hopper
Zhichenzzz
removed
run-on-hopper
Point the image at lmsysorg/sglang:v0.5.20 and install the rebased sglang-miles-v0.5.20 stack (54 commits on v0.5.20; the v0.5.19 stack is frozen as sglang-miles-v0.5.19-final). torch 2.13.0, transformers 5.12.1, apache-tvm-ffi 0.1.11 and nvidia-cutlass-dsl 4.6.2 are unchanged between the two releases, so the wheel tags and the tvm-ffi reconcile step stay as they are.
sglang v0.5.20 turned ServerArgs from a dataclass into a msgspec Struct, so dataclasses.fields(ServerArgs) raises TypeError. The engine launch, the P2P server-args reconstruction and the argv round-trip check all enumerated fields that way; the argv helper is shared with RouterArgs, which is still a dataclass, so it dispatches on the record type.
sglang v0.5.20 (sgl-project/sglang#38375) dropped the deprecated aliases that still had a replacement, so every launcher passing them fails argument parsing. Rewrite the five that miles still emitted, everywhere they appear (scripts, examples, docs, e2e tests and the launch-script snapshots): --sglang-nsa-prefill-backend -> --sglang-dsa-prefill-backend --sglang-nsa-decode-backend -> --sglang-dsa-decode-backend --sglang-mamba-scheduler-strategy -> --sglang-mamba-radix-cache-strategy --sglang-cuda-graph-max-bs -> --sglang-cuda-graph-max-bs-decode --sglang-cuda-graph-bs -> --sglang-cuda-graph-bs-decode
sglang v0.5.20 adds _radix_eviction_policy_explicitly_set, set during resolution and declared Arg(no_cli=True) like the speculative-draft quantization marker, so the per-field CLI round-trip test xfails it the same way.
The production helper enumerates ServerArgs fields with msgspec.structs.fields now that sglang v0.5.20 made the record a Struct, so a dataclass stub raises TypeError before the test reaches its assertions.
… image's sglang branch The argv round-trip helper is on the light worker entrypoint's import path, whose module footprint tests/fast/utils/workers/import_probe.py pins, so it imports msgspec only on the Struct branch. docker/build.py's cu12-x86 variant pins a v0.5.19 base image but inherited the Dockerfile's new SGLANG_BRANCH default, which would build the v0.5.20 stack against a v0.5.19 base. sglang publishes no cu12 image from v0.5.20 on, so the variant is pinned to the frozen v0.5.19 branch instead.
The test pins prime==0.6.19, but prime's own requirement is prime-sandboxes>=0.2.32 with no upper bound. prime-sandboxes 0.3.0 (2026-09-18) removed CommandRequest, which the pinned prime imports, so 'prime env install' dies with an ImportError. main's nightly has been red on this test since that release.
e430bf2 to
524d09f
Compare
There was a problem hiding this comment.
Of the three issues raised in my prior reviews, the two Dockerfile/build.py branch-pin and test-fixture issues are now fixed by the latest commits. The sglang_engine.py:181 / p2p_transfer_utils.py:182 finding (hardcoded msgspec.structs.fields(ServerArgs)) remains open and unaddressed: docker/build.py now explicitly freezes the cu12-x86 variant to sglang-miles-v0.5.19-final, whose ServerArgs is still a stdlib dataclass, so those two call sites will raise TypeError on every engine launch and P2P weight-update session built from that image. A human should confirm this before merge, since the dev-cu12 image is built automatically on every push to docker/Dockerfile and on the nightly schedule.
Extended reasoning...
Overview
This run's diff (d4137b9..HEAD) is the same sglang v0.5.20 bump PR I have reviewed six times before, now with four additional commits pushed since my last review at 2026-09-19T11:44:17Z: a fix pinning docker/build.py's cu12-x86 variant to SGLANG_BRANCH=sglang-miles-v0.5.19-final (addressing the earlier Dockerfile ABI-mismatch finding), a fix making the P2P test's fake ServerArgs a real msgspec.Struct (addressing the earlier test-fixture-mismatch finding), a fix generalizing argv_utils.py's field-enumeration helper to dispatch on dataclasses.is_dataclass() vs Struct, and an unrelated test-env change capping prime-sandboxes.
Security risks
None identified — this is version-pin and API-adaptation code with no auth, crypto, or data-exposure surface.
Level of scrutiny
High, because this PR touches the docker image build (affecting every downstream deployment) and a previously-flagged inline finding remains genuinely unresolved rather than fixed. I re-verified from the current code (not just the resolution metadata) that miles/backends/sglang_utils/sglang_engine.py:181 and miles/backends/training_utils/weight_update/protocols/p2p_transfer_utils.py:182 still call msgspec.structs.fields(ServerArgs) unconditionally, and confirmed via docker/Dockerfile's build recipe (the SGLANG_BRANCH checkout is pip install -e'd as the runtime sglang package) that the newly-frozen cu12-x86 variant will import a dataclass-based ServerArgs there, which is incompatible with the hardcoded msgspec call. This inline thread (posted 2026-09-19T11:44:17Z) is not marked resolved in the timeline, unlike the other two, so it is a live, currently-open finding — not something I am second-guessing or withdrawing.
Other factors
Per the review protocol, two of three prior findings being fixed while one remains open means I should focus this update only on what remains unresolved rather than restate resolved items or stay fully silent, and I should not approve while an unaddressed finding (mine or otherwise) is outstanding.
Still open from earlier reviews (1):
- 🔴
miles/backends/sglang_utils/sglang_engine.py:181—Building the cu12-x86 docker variant now makes every sglang engine launch crash with TypeError, a regression the base b…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
The bump branch tracked sglang-miles-v0.5.20 while it was under review; sgl-project's sglang-miles is fast-forwarded to that same tip (5a8da8cc3f) as this merges, so the default branch name is correct again.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
Still open from earlier reviews (1):
- 🔴
miles/backends/sglang_utils/sglang_engine.py:181—Building the cu12-x86 docker variant now makes every sglang engine launch crash with TypeError, a regression the base b…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
2 participants
ci-sglang-pr: sglang-miles-v0.5.20
https://github.com/sgl-project/sglang/tree/sglang-miles-v0.5.20
Rebases the
sglang-milesstack ontov0.5.20and repoints the miles image at it.sglang side
sglang-miles-v0.5.20= 54 commits onv0.5.20, replayed fromsglang-miles(the v0.5.19 stack: 55 numbered + 6 unnumbered commits, frozen assglang-miles-v0.5.19-final@c9660a7e11). 727 upstream commits of drift; 21 of the 57 picks conflicted.Dropped (already in v0.5.20 or superseded):
Keep NVFP4 blockscale swizzle padding on device (#39142)— byte-identical to upstream #39141.state_capturer: pin the exact host-cache size via mmap + cudaHostRegister (#36562)— a strict subset of upstream #37285, which also adds thedestroy()hooks.dsv4: gate the padded-head path on attn_tp_size, not tp_size— v0.5.20'sMQALayer.forwardno longer has that block (_kernel_num_heads()/_local_attn_sink(kernel_num_heads)replaced it).[38] Restore MqaAttentionBase.maybe_use_decode_attn_tp— only re-added two imports that the Nemotron commit had removed; that removal is gone (below), so the commit became empty.Remove obsolete PD retract guard (#39119)squashed into[8/54] Improve PD pause handling, as its subject asked.Notable conflict decisions. v0.5.20 split
ServerArgsintoarg_groups/fields/*.py(choices inarg_groups/choices.py, noNS()markers;field_order.pyis a compat record, new fields sort last), removed the DSA prefill in-sequence-split CP path, moved the DSA prefill split-ops intodsa_prefill_cuda_graph.py, and made the tokenizer dispatch path async with its owndispatched/abort_sentbookkeeping. Our behaviour was re-expressed on those shapes:[1]):true_on_policy_contract,enable_prefill_only_deterministic_inferencelive infields/exec_.py,RL_ON_POLICY_TARGET_CHOICESinchoices.py; the decode CUDA-graph capture keeps upstream'sget_exec().graphflag inside our prefill-only deterministic patch;qwen3_moekeeps upstream's empty-batch guard around our true-on-policy routing branch.[11]): upstream dropped the unused intermediate buffers andweight_indices_long; we keep only the quant-agnostic device lookup and merge upstream's padded-replay-row clearing with our gathered-length mapping.[15]):return_raw_indicesis threaded into the realDSAIndexerMetadataindsa_indexer_metadata.py. The stack used to carry a secondDSAIndexerMetadataclass indsa_backend.pythat shadowed the real one (and lacked upstream'soutput_size=host-sync fix); it is gone. The CP variant_get_topk_ragged_with_cphad nothing left to attach to and was dropped.[18]): arefresh_attn_sink_cache()that recopies the per-rank slice into upstream's cached padded sink; upstream's_kernel_num_headsstructure is kept as is.[21]): merged with upstream'sabort_sentdedupe and the encoderdispatch_readyevents; a pre-dispatch abort also releases the encoder wait.[32]): the lease release is added to upstream's new handler-failure cleanup only on the non-dispatched branch (dispatched requests get it from the scheduler's abort echo).[36]):needs_engine_info_bootstrap()/registers_parallelism_config()onServerArgs, the two fields infields/model.py,handle_rdt_weight_syncthroughrun_hookin the pipeline;RayEnginekeeps upstream's Ray stat loggers inside our placement-group contextvar.[49],[54]):register_lora_adapterrebuilt withget_lora()/get_serving()accessors;_evict_lru_over_caplikewise.Stale duplicates removed from the stack while rebasing (pre-existing warts from earlier rebases, all shadowing the maintained upstream copy):
dsa_indexer.pyforward_npu/do_npu_cp_balance_indexer(theDSANPUIndexerMixinhas them), module-levelpcg/bcg_dsa_indexer_prefill_split+scattered_to_tp_attn_fullcopies;loader.py_post_load_weightsre-add (the stack renames it topost_load_weights, and the v0.5.20 call site upstream added now uses that name);deepseek_v2.py/glm4_moe.pydetermine_num_fused_shared_expertsandglm4_moe.pyget_input_embeddingscopies; the Nemotron commit's compensating edits todeepseek_v4.pythat no longer apply.glm4_moe_nextnandfused_moeread their flags throughget_exec()like the rest of v0.5.20.New upstream pre-commit hook
check-registered-testsrequires newly added tests to live attest/registered/<kind>/<subsystem>/(unit = CPU-only, est_time ≤ 60). Moved:test_dense_deterministic_math,test_on_policy_wiring→unit/true_on_policy/;test_tp_invariant_ops→e2e/true_on_policy/;test_parallelism_context_integration→e2e/disaggregation/(plussys.exit(pytest.main(...))for the bare-pytest hook);test_distributed_weight_update_spec_worker→unit/rl/;test_moe_lora_tail_stamp→e2e/lora/;test_weight_checker_mxfp4_marlin→e2e/rl/.Every commit passes pre-commit individually (isort / ruff / ruff-format amendments folded in);
ruffF821/F401 is zero across every file the stack touches (the three remaining F811 redefinitions are upstream's own). Authors preserved from the v0.5.19 stack.miles side
docker/Dockerfile:SGLANG_IMAGE_TAG=v0.5.20,SGLANG_BRANCH=sglang-miles-v0.5.20(to be switched back tosglang-milesat merge time, aftersglang-milesis fast-forwarded to the v0.5.20 tip). torch 2.13.0, transformers 5.12.1, apache-tvm-ffi 0.1.11, nvidia-cutlass-dsl 4.6.2 and the/opt/sglangvenv layout are unchanged between v0.5.19 and v0.5.20, so the wheel tags, theSGLANG_BUILD_RUST_EXTS=noneeditable install, the tvm-ffi reconcile step and the cu12pyproject.tomlsed patterns stay as they are.sglang-kernelmoves 0.4.6.post1 → 0.4.7 (comes with the base image).release-docker.ymlmatrix; the upstream Dockerfile dropped CUDA 12.9), so there is nov0.5.20-cu129fordocker/build.py'scu12-x86variant. It is left atv0.5.19-cu129in this PR, which after merge pairs a v0.5.19 base (sglang-kernel 0.4.6.post1) with the v0.5.20 python tree on the nightlydev-cu12build. Either drop the variant or keep it pinned to the last cu12 stack; decision needed before merge.Validation
sglang-miles-v0.5.20): pending, tracked below.