Repository navigation
Conversation
The weight-transfer requests to a rollout engine had no client timeout. vLLM's NCCL trainer_init sends /init_weight_transfer_engine from a side thread and joins it when it leaves its executor, so if the trainer-side rendezvous fails while the engine waits, the trainer hangs instead of raising; a stuck /start_weight_update, /update_weights or /finish_weight_update hangs a push the same way. Add --update-weight-init-timeout (default 600 s) for joining the transfer channel and --update-weight-request-timeout (default 1800 s; with NCCL, /update_weights returns only after the broadcast) for the session requests, and pass them to every such request. A timeout turns the hang into an error on the trainer; the engine-side worker can still be waiting in the rendezvous. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
When a weight push to the rollout engines failed, the exception left the trainer on the rank that saw it while the other ranks were still inside the trainer's collectives, and generation was never resumed or checked. vLLM leaves a partly updated model in that case, so the only safe recovery is a full resend, and every rank has to agree to make it. Iterating the weight source is itself a collective, so the ranks can only agree once they have all finished the pass: - IPC: the sender's client records a failed start/update/finish and skips the rest of the session, so the trainer still runs its per-chunk gather and barriers on every rank. - NCCL: the client still raises, so vLLM never broadcasts to engines that are not receiving; ResumableWeightSource then lets the sender finish the pass it had started (or run a full pass), joining the gathers the other ranks are in. send_weights_with_resync then agrees on the result over the gloo group. After a failure it closes any session left open on the engines (finish without a version), rebuilds the NCCL channel, and sends the whole model once more; a second failure raises on every rank. Before resuming generation, rank 0 checks that every engine reports the pushed version, and all ranks raise otherwise. Connecting the engines is agreed on the same way. Failures inside the data plane (a sender blocked in an NCCL broadcast, a gather failing on one rank) are not recovered. The expert-routing push keeps its own start/finish and has no resend yet. tests/utils/test_weight_sync_resync.py runs four gloo ranks with fake trainers that follow vLLM's IPC and NCCL call order and inject failures at the sender's requests; the plan's agree-after-send sketch is kept as a negative control and times out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
There was a problem hiding this comment.
Code Review
This pull request introduces robust error handling and resynchronization mechanisms for weight updates across distributed rollout engines, supporting both NCCL and IPC transports. It adds ResumableWeightSource to manage partial iterations, send_weights_with_resync to coordinate retries across ranks, and configurable timeouts for weight transfer requests. Two valuable review comments were identified: one addressing a potential AttributeError in a finally block that could mask exceptions, and another addressing a potential RuntimeError when dist.all_reduce falls back to NCCL with a CPU tensor.
| finally: | ||
| self._source.draft = False | ||
| self._trainer.client.draft = False |
There was a problem hiding this comment.
If self._reconnect() fails during send_weights_with_resync, self._trainer is set to None. In the finally block, accessing self._trainer.client will raise an AttributeError, which masks the original RuntimeError raised by send_weights_with_resync. Adding a None check prevents this masking and ensures the original exception is propagated correctly.
| finally: | |
| self._source.draft = False | |
| self._trainer.client.draft = False | |
| finally: | |
| self._source.draft = False | |
| if self._trainer is not None: | |
| self._trainer.client.draft = False |
| flag = torch.tensor([1 if ok else 0], dtype=torch.int32) | ||
| dist.all_reduce(flag, op=dist.ReduceOp.MIN, group=get_gloo_group()) |
There was a problem hiding this comment.
If get_gloo_group() returns None (e.g., if Gloo is not initialized or in certain test environments), dist.all_reduce will fallback to the default process group (NCCL). Since NCCL does not support CPU tensors, calling dist.all_reduce on a CPU tensor will raise a RuntimeError: Tensors must be CUDA tensors. Placing the tensor on the current CUDA device when gloo_group is None and CUDA is available prevents this crash.
| flag = torch.tensor([1 if ok else 0], dtype=torch.int32) | |
| dist.all_reduce(flag, op=dist.ReduceOp.MIN, group=get_gloo_group()) | |
| gloo_group = get_gloo_group() | |
| device = torch.device("cpu") | |
| if gloo_group is None and torch.cuda.is_available(): | |
| device = torch.device("cuda", torch.cuda.current_device()) | |
| flag = torch.tensor([1 if ok else 0], dtype=torch.int32, device=device) | |
| dist.all_reduce(flag, op=dist.ReduceOp.MIN, group=gloo_group) |
Summary
Hardens the weight push from the Megatron trainer to the vLLM rollout engines, for both the IPC (colocated) and the NCCL (distributed) updater. This is the first of the smaller PRs announced in #478 after closing #479, and it is independent of FSDP2.
--update-weight-init-timeout(default 600 s) bounds/init_weight_transfer_engine;--update-weight-request-timeout(default 1800 s) bounds/start_weight_update,/update_weightsand/finish_weight_update.trainer_initsends the init request from a side thread and joins it when it leaves its executor. If the trainer-side rendezvous fails while the engine is still waiting, the trainer previously hung instead of raising.continue_generation, rank 0 checks that every engine reports the pushed version (/weight_info); all ranks raise otherwise.Why the failure handling differs per transport
A first sketch called
send_weights(), then all-reduced a failure flag. That deadlocks, because iterating the weight source is itself a collective: every trainer rank gathers each tensor together.startorupdate, the other ranks wait in the next gather.all_gather_objectand a barrier over all ranks, so any raise on the sender leaves the others there.The policy is therefore per transport, entirely in vime's client and source wrappers:
ResumableWeightSourcethen lets the sender finish the pass it had started, or run a full pass if it had not started one, so it joins the gathers the other ranks are in.Engine session rules relied on
These come from vLLM's worker.
update_weightsends the session, so a freshstartis accepted.startis refused while a session is open, and there is no abort. So before resending, the trainer finishes any open session without a version; the old version stays visible, and the full resend overwrites any partially loaded layers.finish.Not covered
update_weightsrejected mid-broadcast: vLLM's NCCL trainer runs the engines'update_weightson a side thread. A rejection is caught before the broadcast only if its best-effortfuture.done()check sees it in time; otherwise the sender enters the broadcast and blocks, which is the data-plane case below. NCCLstartandfinishfailures, and anupdaterejection caught before the broadcast, are recovered.start/finishitself and has no resend yet (TODO in_update_rollout_weights).Testing (CPU)
tests/utils/test_weight_sync_resync.py(new): four gloo ranks run fake trainers that follow vLLM's IPC and NCCL call order. A fake engine applies the session rules above. Failures are injected at the sender's requests.start,updateorfinishon either transport: every rank returns after one full resend, the engine reports the new version, and generation resumes. The fake NCCL trainer surfaces anupdaterejection before the broadcast, so the NCCLupdatecase covers only that path.tests/utils/test_update_weight_from_{distributed,tensor}.pyare updated, and new cases cover:tests/utils/test_vllm_engine.py: each request carries its timeout.python -m pytest tests/utilspasses, and the CPU CI test files in.buildkite/pipeline.ymlgive the same results as onmain.GPU validation (an NCCL and an IPC push with an injected mid-push failure, plus the init timeout) has not been run yet.
AI assistance
The code, tests and this description were written with Claude Code (AI-assisted). Each commit carries a
Co-Authored-Bytrailer.🤖 Generated with Claude Code