Fix DPO/KTO use_liger_kernel under DeepSpeed ZeRO-3 - #6372
Conversation
|
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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 99af755. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99af755e7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
use_liger_kernel under DeepSpeed ZeRO-3use_liger_kernel under DeepSpeed ZeRO-3
|
thanks! useful to have as a util for other trainers, since I was doing a targeted fix |
`training_step` wraps `super().training_step(...)` in `_get_liger_zero3_lm_head_gather_ctx` so the fused Liger JSD loss's direct `lm_head.weight` read is gathered under DeepSpeed ZeRO-3. `prediction_step` called `compute_loss` directly with no such wrapping, so `trainer.evaluate()` ran the same fused matmul against an ungathered, sharded weight whenever `use_liger_kernel=True` is combined with DeepSpeed ZeRO-3 and an `eval_dataset`/`eval_strategy`. Wrap `prediction_step`'s `compute_loss` call in the same gather context, scoped to the forward only since evaluation never calls backward. Mirrors the analogous SDPO/SDFT (huggingface#6384), GRPO (huggingface#5891), and DPO/KTO (huggingface#6372) ZeRO-3 gather fixes. Separately, `compute_loss`'s liger branch names its student forward-pass output `student_outputs`, uses it to compute `student_hidden`, then deletes it -- but the function's shared final return, `return (loss, outputs_student) if return_outputs else loss`, reads `outputs_student`, a different name never assigned in that branch. `return_outputs=True` isn't reachable from `train()`/`evaluate()`/`predict()` today (all three call `compute_loss` with the default `return_outputs=False`), so this doesn't fire in current end-to-end flows, but it's live, broken code in the method this PR is already touching. Rename the local variable to match the other two branches and stop deleting it before the shared return. Adds regression tests for both: one asserting prediction_step enters the gather-context helper the same way training_step does, one calling compute_loss(..., return_outputs=True) directly against a liger-enabled trainer and asserting it no longer raises UnboundLocalError.
`training_step` wraps `super().training_step(...)` in `_get_liger_zero3_lm_head_gather_ctx` so the fused Liger JSD loss's direct `lm_head.weight` read is gathered under ZeRO-3. `prediction_step` calls `compute_loss` directly with no such wrapping, so `trainer.evaluate()` runs the same fused matmul against an ungathered, sharded weight whenever `use_liger_kernel=True` is combined with DeepSpeed ZeRO-3 and an `eval_dataset`/`eval_strategy`. Wrap `prediction_step`'s `compute_loss` call in the same gather context, scoped to the forward only since evaluation never calls backward (unlike `training_step`, which must span both because the fused JSD loss computes the lm_head grad during backward). Mirrors the analogous GRPO (huggingface#5891) and DPO/KTO (huggingface#6372) ZeRO-3 gather fixes. Adds a regression test to both experimental test suites asserting `prediction_step` enters the gather-context helper the same number of times `training_step` does.

use_liger_kernel=True+ DeepSpeed ZeRO-3 inDPOTrainercrashes. This mirrors the GRPO bug fixed in #5891.Closes #4544.
Why
The Liger fused DPO loss reads
lm_head.weightdirectly instead of going throughmodel.forward(). Under ZeRO-3 this breaks in two places:numel 0and only re-materializes it inside its owning module's forward hook. Since Liger accesseslm_head.weightby attribute (never calling thelm_headmodule), the gather hook never fires and the fused matmul gets an empty shard →size mismatch, got input (s2x2048), vec (0).forward(), ZeRO-3's pre-forward hooks are never armed, so the backward gradient reduction isn't set up and grad accumulation fails →size of tensor a (0) must match the size of tensor b (H).Fix
Apply the same two-part treatment GRPO already uses (#5891):
maybe_gather_lm_head_ctx(new shared helper intrainer/utils.py) all-gathers thelm_headweight/bias for the duration of the fused call. No-op when not on ZeRO-3, or when the weight is already gathered (tied embeddings). Deduplicates by identity so a shared policy/reference head (PEFT with no separateref_model) isn't passed toGatheredParameterstwice._compute_loss_ligerthrough_ForwardRedirectionso the fused loss executes inside the engine'sforward(), arming the ZeRO-3 pre-forward hooks. The redirection is gated to ZeRO-3 / FSDP: on plain DDP it would wrap Liger'storch.funccall insideDDP.forward()and error, so there we call_compute_loss_ligerdirectly on the unwrapped model.Also routes GRPO's inline gather through the shared
maybe_gather_lm_head_ctx(was a duplicated block), per the repo's consistency guideline.Verified (2×H100)
test_dpo_liger(new, mirrorstest_grpo_liger):test_grpo_liger[zero3]still passes after routing GRPO through the shared helper.Reproducer
Before this PR, the following crashes under ZeRO-3; after it, it trains.
accelerate launch --config_file tests/distributed/data/accelerate_configs/zero3.yaml trl/scripts/dpo.py \ --model_name_or_path trl-internal-testing/tiny-Qwen2ForCausalLM-2.5 \ --dataset_name trl-internal-testing/zen --dataset_config standard_preference \ --use_liger_kernel --output_dir /tmp/dpo-liger-zero3Known limitations / follow-ups
_compute_loss_ligerruns the backbone directly on the unwrapped model, which is incompatible with FSDP2's DTensor-sharded params (mixed torch.Tensor and DTensor). This never worked and is left as a follow-up.model(...).logitsforward in_compute_kl_logpsthat doesn't cooperate with the redirection. Not fixed here; separate follow-up.Note
Medium Risk
Changes distributed training loss paths (ZeRO-3/FSDP) for DPO/KTO/GRPO/SFT Liger and chunked CE; incorrect gather/redirection could cause silent wrong grads or crashes, but scope is narrow to fused-lm_head paths with new integration tests.
Overview
Fixes
use_liger_kerneltraining under DeepSpeed ZeRO-3 (and aligns FSDP) for DPO and KTO by reusing the same pattern as GRPO: fused losses readlm_head.weightdirectly, so ZeRO-3 never gathers shards and backward grad reduction breaks.Shared
maybe_gather_lm_head_ctxintrainer/utils.pywraps ZeRO-3GatheredParametersforlm_headweight/bias (no-op on DDP or when already gathered; dedupes tied/shared params). SFT drops its local copy and GRPO switches to this helper.DPO/KTO wrap the Liger loss in that gather context, set
_ForwardRedirection, and on ZeRO-3/FSDP routecompute_lossthrough redirection so the fused path runs inside the engine forward. Plain DDP still calls_compute_loss_ligeron the unwrapped model.KTO additionally avoids a full
model(...).logitspass in_compute_kl_logpswhen Liger is on (backbone + manual matmul with gather) so ZeRO-3 doesn’t double-touchlm_head.Adds distributed
test_dpo_ligerandtest_kto_liger; FSDP2 stays xfail (DTensor vs direct backbone/lm_headaccess).Reviewed by Cursor Bugbot for commit a5b576b. Bugbot is set up for automated code reviews on this repo. Configure here.