Skip to content

fix(dpo): keep apo_down on raw log-ratios for both terms#6469

Open
Solaris-star wants to merge 1 commit into
huggingface:mainfrom
Solaris-star:fix/6441-apo-down-f-divergence-raw-logratios
Open

fix(dpo): keep apo_down on raw log-ratios for both terms#6469
Solaris-star wants to merge 1 commit into
huggingface:mainfrom
Solaris-star:fix/6441-apo-down-f-divergence-raw-logratios

Conversation

@Solaris-star

@Solaris-star Solaris-star commented Jul 20, 2026

Copy link
Copy Markdown

What does this PR do?

DPOTrainer's apo_down loss mixed a raw chosen term with an f-divergence-transformed rejected term. Under non-default f_divergence_type values the two terms silently disagreed; under the default reverse_kl they only matched by accident.

This change makes both terms use the raw reverse-KL log-ratios, matching the APO paper (Eq. 8) and the sibling apo_zero loss. Behavior is unchanged for the default reverse_kl.

Fixes #6441

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline, Pull Request section?
  • Was this discussed/approved via a GitHub issue? Please add a link to it if that's the case.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

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.

  • No AI usage: the PR was written entirely by a human.
  • AI-assisted: some parts were suggested or improved by AI, but the PR was written and reviewed by a human.
  • AI-generated: the PR was mostly or fully generated by an AI tool.

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

Low Risk
Small, localized loss-formula fix with unchanged default reverse_kl behavior; medium impact only for users combining apo_down with non-default f_divergence_type.

Overview
Fixes apo_down in DPOTrainer so both loss terms use raw policy-vs-reference log-ratios (APO Eq. 8), like apo_zero, instead of mixing a raw chosen term with a rejected term built from delta_score (f-divergence–transformed scores).

With non-default f_divergence_type (forward_kl, js_divergence, alpha_divergence), that mismatch silently changed training; default reverse_kl only looked correct by coincidence. Default reverse_kl behavior is unchanged.

Adds test_apo_down_loss_is_f_divergence_invariant, which perturbs weights so log-ratios are non-zero and asserts compute_loss matches the reverse_kl reference across other f-divergence settings (fixes #6441).

Reviewed by Cursor Bugbot for commit a74c1a8. Bugbot is set up for automated code reviews on this repo. Configure here.

apo_down mixed a raw chosen term with an f-divergence-transformed
rejected term via delta_score. The APO paper (Eq. 8) and apo_zero use
raw reverse-KL log-ratios for both terms; mirror that so the loss is
invariant to f_divergence_type.

Fixes huggingface#6441
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DPOTrainer apo_down loss inconsistently applies f_divergence_type across its two terms

1 participant