Exclude padding from RL trainer diagnostics, and fix three related metrics - #6861
Open
behroozazarkhalili wants to merge 13 commits into
Open
Exclude padding from RL trainer diagnostics, and fix three related metrics#6861behroozazarkhalili wants to merge 13 commits into
behroozazarkhalili wants to merge 13 commits into
Conversation
|
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. |
Two of the four are padding problems and two are not. No loss or gradient path changes in any of them. Online DPO summed raw `(B, T)` logprobs for `objective/kl`, `objective/entropy` and, through `non_score_reward`, `objective/rlhf_reward`, two lines below a loss that already multiplies by `~cr_padding_mask`. PPO masked `pg_loss` and `pg_clipfrac` but left `policy/approxkl_avg`, `policy/entropy_avg` and `policy/ratio_avg` on a bare `.mean()`. Both now use the mask already in scope. The padded positions hold `log p(pad | context)`: `_forward` returns `selective_log_softmax` over the full padded width and nothing zeroes it before the stat sites. That value rises toward zero as the policy learns to emit pad after EOS, so the contamination shrinks during training instead of sitting at a fixed offset. Holding the true masked entropy fixed and moving only `p(pad|ctx)` from 0.01 to 0.99 slides the reported value from 48.99 to 37.51, a downward trend that nothing real is producing. XPO logged `logps/chosen` and `logps/rejected` as policy plus reference logps. Online DPO logs the policy term alone and Nash-MD logs the two separately, so the sum matches neither and has no interpretation. Now the policy term. GRPO computed `policy_loss` on every loss branch but appended it only inside the entropy-bonus block, so the metric vanished for standard runs. The append moves out. Its value is unchanged: it is captured after the KL term is folded in, so with `beta != 0` it carries that too, and excludes only the entropy bonus and the MoE auxiliary loss. Resolves #6809
behroozazarkhalili
force-pushed
the
fix/6809-rl-metrics-masking
branch
from
August 21, 2026 17:04
654abcd to
c31418c
Compare
The PPO statistics buffers took their last axis from gradient_accumulation_steps, but the micro-batch loop writes local_mini_batch_size // per_device_train_batch_size slots per minibatch and resets its index for each one. The unwritten slots stayed at zero, and every statistic is reduced with .mean(), so approxkl, pg_clipfrac, pg_loss, vf_loss, vf_clipfrac, entropy and ratio all came out scaled by 1 / num_mini_batches. They were right only at the default num_mini_batches=1. Taking the axis from the same loop bounds leaves no unwritten slot: over the 86 configurations the exact-division checks admit, the old size is wrong in 50 and the new size in none. The new test pins the scaling through ratio, which is exactly 1 while the policy has not moved yet. With two minibatches it reported 0.5 before this change. GRPO recorded policy_loss only in _compute_loss. compute_loss sends Liger runs to compute_liger_loss and returns, so a Liger run never reached that line, though the comment above it says the metric is logged for every run. The Liger path now records it where the non-Liger path does: DAPO, CISPO and VESPO divide the normalizer in as they build the loss and capture afterwards, and the remaining loss types capture before the accumulation rescale.
The doc-builder style hook reflows docstrings to a wider column than these were wrapped at, so it rewrote both files and failed the quality check. This is its own output.
This was referenced Sep 2, 2026
`masked_mean` divided by `mask.sum()` with no floor. A response whose first token is the pad token yields `sequence_length = -1`, so `~padding_mask` is empty, the divisor is zero, and the result is NaN where the bare `.mean()` it replaced returned a finite number. The statistics buffers are reduced with `.mean()`, so one such micro-batch turned `policy/approxkl_avg`, `policy/entropy_avg`, `val/ratio` and `val/ratio_var` into NaN for the entire update, which is the opposite of what moving those metrics to a masked mean was meant to achieve. Clamping the denominator reports 0 for the empty case and leaves every finite case bit-identical, matching the `.clamp(min=1.0)` normalization already used throughout the library. Rebuild the Liger `policy_loss` parity test so it can actually fail. The old configuration let the DAPO normalizer collapse to 1, where both branches of the capture-point ternary agree, and the loss it compared was structurally zero because GRPO advantages are centred within a group and the policy ratio is exactly 1 without `old_per_token_logps`. The test now offsets the fused loss by a constant, reads the accumulation count where `compute_liger_loss` reads it rather than after `train()` returns, and requires that at least one call had a normalizer other than 1. Both parametrizations pass clean and both fail against an inverted ternary. Correct three statements that were false as written: the PPO comment claimed every statistic is mean-reduced, but `val/ratio_var` uses `.var()`, where the padding zeros manufacture spread instead of scaling it; the Liger test comment said the accumulation count always falls to the epoch remainder on the last optimizer step, which does not hold when the dataloader length divides evenly; and the `policy_loss` doc entry did not say that `cispo`, `dapo` and `vespo` report a per-micro-batch value while the other loss types report the window value.
The previous commit clamped the `masked_mean` denominator so an all-padding micro-batch yields a loss of 0 instead of NaN. That kept the weights finite but had two side effects. The 0 it returns was then written into the statistics buffers, where the `.mean()` reductions counted it as an observation: with one empty and one real micro-batch, `val/ratio` read 0.5 for a real ratio of 1. Each buffer now starts as NaN, a slot is written only when the micro-batch holds at least one valid token, and the reductions use `nanmean`; `val/ratio_var` is the variance of the written slots. The loss path is unchanged, so the optimizer still sees 0 for an empty micro-batch and the accumulation cadence is untouched. `clamp(min=1.0)` also rescaled every mask whose sum was below one: a fractional mask of 0.5 on a value of 2.0 returned 1.0 where the plain division returned 2.0. Only an exactly zero denominator is replaced now, and the comment no longer claims the clamp matched the rest of the library, which normalizes both ways. The `policy_loss` note in grpo_trainer.md said the per-micro-batch value for cispo, dapo and vespo is smaller by `current_gradient_accumulation_steps / steps_per_generation`. The code divides by that ratio, and `gradient_accumulation_steps=1` with `steps_per_generation=2` is an accepted configuration where it is 0.5 and the value is larger. The note now says what the division is and when it shrinks the value. A regression test makes generation emit the pad token first for one row, which gives that row a sequence length of -1 and an all-padding micro-batch, and asserts `val/ratio` stays within 1e-4 of 1; on the previous commit it reads 0.5. The test pads with the tokenizer's own pad token: padding with EOS instead moved the real micro-batch's ratio by up to 2e-3 even without the forced row, while the distinct pad token keeps it within 1.2e-7. A second regression test covers the other half of the `policy_loss` change in this PR. On main the append sits inside `if self._entropy_bonus_enabled:`, so a run with the default `entropy_coef=0.0` logs no `policy_loss` at all; measured on main at 8397289, `entropy_coef=0.0` logs no key and `0.01` does. The two existing entropy tests both enable the bonus, so neither noticed. The new test trains one step with the default and asserts the key is logged; gating the append again makes it fail.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d6da644. Configure here.
…r run The NaN-initialised buffers from the previous commit were allocated once before the update loop. A slot is written only for a micro-batch with a valid token, so an all-padding micro-batch in a later update left the value the previous update had written there, and `nanmean` folded that stale number into the current update's averages. Before the NaN change every slot was overwritten every update, so the single allocation was safe; it stopped being safe the moment slots could be skipped. The buffers are now created at the top of each update. A regression test makes every row of the second update all padding: that update has nothing to report, so its `val/ratio` must be NaN, while the stale buffers reported the first update's value.
# Conflicts: # tests/test_grpo_trainer.py
…e metric values The value loss and value clip fraction were gated by the policy mask, so a micro-batch whose only response is empty dropped its value statistics although its first value timestep is valid through padding_mask_p1. The two value slots now use the value mask. The PPO all-padding test was not an oracle: padded old and new log-probabilities share a sentinel, so the unmasked ratio is exactly one on the previous code too. It now checks the valid-token entropy against logits it controls and requires the empty row's value loss to be logged. Online DPO gains an exact oracle for the masked KL, entropy and reward metrics; XPO gains one showing logps/chosen and logps/rejected hold policy log-probabilities only.
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.

Fixes the four issues reported in #6809. This PR primarily corrects reported metrics. It also changes PPO optimization for one edge case:
masked_meannow returns 0 on an empty mask, so a micro-batch whose policy mask is empty produces a zero policy loss and zero gradients instead of NaN. Losses and gradients for every nonempty mask are unchanged.Credit to @mmjerge, who found all four during a code audit.
The four
Two are padding problems and two are not, so they are worth separating.
mainobjective/kl,objective/non_score_reward,objective/rlhf_reward,objective/entropypolicy/approxkl_avg,policy/entropy_avg,policy/ratio_avg.mean()whilepg_lossbeside them is maskedlogps/chosen,logps/rejectedpolicy_lossEvery fix uses a mask or a value already in scope at that site, so nothing new is plumbed through. For XPO, Online DPO logs the policy term alone and Nash-MD logs the two separately; the sum matches neither, so this now logs the policy term.
What the padding actually contains
_forwardreturnsselective_log_softmaxover the full padded width and nothing zeroes it before the stat sites, so the padded entries holdlog p(pad | context). That value rises toward zero as the policy learns to emit pad after EOS, which means the contamination shrinks as training proceeds rather than sitting at a fixed offset.Holding the true masked
objective/entropyfixed at37.4808and varying onlyp(pad|ctx):The reported value falls by 11.5 while the quantity it measures never moves, so the metric shows a downward trend that nothing real is producing. The effect is largest early in training, when these curves get used to decide whether a run is worth continuing.
Those
p(pad|ctx)values are plausible rather than measured from a live run; pinning the real distribution needs a GPU run this change does not otherwise require. The identity below does not depend on them.Verification
1.8e-14, which is float64 summation rounding. It is zero exactly when every padded entry is zero, which no code path arranges.ruff@0.13.3check and format clean on all four files, and the pinned doc-builder gate clean.logprobsreaches the loss only throughcr_logprobsat line 1213, and line 1220 already multiplies that by~cr_padding_mask, so the new mask is idempotent there. In PPO the changes are confined to the three*_statsbuffers. XPO and GRPO change only what is appended toself.statsandself._metrics.policy_lossis logged before any regularizer. It is captured afterper_token_loss = per_token_loss + self.beta * per_token_kl, so withbeta != 0it includes KL. The comment now states that. The metric's value is deliberately unchanged, sincepolicy_loss = loss.detach()is pre-existing and this PR moves only where it is appended.Note for the GRPO item
trl/experimental/gspo_token/grpo_trainer.pycopies_compute_loss, and #6856 re-syncs that copy againstmainincluding this same gated append. Whichever lands second should carry thepolicy_losschange across, so the copies do not drift apart again.Resolves #6809
Note
Medium Risk
Changes affect widely watched training metrics and PPO statistic reduction; only behavioral training change is zero policy loss on all-padding micro-batches, while actual optimization paths are otherwise unchanged per the PR.
Overview
Corrects logged metrics across Online DPO, PPO, XPO, and GRPO so training curves match what the loss paths actually optimize—without changing those loss computations in the common case.
Online DPO masks padded completion positions when aggregating
objective/kl,objective/entropy, and related objective stats (aligned with the existing loss mask).XPO reports
logps/chosenandlogps/rejectedas policy log-probability sums only, instead of policy plus reference.GRPO always logs
policy_loss(including defaultentropy_coef=0and Liger runs), with capture timing documented and matched per loss family (dapo/cispo/vespovs others).grpo_trainer.mdis updated accordingly.PPO fixes several aggregation bugs: statistic buffers are sized to real micro-batches per minibatch (fixing ~
1/num_mini_batchesdilution), reinitialized each update with NaN slots skipped viananmean, policy vs value masks split for zero-length responses, andapproxkl/entropy/ratio use masked means.masked_meanon an empty policy mask now returns 0 (zero policy loss on that micro-batch instead of NaN).New unit tests pin the expected metric values for these cases.
Reviewed by Cursor Bugbot for commit 4d77d03. Bugbot is set up for automated code reviews on this repo. Configure here.
Update: two further metric defects
PPO statistics were scaled by
1 / num_mini_batches. The buffers inppo_trainer.pytook their last axis fromgradient_accumulation_steps, while the micro-batch loop writeslocal_mini_batch_size // per_device_train_batch_sizeslots per minibatch and resets its index for each one. The unwritten slots stayed at zero and every statistic is reduced with.mean(), soapproxkl,pg_clipfrac,pg_loss,vf_loss,vf_clipfrac,entropyandratiowere all under-reported by that factor. They were right only at the defaultnum_mini_batches=1. The axis now comes from the same loop bounds: across the 86 configurations the exact-division checks admit, the old size is wrong in 50 and the new size in none.The regression test pins this through
ratio, which is exactly 1 while the policy has not moved. Withnum_mini_batches=2it fails on the old code withassert 0.4999999701976776 == 1.0 ± 1.0e-04, and passes on the new code.policy_losswas never logged for Liger runs.compute_losssends them tocompute_liger_lossand returns, so they never reached the append in_compute_loss, even though the comment there says the metric is logged for every run. The Liger path now records it at the point that matches the non-Liger capture: DAPO, CISPO and VESPO divide the normalizer in while building the loss and capture afterwards, and the other loss types capture before the accumulation rescale. The new test covers one loss type from each group.Update: value diagnostics for zero-length responses, and metric-value oracles
Value statistics were gated by the policy mask. A zero-length response has no policy timestep, but its first value timestep is valid through
padding_mask_p1, soloss/value_avgandval/clipfrac_avgwere dropped for such a micro-batch. The two value slots now use the value mask; the policy slots keep the policy mask.The tests now pin values rather than completion. The PPO all-padding test was not an oracle against the previous reduction: padded old and new log-probabilities share a sentinel, so the unmasked ratio is exactly 1 there too. It now controls the training logits, checks
policy/entropy_avgagainst the valid-token entropy, and requires the empty row's value loss to reachloss/value_avg. Online DPO gains an exact oracle forobjective/kl,objective/entropy,objective/non_score_rewardandobjective/rlhf_rewardon hand-built completion rows whose padded values would dominate an unmasked sum. XPO gains an oracle showinglogps/chosenandlogps/rejectedhold policy log-probabilities only.On a compute node (job 58035371): the three oracles pass; restoring the shared policy gate for the value slots fails the PPO test; removing the KL mask in Online DPO fails its oracle; the full PPO, Online DPO and XPO files give 88 passed, 5 skipped; ruff 0.13.3 and the pinned doc-builder gate are clean.
Two related defects live elsewhere on purpose: #6856 re-syncs the
gspo_tokencopy of_compute_loss(which carries thepolicy_lossappend), #7033 addspolicy_lossto GMPO, and #7010 covers gathering XPO per-example metrics before reduction.