Warn when truncated sampling biases the vLLM importance-sampling ratio - #6880
Open
behroozazarkhalili wants to merge 1 commit into
Open
Warn when truncated sampling biases the vLLM importance-sampling ratio#6880behroozazarkhalili wants to merge 1 commit into
behroozazarkhalili wants to merge 1 commit into
Conversation
…mpling ratio
GRPO asks vLLM for `processed_logprobs`, which are renormalized over the support
that survives top_p/top_k/min_p, while the trainer takes a full-vocab log-softmax.
`grpo_trainer.py:2680` differences the two, so the result carries log(S), the log
of the surviving probability mass, on top of the train/inference mismatch the
correction exists to measure.
Measured on one H100 with GRPOTrainer in colocate mode,
`sampling/sampling_logp_difference/mean` reads |log(top_p)| exactly:
top_p predicted measured
1.0 0.0000 0.0007
0.9 0.1054 0.1054
0.8 0.2231 0.2232
The sequence-level `sampling/importance_sampling_ratio/mean` confirms the same
mechanism through a different quantity, reading S^16 for 16-token completions:
0.1855 against 0.185302 at top_p=0.9, and 0.02814 against 0.028147 at top_p=0.8.
The correction therefore scales the policy gradient by a factor the user did not
ask for, with no error and no log line. This warns at config time instead of
changing the math, because a correct fix needs the kept-token set from the sampler
and vLLM only returns it from a release outside the current vllm<=0.27.1 pin.
The guard is in `GRPOConfig.__post_init__`, so it reaches `GRPOWithReplayBufferConfig`
and the gspo_token variant through inheritance. RLOO is unaffected because it has no
importance-sampling correction.
|
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. |
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.
What this does
Warns when
vllm_importance_sampling_correction=Trueis combined withtop_p < 1,top_k > 0, ormin_p > 0.Refs #6789. This is direction 4 from that issue, the warning, not the math fix. It forecloses none of the other directions.
Why
vLLM is asked for
processed_logprobs, which are renormalized over the support that survives truncation, while the trainer takes a full-vocab log-softmax.grpo_trainer.py:2680differences the two:so the result is
log p_hf - log p_vllm + log S, whereSis the surviving mass. The first two terms are the train/inference mismatch the correction exists to measure.log Sis not, and it flows straight intovllm_importance_sampling_ratio, which multiplies the per-token loss.Measured on one H100, GRPOTrainer in colocate mode,
trl-internal-testing/small-Qwen2ForCausalLM-2.5,max_completion_length=16. The predicted values were fixed before the run:abs(log S)A second logged quantity confirms the same mechanism independently. With 16 completion tokens the sequence-level correction is
S^16, andsampling/importance_sampling_ratio/meanreads 0.1855 against a predicted 0.185302 attop_p=0.9, and 0.02814 against 0.028147 attop_p=0.8. Both agree to four decimals, which fixes the per-token surviving mass at exactlyS = top_p.At
top_p=0.8that weights a 16-token completion at 2.8 percent of its uncorrected value, with no error and no log line.Why warn rather than fix the math
A correct fix needs the kept-token set the sampler actually used. Both frameworks the issue cites do exactly that: prime-rl #3235 transports kept ids out of vLLM's engine-core worker, and slime v0.3.1 requires
rollout_top_p_token_idswheneverrollout_top_pis not 1.0. vLLM exposes the set only in #49577, which is merged but lands after 0.27.1 and so falls outside the currentvllm>=0.18.0,<=0.27.1pin.Recomputing the mask from training-side logits is possible today but is an approximation, and it has a sharp edge: the sampled token can fall outside the recomputed nucleus, giving
-infand a NaN batch unless it is force-included. I left that for the design discussion on the issue.Scope
The guard is in
GRPOConfig.__post_init__, soGRPOWithReplayBufferConfiginherits it and the gspo_token variant picks it up through the shared config. RLOO is untouched because it has no importance-sampling correction at all.Verification
should_warn=Truecases fail before the guard, all six pass after.top_p,top_k, andmin_pconditions, and removing theuse_vllm and correctiongate, each kills a test. The file is byte-identical after restore.ruff0.13.3 check and format clean, version read from.pre-commit-config.yaml.doc-builderat the pinned2430c1ewith--max_len 119clean on both files, withtrl/trainer/sft_trainer.pyas a positive control.Note
Low Risk
Config-time UserWarning only; training math and sampling behavior are unchanged.
Overview
Warns when vLLM importance-sampling correction is used with truncated sampling (
top_p < 1,top_k > 0, ormin_p > 0).vLLM logprobs are renormalized over the surviving support while the trainer uses a full-vocab log-softmax, so the IS ratio includes
log S(surviving mass) on top of the train/inference mismatch.GRPOConfig.__post_init__now emits aUserWarningin that combination and stays silent when correction is off or sampling is untruncated. Tests cover the truncation knobs and the no-correction case.Reviewed by Cursor Bugbot for commit 6cca24b. Bugbot is set up for automated code reviews on this repo. Configure here.