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. |
qgallouedec
left a comment
There was a problem hiding this comment.
Benchmarked it (single H100, Qwen3-0.6B, bs=4, seq 512)
| config | median step | peak |
|---|---|---|
| main, fused, inline ref | 0.2243 s (n=4) | 7.05 GB |
| this PR, inline ref | 0.2457 s (n=6) | 7.05 GB |
this PR, precompute_ref_log_probs=True |
0.1971 s (n=2) | 4.83 GB |
+9.5% per step at equal config, -12.1% with precomputed ref logps, which main rejects outright. Memory unchanged in the first case, 31% lower in the second.
So nothing unexpected!
A few things:
-
For the record, I found that
compute_lossredirects onis_zero3 or self.is_fsdp_enabled, so plain DDP falls through to_compute_loss(unwrapped_model, ...)andprepare_for_backward()never fires. That is inherited from main so leaving it is right. I'll open an issue to fix it across trainers. -
Docs need updating,
dpo_trainer.mdhas a "Compatibility and constraints" section listing, underuse_liger_kernel=True: only a singleloss_type, nocompute_metrics, noprecompute_ref_log_probs. This PR lifts the first and the third and adds a new one (use_weighting=True), so three lines there are now wrong. -
Same section is where the f-divergence lift belongs, it was only ever enforced in code.
|
Related to #7245: DPO silently skips DDP gradient synchronization with
On main the one-line fix does not work. Adding On this branch the same change gives 0/310. So the DPO half of #7245 belongs here, as one line in if is_zero3 or self.is_fsdp_enabled or model is not unwrapped_model:KTO is going separately, it is already on the chunked path. |
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.
Reviewed by Cursor Bugbot for commit c2196d5. Configure here.
| is_zero3 = deepspeed_plugin is not None and deepspeed_plugin.zero_stage == 3 | ||
| unwrapped_model = self.accelerator.unwrap_model(model) | ||
| if is_zero3 or self.is_fsdp_enabled: | ||
| if is_zero3 or self.is_fsdp_enabled or model is not unwrapped_model: |
There was a problem hiding this comment.
DDP redirect not copied to KTO
Medium Severity
compute_loss now redirects through the wrapper whenever model is not unwrapped_model, so DDP arms its reducer on the chunked path. The same duplicated block in KTOTrainer.compute_loss still redirects only for ZeRO-3 and FSDP. Copied trainer logic in this repo has to stay aligned, and KTO still skips DDP gradient synchronization with use_liger_kernel=True.
Triggered by project rule: ../.ai/AGENTS.md
Reviewed by Cursor Bugbot for commit c2196d5. Configure here.
`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. Measured on 2 GPUs with different data per rank, comparing every parameter's gradient across ranks: 310/310 parameters differed before, 0/310 after. DPO has the same bug but cannot take the same fix: its fused loss runs `torch.func.grad_and_value` internally, which raises `NotImplementedError: Cannot access storage of TensorWrapper` when run inside `DistributedDataParallel.forward()`. It is fixable once #7243 replaces that path. Ref #7245
|
Thanks!, both points addressed. Three things left:
|


What does this PR do?
Moves DPO onto the shared chunked log-probability path while keeping the trainer loss math in one place. This removes the duplicated fused DPO loss and lifts its restrictions on mixed losses, f-divergences, and precomputed reference log-probs.
Part of #7063.
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?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag members/contributors who may be interested in your PR.
Note
High Risk
Changes the core DPO forward/backward path (memory, numerics, distributed hooks) and deletes the old fused implementation; regressions would affect all
use_liger_kernelDPO training.Overview
Replaces the separate fused DPO loss with a shared chunked log-probability path so
use_liger_kernel=Trueno longer depends onFusedLinearDPOLoss/liger-kernelfor DPO. Policy and reference log-probs are computed via backbone forward +_ChunkedLogProbFunction(vocab-chunkedlm_head), then the existing multi-loss / f-divergence logic in_compute_lossruns unchanged.Removes
trl/losses/dpo_loss.py,fused_linear_preference.py, and the largetests/test_dpo_loss.pyparity suite. Drops init-time blocks on multipleloss_type, non-defaultf_divergence_type, andprecompute_ref_log_probs; onlyuse_weighting=True(andcompute_metrics, MoE aux, PEFT edge cases) stay incompatible. Adds FSDP2 support by gathering shardedlm_headDTensors before projection.Docs and distributed tests are updated (FSDP2 no longer xfail for Liger DPO). Trainer tests now compare chunked vs full-logit loss/gradients across loss types and f-divergences.
Reviewed by Cursor Bugbot for commit c2196d5. Bugbot is set up for automated code reviews on this repo. Configure here.