Skip to content

[GRPO] Multiply the KL term by the per-token importance sampling ratio - #7188

Open
Alexander230 wants to merge 1 commit into
huggingface:mainfrom
Alexander230:fix/6586-grpo-kl-bias-correction-seq-level
Open

[GRPO] Multiply the KL term by the per-token importance sampling ratio#7188
Alexander230 wants to merge 1 commit into
huggingface:mainfrom
Alexander230:fix/6586-grpo-kl-bias-correction-seq-level

Conversation

@Alexander230

@Alexander230 Alexander230 commented Sep 11, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes #6586.

The correction now always uses the per-token ratio, so the result no longer depends on importance_sampling_level; token-level behaviour is unchanged.

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

  • AI-assisted: some parts were suggested or improved by AI, but the PR was written and reviewed by a human.

Who can review?

Anyone

🤖 Generated with Claude Code


Note

Medium Risk
Changes GRPO loss gradients whenever beta != 0 and use_bias_correction_kl is enabled with importance_sampling_level="sequence", which can alter off-policy training behavior.

Overview
Fixes #6586 by changing how bias-corrected KL is weighted in GRPOTrainer._compute_loss: when use_bias_correction_kl is on, the KL term is multiplied by the per-token importance ratio exp(log_ratio) instead of coef_1 (which follows importance_sampling_level and can be sequence-aggregated).

Policy-loss importance sampling is unchanged; only the KL correction is decoupled so sequence-level IS no longer broadcast onto per-token KL. GRPOConfig docs now state that KL bias correction always uses the per-token ratio.

A regression test builds synthetic batches where sequence-level ratios cancel to 1 but per-token ratios do not, with zero advantages so loss is KL-only, and asserts _compute_loss matches for "sequence" vs "token" IS levels.

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

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.

KL bias correction with importance_sampling_level="sequence" broadcasts a sequence-level ratio onto per-token KL

1 participant