Re-sync gspo_token _compute_loss with GRPOTrainer - #6856
Open
behroozazarkhalili wants to merge 13 commits into
Open
Re-sync gspo_token _compute_loss with GRPOTrainer#6856behroozazarkhalili wants to merge 13 commits into
behroozazarkhalili wants to merge 13 commits into
Conversation
`trl/experimental/gspo_token` overrides `_compute_loss` with a copy of `GRPOTrainer._compute_loss` plus one branch. The copy stopped tracking its source, so four `GRPOConfig` options, four VLM forward kwargs and one metric silently did nothing on this trainer. `use_bias_correction_kl` failed differently from the rest. It multiplies the per-token KL by `coef_1`, and the copy computed the KL forty lines before `coef_1` existed, so the option was unreachable rather than missing. Restoring the canonical statement order is what fixes it. The others were absences: the OPSM mask and its application, the entropy bonus with its adaptive controller, `compute_aux_loss` and the router auxiliary term, the `mm_token_type_ids`, `image_position_ids`, `spatial_shapes` and `num_tiles` kwargs that `_generate_and_score_completions` already puts in `inputs`, and the `policy_loss` metric. Four newer `loss_type` values raised instead, which at least failed loudly. The method is now the canonical one at 47d20ec, lines 3109 to 3379, with two divergences: the `sequence_token` branch, and an error message that names it. Reviewers can check that with: SRC=trl/trainer/grpo_trainer.py GSPO=trl/experimental/gspo_token/grpo_trainer.py diff <(git show "47d20ec6:$SRC" | sed -n '3109,3379p') \ <(sed -n '22,297p' "$GSPO") Resolves #6807
|
@behroozazarkhalili looks good thanks! |
behroozazarkhalili
marked this pull request as ready for review
August 21, 2026 15:23
The job is gated on `draft == false` and the PR was opened as a draft, so it was skipped. Marking the PR ready does not re-trigger it, because `ready_for_review` is not one of the default `pull_request` event types. An empty commit fires `synchronize`, which does. No code change: the tree is identical to 4520d7b.
|
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. |
The branch was 8 commits behind and merged with no conflicts. None of the eight touched trl/trainer/grpo_trainer.py or trl/experimental/gspo_token/, so the reference this PR syncs against did not move; they are CI and test changes only. Re-checked the invariant on the merged tree by comparing the two _compute_loss bodies as parsed ASTs: they differ in exactly 2 hunks, the sequence_token branch and the error message that lists it, which are the two intended divergences.
…om sequence The only test of the re-synced `_compute_loss` trains with one advantage per sequence. Under that input `sequence_token` and `sequence` agree in value and in gradient, so the test would pass even if the branch had collapsed to plain sequence-level GSPO. The new test calls `_compute_loss` directly with hand-built inputs. With one advantage per sequence it confirms the two levels match; with token-varying advantages it confirms the loss values still match while the parameter gradients differ, which is the whole point of the token variant: the detached sequence weight routes each token's own advantage into its gradient. Replacing the branch by the sequence branch makes the gradient assertion fail.
…not only to the other levels The gradient test compared sequence_token against sequence, so any branch that merely differed from sequence would pass, including one that forgot the stop-gradient on the sequence weight. With old_per_token_logps equal to the live log-probs the sequence weight is exactly 1 and must carry no gradient, so sequence_token has to reproduce the token-level gradient exactly. The test now asserts that equality. Removing the detach makes it fail while the relation checks still pass.
…e_token branch The existing gradient test runs with beta=0 and only exercises the sequence_token branch, which the file already had before the resync, so it passes against the pre-resync implementation and does not guard what this PR restores. The new test calls the subclass and the unbound GRPOTrainer._compute_loss on the same trainer and inputs at importance_sampling_level="token" and asserts the same loss, the same gradient and the same logged metrics, once per restored option: the bias-corrected KL, the off-policy mask and the entropy bonus. The pre-resync file fails all three rows.
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.
Re-syncs
trl/experimental/gspo_token/grpo_trainer.py::_compute_lossagainst the currentGRPOTrainer._compute_loss, as reported in #6807.Credit to @mmjerge, who found this and offered to send the patch.
What was wrong
The override is a copy of
GRPOTrainer._compute_lossplus one branch. It stopped tracking its source, so fourGRPOConfigoptions, four VLM forward kwargs and one metric silently did nothing on this trainer. All of it reproduces at47d20ec6:use_bias_correction_klcoef_1off_policy_mask_thresholdget_off_policy_maskcall and its application both absententropy_coefand the adaptive controller_entropy_bonus_enabledblock absentrouter_aux_loss_coefcompute_aux_losskwarg and the aux term absentmm_token_type_ids,image_position_ids,spatial_shapes,num_tilesabsentsequence_tokenerror messageThe issue did not list the
policy_lossmetric, whichGRPOTrainerlogs and this copy does not.use_bias_correction_klfailed differently from the rest. It multiplies the per-token KL bycoef_1, and the copy computed the KL forty lines beforecoef_1existed, so the option was unreachable rather than missing. Restoring canonical statement order is what fixes it.What this changes
_compute_lossis now the canonical method at47d20ec6, lines 3109 to 3379, with two divergences: thesequence_tokenbranch ofimportance_sampling_level, and theValueErrormessage naming that third value. Checking that claim takes one command, and it returns those two hunks and nothing else:The diff is large relative to the bug because the method was rebuilt from its source instead of patched feature by feature. Patching is how it drifted: #6024, #6380, #6439 and #6083 each touched this file and each left drift behind.
Verification
Tests (experimental)green in CI, and the 9 gspo_token tests pass on a compute node against the branch.test_matches_grpo_trainer_outside_the_sequence_token_branchcompares the subclass with the unboundGRPOTrainer._compute_losson the same trainer and inputs atimportance_sampling_level="token", in loss, gradient and logged metrics, once per restored option (bias-corrected KL, off-policy mask, entropy bonus); the pre-resync file fails all three rows, while the earlier gradient test passes on it.Check code qualitygreen, which isruff@0.13.3plus the pinned doc-builder; I ran both locally too, the doc-builder one with a negative control confirming it can still fail.torch.equalon the weight tensor returnsTrue, and the stop-gradient identityd/dlogp exp(w) == exp(seq_weight)holds.importance_sampling_level="sequence_token":importance_sampling_levelvalues, so the table isolates the options rather than the rewrite.sequence_tokenyields(B, T), the same shape as"token", so the newly reachablecispo,sapo,vespoandluspobranches broadcast correctly.Two things reported rather than fixed
gmpohas the same gaps.trl/experimental/gmpo/gmpo_trainer.pyis also missing the bias-correction KL, OPSM, the entropy bonus, the aux loss, andspatial_shapes/num_tiles. It is not a mechanical re-sync: GMPO's objective is sequence-level,coefis(B,), and there is noloss_typebranching, souse_bias_correction_kland OPSM have no direct translation. Happy to open a separate PR once you decide what those should mean there.vespoignoresimportance_sampling_level.get_gamma_weightsreceiveslog_ratio_per_token=log_ratio, sophi_seqnever seeslog_importance_weightsorcoef_1. Measured across all three levels the vespo loss is identical to twelve decimal places.GRPOTrainerbehaves the same way for"sequence", so per the AGENTS.md consistency rule I reproduced it rather than adding a guard here that would diverge from the source.Note on conflicts
#6637 and #6667 both touch this file's KL block, and this re-sync moves that block. Whichever merges second will conflict. I will rebase mine.
Resolves #6807
Note
Medium Risk
Large rewrite of RL training loss logic affects gradients and optional regularizers; risk is mitigated by parity tests against GRPOTrainer and unchanged sequence_token math.
Overview
Re-aligns
GSPOTokenTrainer._compute_losswith the currentGRPOTrainerimplementation so driftedGRPOConfigbehavior works again, while keeping the GSPO-token-onlyimportance_sampling_level="sequence_token"branch unchanged.The override again applies bias-corrected KL (
use_bias_correction_kl), off-policy masking (off_policy_mask_threshold), entropy regularization (static and adaptive), MoE router aux loss, extra VLM forward kwargs, and the full set of loss types and metrics (e.g.policy_loss, CISPO/VESPO clip logging) that the stale copy had dropped or computed in the wrong order.Tests add gradient checks that
sequence_tokenroutes per-token advantages differently fromsequence, and a parametrized parity test thatGSPOTokenTrainermatchesGRPOTraineron loss, gradients, and logged metrics whenimportance_sampling_level="token"and each restored option is enabled.Reviewed by Cursor Bugbot for commit 3cb6e52. Bugbot is set up for automated code reviews on this repo. Configure here.