feat(sglang): support checkpoint-engine refit - #3519
Conversation
35156f7 to
52d06f9
Compare
GPU follow-upI don't have an environment where SetupSingle node, 2 GPUs, Topology mirrors what the PR introduces:
To exercise ResultEvery weight arrived bit-exact on both ranks, exactly once, per-rank content distinct, across genuinely different transport boundaries (5 vs 6 batches). Caveats
One proposed change: drop an unreachable branchIn if any(finished[rank] and not queue for rank, queue in enumerate(pending)):
raise RuntimeError(
"Checkpoint-engine streams ended with different weights across "
"SGLang ranks."
)
- if any(not queue for queue in pending):
- continue
aligned = [[] for _queue in pending]The reason to delete rather than keep it as a defensive guard: if the invariant ever did break, Environment notes, in case they save someone time
Also happy to pushA property-test module for the alignment logic: randomized per-rank bucket layouts asserting global weight order is preserved, plus a receiver that poisons its recycled buffer to pin down the double-buffer contract in One thing worth confirming, since I could not verify it without a real server: SGLang 0.5.12.post1 asserts |
|
Pushed the property tests I mentioned, plus four more gaps. Each one was verified by mutation — the test fails when the behaviour is broken and passes when it is not — so none of them are assertions that cannot fail:
The buffer one is the reason I wanted this in the tree. The payload-index one is the property with the worst failure mode: SGLang indexes 149 unit tests passing across |
create_weight_synchronizer reads generation.cfg unconditionally, before any backend dispatch. SGLangGeneration stored the generation config only as self.sglang_cfg and defined no cfg attribute, so backend=sglang raised AttributeError there instead of getting its HTTPWeightSynchronizer. VllmGeneration, TRTLLMGeneration and MegatronGeneration all expose cfg; megatron_generation.py documents it as the GenerationInterface contract. sglang_cfg already is that same config object, so alias it with a read-only property rather than keep a second reference that could drift. Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Three gaps found reviewing this branch: - tests/functional/grpo_sglang_nixl_non_colocated.sh was referenced by nothing, so it had never run. The CI guard only checks that L1_Functional*.sh shards appear in the workflow matrix, not that leaf scripts are called. Register it with L1_Functional_Tests_SGLang.sh, which is already in all three matrices. - The only test of _aligned_checkpoint_engine_batches asserted weight names only. The aligner itself enforces name equality across ranks, so that assertion is invariant under any rank permutation. Mutating aligned[rank] -> aligned[len(pending)-1-rank] delivered every shard to the wrong TP rank and the suite stayed green. Assert the tensors too; the mutant now fails. - docs/design-docs/checkpoint-engines.md and docs/guides/checkpoint-engine-refit.md both still said SGLang has no checkpoint-engine refit. Update both, record the actual limits (one node per logical engine, no shard_expert_weights), and generalize the 'Adding Another Backend' timing-line step, which named the vLLM line only. Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Five properties that had no coverage; each was verified by mutation, i.e. the test fails when the corresponding behaviour is broken and passes when it is not. - Recycled receive buffers. nixl.py hands out views into a rotating buffer pool, so a fake engine that allocates fresh tensors every batch makes that entire bug class invisible. _RecyclingEngine poisons a buffer once it is recycled. The aligner is correct today because it only advances a rank whose deque is empty; making it prefetch turns the yielded tensors into NaN. - Multi-dtype batches. NIXL packs buckets by bytes, not dtype, so a mixed bf16/fp32 batch is the normal production shape, but nothing drove more than one dtype group through the update path. - weight_version across refits. Its semantics were pinned only for the first refit, so bumping per POST instead of per refit went undetected. - base_gpu_id remapping. _to_local_gpu_id was stubbed to identity with base_gpu_id=0, which is exactly the case where remapping and doing nothing are indistinguishable. - Payload index to SGLang rank. SGLang indexes serialized_named_tensors by its own TP rank, so a transposed list loads every shard onto the wrong GPU and still reports success. Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
refit.md still stated flatly that non-colocated SGLang generation is not supported, which now contradicts checkpoint-engine-refit.md on the same click (refit.md links to it). The statement is still true for every transport other than checkpoint-engine refit, which factory.py:107-110 rejects, so narrow it rather than delete it, and add SGLang to the NIXL full-weights row. Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
957601d to
c6be7b3
Compare
Two follow-ups on this PR's own changes.
The dp_size=1 guard (weight_sync/factory.py:90) was documented only in
checkpoint-engine-refit.md. design-docs/checkpoint-engines.md still said
'Both are rejected' for what is now three constraints, and refit.md's
constraint table did not mention it at all.
MetricSetupTiming.vllm_checkpoint_engine_init_time_s is no longer written
by anything: grpo.py:1504 moved to extras[f'{backend}_checkpoint_engine_
init_time_s']. For vLLM that formats to the same string, and to_dict()
merges extras over the typed fields, so the emitted metric name is
unchanged -- the field is just dead state now.
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
The factory guarded dp_size and shard_expert_weights but not pp_size. The mixin creates one receiver per engine GPU, while SGLang indexes serialized_named_tensors by TP rank -- and sglang_worker.py:386 asserts tp_size == num_gpus_per_engine // pp_size, so with pp_size>1 the payload list is pp_size times longer than the engine expects. Fail loudly at setup instead. Recorded in all three docs alongside the other two limits. Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
Mirrors the dp_size case immediately above it. Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
What does this PR do?
Adds checkpoint-engine refit for non-colocated SGLang:
The supported envelope is intentionally narrow: one node per logical engine,
sglang_cfg.dp_size=1, andshard_expert_weights=false. Unsupported configurations now fail during setup instead of partway through refit.The branch is rebased on current
main. I also removed a defensivecontinuethat could spin forever if the stream invariant were ever broken; the existing mismatch path now fails loudly.Closes #3288.
Validation
ruffand formatting checks pass after the rebase and follow-up fix0.5.12.post1source: the previously suspectedbegin_weight_updatestate machine is not present in this versionThe remaining acceptance check is the registered GRPO functional test against a real SGLang server in CI; I do not have that environment locally.
#3330 and #3426 overlap with parts of this work. I am happy to keep this standalone or rebase it onto whichever direction you prefer.
cc @RayenTian @yuki-97