Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the vLLM weight transfer process by removing the _NcclBridge subprocess and implementing in-process transfer using NCCLWeightTransferEngine. This change reduces architectural complexity by running the transfer logic directly in the Megatron trainer. A review comment suggests using a generator expression when preparing tensors for transfer to avoid a significant memory spike on rank 0, ensuring that only one extra contiguous copy of a tensor exists in memory at a time.
| named_gpu = [] | ||
| for name, param in converted_named_tensors: | ||
| data = param.data if hasattr(param, "data") else param | ||
| named_gpu.append((name, data.contiguous())) | ||
| NCCLWeightTransferEngine.trainer_send_weights( | ||
| iterator=iter(named_gpu), |
There was a problem hiding this comment.
Building a list of contiguous tensors for the entire bucket creates a significant memory spike on rank 0, as it effectively doubles the memory required for the weights being transferred in that bucket. Since trainer_send_weights accepts an iterator, you can use a generator expression to yield contiguous tensors one by one. This ensures that only one extra contiguous copy exists in memory at any given time, which aligns better with the goal of reducing GPU 0 memory skew.
named_gpu_iter = ((name, (param.data if hasattr(param, "data") else param).contiguous())
for name, param in converted_named_tensors)
NCCLWeightTransferEngine.trainer_send_weights(
iterator=named_gpu_iter,8df6c8e to
940fabc
Compare


Purpose
Follow-up to vLLM native weight sync integration (PR #3). Issue #4 reports that the previous
_NcclBridgesubprocess on trainer rank 0 / GPU 0 pins ~10 GiB extra memory vs other trainer ranks and adds ~30 s to the first weight sync (subprocess spawn + second CUDA context).This PR is step 1/N of the performance work for #4: call vLLM’s
NCCLWeightTransferEngine.trainer_init()in the Megatron train actor process (SkyRL-style), and send weights viatrainer_send_weights()on the existing packed bucket path—notorch.multiprocessingchild process.Later steps (not in this PR) may further shrink the remaining rank-0 vs rank-1 gap (packed buffer sizing, optional teardown/recreate of PyNccl after each sync, docs).
What’s included
UpdateWeightFromDistributed(vLLM path)connect_rollout_engines_from_distributed:NCCLWeightTransferEngine.trainer_init(...)in-process on trainer rank 0.update_weights_from_distributed:NCCLWeightTransferEngine.trainer_send_weights(..., packed=...)(unchanged packed bucket semantics; MoE expert buckets stillpacked=False).disconnect_rollout_engines_from_distributed: notify rollout engines only; no bridgeshutdown()._NcclBridge,_nccl_bridge_worker, and related multiprocessing spawn/teardown.init_process_group+dist.broadcast;_is_vllm_backend()routing; rollout HTTP metadata + engineinit_weight_transfer_engineflow.Test plan
Same environment as #4 and PR #3:
tensor-model-parallel-size 2on actor)--rollout-backend vllm --vllm-weight-sync-mode nativeNcclBridgesubprocess implementation on the same script and hardware.Docker / install (same as PR #3)
E2E: Megatron + vLLM native sync (this change)
From repo root:
Key flags (also in the script):
--rollout-backend vllm--vllm-weight-sync-mode nativeglobal-batch-size 256, etc.Optional: minimal coexistence check (no full training)
Validates
trainer_init+trainer_send_weightsin-process alongside an existingtorch.distributedNCCL group (coexistmode).Test results
Hardware: 8× A100-80GB, Qwen3-4B,
run_scripts/qwen_4b.sh, native vLLM weight sync.Weight sync latency (
perf/update_weights_time)NcclBridgesubprocess)trainer_init)Trainer GPU memory (rank 0,
used_GBafterupdate_weights)clear_memory(~50.6 GiB on rank 0 after step ≥1) is in line with the old run;clear_memorystill returns rank 0 to ~30.7 GiB before the next sync.Correctness / stability
vLLM in-process weight transferon connect; noNcclBridge ready.Rank imbalance (remaining)
Removing the bridge subprocess eliminates the ~10 GiB child-process component from #4; the remaining gap is expected from rank-0 HF conversion buckets + in-process PyNccl / packed buffers—target for a follow-up [2/N] PR.
Closes #4 partially (subprocess + first-sync latency); rank-0 gap reduction tracked as follow-ups above.