Fix DPO IPO log-prob normalization - #9925
Open
taking-lying-flat wants to merge 2 commits into
Open
Conversation
taking-lying-flat
marked this pull request as ready for review
August 16, 2026 21:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
logps_mean/chosenandlogps_mean/rejectedwhen IPO is configured.Root cause
IPO uses length-normalized completion log-probabilities, while standard sigmoid DPO uses sequence sums. The previous implementation normalized the shared per-token tensor before reducing it into per-sequence scores:
This caused three related problems.
Padding-free and packing used the whole packed row as the denominator
Padding-free batches have shapes similar to:
Consequently,
loss_mask.sum(-1)produced one count for the entire packed row. Sequence boundaries fromcu_seqlenswere only applied afterward, so every chosen/rejected sequence was divided by the combined token count.For example, with two completions:
The correct IPO scores are
-3 / 2 = -1.5and-6 / 6 = -1.0. The previous padding-free path returned-3 / 8 = -0.375and-6 / 8 = -0.75. This changes the relative preference margin, not only its scale.Packing uses the same flattened representation, and sequence-parallel gathering restores the full packed row before this reduction, so both paths were affected.
IPO contaminated other losses in a mixed configuration
loss_typecan contain multiple weighted losses, for example:The forward pass creates one shared pair of
chosen_logpsandrejected_logps, while the loss loop evaluates each configured loss afterward. Because the old forward normalization ran whenever IPO appeared anywhere in the list, sigmoid DPO also received normalized log-probabilities instead of sequence sums.As a result, the mixed sigmoid component did not match standalone sigmoid DPO.
The ordinary padded IPO path could fail broadcasting
Outside padding-free mode, the tensors have shapes
[B, L]and[B]. Dividing them directly is invalid for the usual case whereB != L, producing a size-mismatch error. Moving normalization after sequence reduction also removes this invalid broadcast.Fix
concatenated_forward()now always reduces per-token log-probabilities into sequence sums and returns the valid completion-token counts alongside them. In padding-free mode, each count is computed inside itscu_seqlenssegment from the correspondingloss_maskslice.The per-loss loop then:
Reference precomputation continues to store sequence sums, so the same cached values work for standalone and mixed losses. Other loss types retain their existing sum-based behavior.
The existing
logps/chosenandlogps/rejectedmetrics remain sequence sums for compatibility. When IPO is present, the trainer additionally reports the mean values actually supplied to IPO aslogps_mean/chosenandlogps_mean/rejected.The regression tests use completions with clearly different valid lengths and verify:
cu_seqlenssegmentation;