vLLM Stateful Weight Sync Sender - #2142
Conversation
Signed-off-by: hao-aaron <ahao@anyscale.com>
Signed-off-by: hao-aaron <ahao@anyscale.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the weight synchronization architecture to use vLLM's native trainer-send abstraction, introducing specialized trainer-side engines and weight sources for the nccl, ipc, delta, and sharded_rdt backends, while removing legacy worker-extension mixins. The review feedback identifies a critical runtime bug in _packed_buffer_size_bytes where itemsize is incorrectly accessed on a torch.dtype object, which should be replaced with element_size(). Additionally, it is recommended to add explicit validation to ensure the number of server URLs is perfectly divisible by the data parallel size in both nccl_init_payloads and rdt_init_payloads to prevent truncation errors during rank offset and replica calculations.
| def rdt_init_payloads( | ||
| init_info: Dict[str, Any], | ||
| server_urls: Sequence[str], | ||
| data_parallel_size: int, | ||
| ) -> List[Dict[str, Any]]: | ||
| """Per-server sharded-RDT init payloads, stamped with the deployment ordinal. | ||
|
|
||
| Each deployment has its own self-contained parallel config, so its internal | ||
| worker index restarts at 0 and would collide under the M:N block assignment. | ||
| Stamping each server with its ordinal (``server_index // data_parallel_size``) | ||
| as ``replica_rank``, plus the deployment count as ``num_replicas``, lets the | ||
| engine offset its consumers into a globally distinct range. | ||
|
|
||
| The ordinal divides by ``data_parallel_size`` because the DP servers of one | ||
| deployment share a parallel config, so they must share one ``replica_rank``. | ||
| """ | ||
| dp = max(1, int(data_parallel_size)) | ||
| num_replicas = max(1, len(server_urls) // dp) |
There was a problem hiding this comment.
If len(server_urls) is not perfectly divisible by data_parallel_size (e.g., due to a misconfiguration), the division len(server_urls) // dp will truncate.
In rdt_init_payloads, this can result in a server having a replica_rank (i.e., i // dp) that is greater than or equal to num_replicas, violating the rank boundary and causing out-of-bounds or mapping errors in RDT.
We should explicitly validate that len(server_urls) % dp == 0 before performing these calculations.
def rdt_init_payloads(
init_info: Dict[str, Any],
server_urls: Sequence[str],
data_parallel_size: int,
) -> List[Dict[str, Any]]:
"""Per-server sharded-RDT init payloads, stamped with the deployment ordinal.
Each deployment has its own self-contained parallel config, so its internal
worker index restarts at 0 and would collide under the M:N block assignment.
Stamping each server with its ordinal (``server_index // data_parallel_size``)
as ``replica_rank``, plus the deployment count as ``num_replicas``, lets the
engine offset its consumers into a globally distinct range.
The ordinal divides by ``data_parallel_size`` because the DP servers of one
deployment share a parallel config, so they must share one ``replica_rank``.
"""
dp = max(1, int(data_parallel_size))
num_servers = len(server_urls)
if num_servers % dp != 0:
raise ValueError(
f"Number of servers ({num_servers}) must be divisible by data_parallel_size ({dp})."
)
num_replicas = max(1, num_servers // dp)
return [{**init_info, "replica_rank": i // dp, "num_replicas": num_replicas} for i in range(num_servers)]There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 1c62fb9. Configure here.
There was a problem hiding this comment.
I had an agent review the deletions in this file. It looks like you've deleted a bunch of tests for code that is still present. Can we revert that? We should always keep tests for code that lives in skyrl even if there's something similar on the vLLM side for RDT.
| Deleted class | Still-live code it covered |
|---|---|
TestPpLocalOwnership (~120 lines) |
rdt_send.py:961 held_names, _pp_local, _demoted, walk reorder |
TestExpertOwnership (~75 lines) |
EP stamping, "expert count not divisible by ep_size raises" (rdt_send.py:1053+) |
TestHeldNamesComposition |
ownership composition across pp × ep |
TestStampedYieldValidation |
held-name/tensor agreement checks |
TestQkvIndexDeviceCtx |
rdt_send._qkv_index_device_ctx (rdt_send.py:53) |
TestShardedRdtVllmRegistration |
rdt_vllm_register.ensure_registered() |
TestLoraLoadRequest |
weight_sync/base.py LoraLoadRequest |
There was a problem hiding this comment.
Can you simple move these RDT specific tests to a separate file and not delete them
| def register_delta_trainer_engine() -> None: | ||
| """Register ``delta`` in ``WeightTransferTrainerFactory`` (idempotent).""" | ||
| from vllm.distributed.weight_transfer.factory import WeightTransferTrainerFactory | ||
|
|
||
| if DELTA_BACKEND in WeightTransferTrainerFactory._registry: | ||
| return | ||
| WeightTransferTrainerFactory.register_engine( | ||
| DELTA_BACKEND, | ||
| "skyrl.backends.skyrl_train.weight_sync.delta_trainer", | ||
| "DeltaTrainerWeightTransferEngine", | ||
| ) |
| enable_bucketing=enable_bucketing, | ||
| batch_size_threshold_gb=( | ||
| inference_engine_cfg.weight_transfer_threshold_cuda_ipc_GB if enable_bucketing else 0.0 |
There was a problem hiding this comment.
Deleting this means that SkyRL doesn't set the bucket size for packed transfer anymore. weight_transfer_threshold_cuda_ipc_GB becomes unused.
Can we pass this to vLLM in init info?
There was a problem hiding this comment.
Also, the current logic has a common weight extractor for all weight sync backends. Which means that weight_transfer_threshold_cuda_ipc_GB also sets the bucket size for NCCL.
So we should use the same field weight_transfer_threshold_cuda_ipc_GB to set the packed transfer size even in the case of NCCL: https://github.com/vllm-project/vllm/blob/8a728663c1c3eeace834a95f5654fa653cc1998c/vllm/distributed/weight_transfer/nccl_engine.py#L80
I'll work on renaming the config in SkyRL meanwhile to avoid confusion
| """Load weights the engine can reach itself, rather than transferring them. | ||
|
|
||
| Args: | ||
| init_info: WeightSyncInitInfo from the sender containing all info needed | ||
| to create the appropriate receiver. | ||
| The only caller is the LoRA path, which passes a | ||
| :class:`LoraLoadRequest` naming an adapter directory on disk. Tensor | ||
| transfer goes through the trainer-side engines (see | ||
| ``weight_sync/trainer_engines.py``). | ||
| """ |
There was a problem hiding this comment.
Remove these docstring edits for update_named_weights?
They are wrong
There was a problem hiding this comment.
One pattern I see is some weird docstring comments from the agent that references now deleted code.
I have added many guidelines in the .claude/ folder in SkyRL to specifically avoid such docstrings.
Can you point your agent to the guidelines in .claude/docs and specifically the guide here:
asking it to make edits?
SkyRL/.claude/docs/contributing.md
Lines 51 to 157 in c516f3a
SumanthRH
left a comment
There was a problem hiding this comment.
Did a first pass! Take a look!
|
Overall given the large set of changes, we should also run some e2e tests. It would be good to:
|
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Weight sync: migrate onto vLLM 0.28's trainer-send abstraction
Deletes SkyRL's
WeightTransferStrategy/WeightTransferSender/WeightExtractorlayers and drives all four weight-sync backends through vLLM's own
TrainerWeightTransferEngine, over vLLM's native RLHF routes.Follow-up to #2114.
Why
vLLM 0.26 shipped only the receive half of the weight-transfer abstraction, so SkyRL
owned the send half: a strategy/sender hierarchy, its own extractor + chunk types, and a
/collective_rpcworker extension that wrapped every receive inset_current_vllm_configbecause vLLM did not.0.28 ships the send half (
TrainerWeightTransferEngine,WeightSource,VLLMWeightSyncClient,WeightTransferTrainerFactory) and wrapsset_current_vllm_configitself in
GPUWorker.start_weight_update/update_weights/finish_weight_update. Allof SkyRL's parallel machinery becomes redundant.
Shape of the change
Before — worker → SkyRL strategy → SkyRL sender →
/collective_rpc→ SkyRL workerextension → vLLM engine.
After — worker builds a
WeightSource→ vLLM trainer engine → native RLHF routes →vLLM engine (subclassed only where SkyRL needs a hook).
init_weight_sync_stateis now four statements;broadcast_to_inference_enginesis oneengine.send_weights()plus a memory bracket.New modules (
weight_sync/)sources.pyFsdpWeightSource/MegatronWeightSource— vLLM'smetadata()+__iter__, nothing elsetrainer_engines.pybuild_trainer_engine: resolve backend → build source →trainer_initcontrol_plane.pySkyrlWeightSyncClient(blocking HTTP) + the per-server init rewritesskyrl_engines.pyskyrl_nccl/skyrl_ipc+ the drafter-reload proxydelta_trainer.pyDeltaTrainerWeightTransferEnginepatches/vllm/patch_model_runner_registry.pyGPUModelRunnerso an engine can reach the drafterDeleted
transfer_strategy.py,broadcast_strategy.py,cuda_ipc_strategy.py,delta_strategy.py,sharded_rdt_strategy.py,nccl_trainer_send.py,weight_extractor.py,weight_extractor_utils.py,layerwise_reload.py,rdt_control_plane.py,rdt_control_protocol.py.Also gone:
WeightChunk,WeightUpdateRequest,FSDPWeightExtractor,MegatronWeightExtractor,get_transfer_strategy_cls,for_servers/to_api_payload/get_vllm_transfer_engine,sender_initializes_receivers,RemoteInferenceClient.init_weight_update_communicator/update_weights_nccl/update_weights_ipc/start_weight_update/finish_weight_update.sharded_rdt_base.pyshrank 410 → 207: the vendored ABCs now come from the wheel; whatremains is the two channels a pull backend needs and vLLM has no concept of
(
held_names()ownership,groups()/iter_groups()).Backends
ncclNCCLTrainerWeightTransferEngineskyrl_ncclipcIPCTrainerWeightTransferEngineskyrl_ipcdeltaDeltaTrainerWeightTransferEnginedeltasharded_rdtShardedRDTTrainerWeightTransferEnginesharded_rdtThe receive side takes new names for NCCL/IPC because SkyRL subclasses vLLM's engines and
register_enginerefuses an already-registered name.get_transfer_strategystays and iscalled from both sides — the driver (to configure the servers) and
build_trainer_engine(to pick the trainer engine) — so the two cannot disagree.The worker extension
NewInferenceWorkerWrapgoes from 8 weight-sync methods to 2. Both remaining are limits ofdispatch, not of the engine abstraction:
fetch_weights—/collective_rpcreaches worker methods by name only, and no nativeroute can invoke an engine method.
skyrl_sleep_for_weight_sync/skyrl_wake_for_weight_sync—EngineCore.sleephardcodes
clear_prefix_cache = level >= 1, andCuMemBackend.suspendcannot express"discard weights, offload kv_cache".
Gone with the rest:
skyrl_start/finish_weight_update,update_weights_nccl,update_weights_ipc,init_weight_transfer_engine_rdt,update_weights_rdt, andLayerwiseReloadWorkerMixin(0.28'sget_numel_loadedis character-identical to SkyRL'spatched copy, so the
CopyCounterpatch is no longer needed).Behavior changes worth reviewing
No bucketing. The Megatron export is one whole-model
export_hf_weightscall.Bucketing never bounded memory (it accumulates a bucket before handing it on); its
real purposes — IPC handle count and fused-loader grouping — are subsumed by vLLM's
packed_ipc_producer. One call also satisfies_accumulate_grouped_export's"every task of a
group_keyin one call" by construction.packed=Trueforced on IPC, overriding vLLM's default. The unpacked path holds acontiguous copy of every parameter until past
finish_weight_update— the whole modelresident on the trainer.
Packed buffer sized from the source. vLLM's 1 GiB default is smaller than a
large-vocab embedding (Qwen3-235B: 1.24 GiB), which raises on the IPC path. Pulls the
Megatron
metadata()dry export forward to init.Delta gained a drafter reload it never had. The old
DeltaWeightTransferEngine.receive_weightswas a bareself.model.load_weights(...), soMTP + delta silently drafted with stale weights. Now re-streams
iter_tensorsa secondtime. This is the one place the PR is not behavior-preserving — happy to split it out.
Delta owns its layerwise lifecycle. Previously no-ops, with the
/collective_rpcwrap doing it; now
start/finish_weight_updateruninitialize/finalize_layerwise_reload._weight_sync_threadselects this rank's CUDA device inside the worker thread.send_weightsruns off the event loop, the current device is thread-local and defaultsto 0, and under
RAY_EXPERIMENTAL_NOSET_CUDA_VISIBLE_DEVICESthis rank's device is not0 — so the NCCL communicator, IPC handles and gathers would land on the wrong GPU.
Capability probes replace declared flags.
getattr(engine, "skyrl_…", default),because two of the four trainer engines are vLLM's own classes and cannot carry SkyRL
attributes.
sender_initializes_receiversis gone — vacuous now that every enginerendezvouses in its own
trainer_init.Spec-decode drafter
Unchanged in effect, moved in mechanism.
spec_decode_utils.pyis untouched.vLLM's native
/start_draft_weight_updateis a separate session with its own data —right for EAGLE (separate checkpoint), wrong for MTP, where the head's weights are already
in the policy's stream and
share_embeddingsunconditionally aliases the target'sembed_tokens/lm_headinto the draft (so a draft session would re-send the singlelargest tensor). Instead the drafter is reloaded from the same stream in the same session,
via a
_LoadWeightsProxyinstalled only when this process actually has a drafter.Per backend:
ncclproxy → proxy (same);ipcdirect call → proxy (same effect);deltanone → second pass (new);sharded_rdtnone → none (still a gap).Known limitation, pre-existing: none of these run
initialize_layerwise_reloadon thedraft model, so
process_weights_after_loadingnever runs there. Fine for bf16 MTPheads; wrong for a quantized drafter.
Tests
Deleted with their subjects:
test_transfer_strategies.py(969 lines),test_sharded_rdt_strategy.py,test_weight_chunk.py,test_weight_extractor_utils.py,test_rdt_control_plane.py.Added:
test_control_plane.py(the init rewrites — a bad NCCLrank_offsethangs in therendezvous rather than erroring),
test_sources.py(channel agreement + laziness),test_trainer_engines.py(init-info choices, backend resolution, capability probes).Rewritten:
test_prefix_cache_reset.py(probes, with a fake engine rather than a Mock sothe defaults are actually exercised), GPU
test_weight_sync.py(all four backends now gothrough
build_trainer_engine, i.e. the production path),test_megatron_extractor_consistency.py→test_megatron_weight_source.py.-m "not vllm"-m "vllm"test_weight_sync.pytest_delta_weight_sync_e2e.py[fsdp]test_offload_kv_weight_sync.pyNot run — needs a real Megatron model: GPU
megatron/test_megatron_weight_source.pyandthe megatron half of the delta e2e. Apply
run_train_gpu_ciandrun_train_megatron_gpu_ci.The delta e2e needs
~/data/gsm8k/validation.parquet(
python examples/train/gsm8k/gsm8k_dataset.py --output_dir ~/data/gsm8k). Without it thetest fails in
get_test_promptsafter the sync has already succeeded, which reads like aweight-sync failure and is not one.
Known gaps
executed. A
speculative_configparametrization on the delta or offload e2e would close it.MegatronWeightSourceis unit-tested against a fakebridge only.
NCCLTrainerWeightTransferEngine._checked_iternow validatesmetadata()against iteration at runtime, so expect it to surface latent ordering bugson the first real Megatron MoE run; that is the feature working.
is_sender=Falsepaths (IPC handleall-gather, delta's result gather) are unexercised.
pp > 1— megatron-bridge'sbroadcast_from_pp_ranktakes acache_key, and the old extractor rebuilt tasks each sync "so mapping objects start withclean PP-collective caches". A single long whole-model export may accumulate per-mapping
cached tensors. Needs a peak-memory measurement.
Docs
.claude/docs/weight_sync.mdrewritten;architecture.md,backends/fsdp.mdanddocs/content/docs/getting-started/inference_architecture.mdxupdated.Note
High Risk
This rewires the core training→inference weight path (NCCL rank mapping, packed IPC, delta reload, RDT fan-out); mistakes tend to hang or silently stale weights rather than fail fast.
Overview
Replaces SkyRL’s custom weight transfer strategy/sender/extractor stack with vLLM 0.28’s trainer-send model: each training worker builds a
WeightSourceand aTrainerWeightTransferEnginewhosesend_weights()drives the native/init_weight_transfer_engine→/start_weight_update→/update_weights→/finish_weight_updateround trip via a blockingSkyrlWeightSyncClient.Removed the old broadcast/IPC/delta/RDT strategy modules, chunk/extractor types, vendored
nccl_trainer_send, layerwise/collective_rpcweight paths, and most ofRemoteInferenceClient’s weight-sync APIs. Addedsources.py,trainer_engines.build_trainer_engine,control_plane.py(NCCL/RDT per-server init rewrites), receive-sideskyrl_nccl/skyrl_ipcengines with drafter reload,delta_trainer, and aGPUModelRunnerregistry patch.NewInferenceWorkerWrapnow only handlesfetch_weights(delta, pre-pause) and direct CuMemAllocator sleep/wake for KV offload during sync; receive lifecycle and chunk loading live in vLLM engines. Delta owns layerwise reload on the receive engine and re-streams the checkpoint for spec-decode drafter reload (behavior change vs. prior bareload_weights). Docs and public inference architecture pages are updated to match the four backends (nccl,ipc,delta,sharded_rdt) andget_vllm_receive_backendon the driver.Reviewed by Cursor Bugbot for commit 055bc62. Bugbot is set up for automated code reviews on this repo. Configure here.