Skip to content

Set explicit neutral top_p in GKD on-policy generation - #6337

Merged
albertvillanova merged 2 commits into
mainfrom
gkd-onpolicy-top-p
Jul 10, 2026
Merged

albertvillanova merged 2 commits into
mainfrom
gkd-onpolicy-top-p

Conversation

@sergiopaniego

@sergiopaniego sergiopaniego commented Jul 9, 2026 •

Copy link
Copy Markdown
Member

What does this PR do?

Since transformers v5, unset GenerationConfig fields default to None, which means "inherit the value from the model's generation_config" (huggingface/transformers#42702). GKDTrainer builds its on-policy generation config with an explicit top_k=0 but no top_p, so models that ship sampling defaults (e.g. Qwen3 or Gemma with top_p=0.95) silently truncate the on-policy sampling distribution during training.

Repro (CPU):

from transformers import AutoModelForCausalLM, GenerationConfig

model = AutoModelForCausalLM.from_pretrained("trl-internal-testing/tiny-Qwen3ForCausalLM")
model.generation_config.top_p = 0.33
generation_config = GenerationConfig(max_new_tokens=4, temperature=0.9, do_sample=True, top_k=0)
prepared, _ = model._prepare_generation_config(generation_config)
print(prepared.top_p)  # 0.33 (inherited), expected 1.0

This sets an explicit neutral top_p=1.0, completing the intent of the explicit top_k=0 and matching the sibling trainers (PPO hardcodes top_p: 1.0; GOLD, Distillation, SDFT, SSD and SDPO set top_p explicitly from their configs).

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?

@kashif


Note

Low Risk
Small, targeted change to GKD training-time generation kwargs; improves consistency with intended temperature-only sampling and aligns with other TRL trainers, with no auth or data-path impact.

Overview
GKD on-policy sampling no longer inherits the student model’s default top_p (e.g. 0.95 on Qwen/Gemma). GKDTrainer now sets top_p=1.0 alongside the existing top_k=0, so transformers v5’s “unset field inherits from model.generation_config” behavior cannot narrow the training-time sampling distribution.

test_generation_config_init asserts trainer.generation_config.top_p == 1.0.

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

@bot-ci-comment

bot-ci-comment Bot commented Jul 9, 2026

Copy link
Copy Markdown

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.

@albertvillanova albertvillanova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for for the detailed report and the fix!

The fix is minimal, the reasoning is sound, and it closes a real silent-truncation bug for models that ship sampling defaults (Qwen3, Gemma, etc.).

@albertvillanova
albertvillanova merged commit e97bc0a into main Jul 10, 2026
5 checks passed
@albertvillanova
albertvillanova deleted the gkd-onpolicy-top-p branch July 10, 2026 10:45
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.

2 participants