Repository navigation
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f8076b6. Configure here.
f8076b6 to
2f61522
Compare
…exception propagation - vllm_generation.py: Under DeepSpeed ZeRO-3 with PEFT/LoRA, gather/merge/sync/unmerge one BaseTunerLayer module at a time instead of materializing the entire model on every rank. Prevents OOM on large models. FSDP path unchanged. - vllm_serve.py: Propagate worker exceptions back through the pipe so the parent process surfaces failures instead of hanging. Added to generate(), chat(), and _run_prompt_logprobs().
2f61522 to
533325d
Compare
|
Checked both halves against current main. The The # trl/generation/vllm_generation.py, _iter_named_params(), PEFT + non-FSDP branch
with self._dist.gather_params(list(model.parameters())):
model.merge_adapter()
...
for name, param in model.named_parameters():
...So DeepSpeed ZeRO-3 + PEFT still materializes every parameter on every rank before syncing, same OOM risk this PR describes. But the surrounding architecture changed since this PR was written: weight sync used to be a Worth keeping open as a real, live bug report even though this specific diff isn't mergeable — the module-at-a-time approach here is the right shape for the fix, just needs to be re-expressed as a generator. |
|
@rak96 is it possible to solve the conflicts for review ? |

What does this PR do?
This PR makes vLLM weight synchronization more memory-efficient for PEFT/LoRA models trained with DeepSpeed ZeRO-3.
Previously, the PEFT weight-sync path gathered all model parameters before merging adapters and syncing weights to vLLM. Under DeepSpeed ZeRO-3, this can materialize the full model on every rank and cause OOMs for large models.
This PR changes the non-FSDP PEFT path to gather, merge, sync, and unmerge one
BaseTunerLayermodule at a time. This avoids a full-model gather while preserving the existing parameter-name normalization and vLLM loading behavior.It also adds minimal worker exception propagation in vllm_serve.py, so worker-side failures are sent back through the pipe and surfaced by the parent process instead of hanging.
No new dependencies are added.
Changes
Testing
python -m ruff check trl/generation/vllm_generation.py trl/scripts/vllm_serve.pypython -m ruff format --check trl/generation/vllm_generation.py trl/scripts/vllm_serve.pypython -m pytest tests/test_vllm_client_server.py::TestChunkList tests/test_vllm_client_server.py::TestExtractLogprobs -qpython -m pytest tests/test_vllm_client_server.py -q -m "not slow"Fixes # (issue)
Before submitting
AI writing disclosure
We welcome the use of AI tools to help with contributions. For transparency and to help us improve our review process, please indicate the level of AI involvement in this PR.
Who can review?
@qgallouedec @AmineDiro @kashif @remi-or
Note
Medium Risk
Touches distributed PEFT/LoRA weight syncing under DeepSpeed ZeRO-3 and changes inter-process error handling; mistakes could cause incorrect weight updates or new runtime failures/hangs during generation.
Overview
Improves PEFT/LoRA weight synchronization to vLLM under DeepSpeed ZeRO-3 by switching from a full-model gather/merge to a module-at-a-time gather+merge+sync+unmerge flow (using
BaseTunerLayer/ModulesToSaveWrapper) to avoid per-rank full-model materialization and OOMs.Keeps the FSDP PEFT path as a full merge+sync via existing FSDP-aware traversal, but ensures unmerge happens within the gathered context.
Updates
vllm_serve.pyso worker-side exceptions duringgenerate/chat(and prompt-logprob generation) are sent through the pipe and raised in the parent, preventing silent hangs and surfacing failures earlier.Reviewed by Cursor Bugbot for commit 533325d. Bugbot is set up for automated code reviews on this repo. Configure here.