Redirect the liger loss through the distributed wrapper under DDP - #7246
qgallouedec wants to merge 1 commit into
Conversation
`compute_loss` entered `_forward_redirection` only for ZeRO-3 and FSDP, so under plain DDP the loss ran on the unwrapped model. `DistributedDataParallel.forward()` never ran, its reducer was never armed, and gradients were never all-reduced: every rank kept its own. Fixes #7245
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Superseded. Splitting this in two, because the same fix does not work for both trainers. KTO is #7247, verified: 310/310 parameters differing across ranks before, 0/310 after, and zero3/zero2/ddp all green. DPO cannot take this change on main. The fused loss runs Tracked in #7245. |
What does this PR do?
Fixes #7245.
compute_lossinDPOTrainerandKTOTrainerentered_forward_redirectiononly for ZeRO-3 and FSDP, and otherwise ran the loss on the unwrapped model. Under plain DDP that meansDistributedDataParallel.forward()never runs, so its reducer is never armed byprepare_for_backward(), the autograd hooks return early, and gradients are never all-reduced. Each rank keeps its own.Measured on 2 GPUs, one step, different data per rank, comparing every parameter's gradient across ranks:
use_liger_kernel=True, beforeuse_liger_kernel=True, afteruse_liger_kernel=FalseIntroduced in #6372, which unwrapped the model to fix ZeRO-3. Present in v1.10.0, v1.11.0 and v1.12.0.
The condition now matches the one
compute_ref_log_probsalready uses in both files, and thatGRPOTraineruses in #7077.is_zero3is kept rather than folded in: under ZeRO-3 the model is aDeepSpeedEngine, somodel is not unwrapped_modelshould already cover it, but deepspeed is xfailed against the transformers version I have, so I left the existing term alone rather than change a path I could not exercise. Worth simplifying separately.Note
Medium Risk
Changes distributed training gradient synchronization for Liger-enabled DPO/KTO; fixes a silent DDP bug but touches core
compute_losspaths used on every training step.Overview
Fixes incorrect multi-GPU training when DPO or KTO use
use_liger_kernel=True.compute_lossnow routes the Liger loss through_forward_redirectionwhenever the passedmodelis a distributed wrapper (model is not unwrapped_model), in addition to ZeRO-3 and FSDP—so DDP’sforward()runs and gradient all-reduce is armed.Regression tests were added for DPO and KTO that mock a wrapped model and assert
compute_losscalls_forward_redirection, matching the pattern already used for reference log-probs.Reviewed by Cursor Bugbot for commit e02eb31. Bugbot is set up for automated code reviews on this repo. Configure here.