Repository navigation
fix(loss): apply loss_mask in agg_loss token-mean - #509
Open
flyingdoubleG wants to merge 1 commit into
Open
flyingdoubleG wants to merge 1 commit into
flyingdoubleG wants to merge 1 commit into
Conversation
The token-mean branch summed loss_mat over every position and divided by the masked token count. Since v0.2.0 (3077bef), when the global-token normalization was added, the mask is no longer applied, so tokens that the caller masked out still add to the loss and receive gradient: padding and, in the agentic and RLVR actor workers, tokens removed by the train-infer correction filter (response_mask * filter_mask), whose policy-gradient loss is not zero. The clip-fraction metrics computed with token-mean count those tokens too. Use masked_sum(loss_mat * weights, loss_mask) / batch_num_tokens, as the other modes already do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
agg_loss(..., loss_agg_mode="token-mean")sumsloss_matover every position and divides by the masked token count:loss_maskis not applied to the numerator, so tokens the caller masked out still add to the loss and receive gradient. The other modes usemasked_sum/masked_meanand are not affected. This came in with v0.2.0 (3077bef), wheremasked_mean(loss_mat * weights, loss_mask)was replaced to support the globalbatch_num_tokens; the mask was dropped in that change.Where it matters
AgenticActorPGWorker.loss_funcand the RLVRActorPGWorker.loss_funcapply the train-infer correction filter asresponse_mask * filter_mask, then aggregate the policy-gradient loss withagg_loss(pg_loss, response_mask, loss_agg_mode, ...). The IS weight is not zero on filtered tokens and their advantages are not masked, so withloss_agg_mode: token-meanthe filtered tokens are still trained on. Five example configs usetoken-mean(e.g.examples/qwen3-8B-gsm8k-math-rlvr_megatron/rlvr_config.yaml).token-mean, also count masked positions.Minimal check with the function from
roll/utils/functionals.py(one token filtered, one padding token, loss 1 everywhere):token-mean(before)token-mean(after)seq-mean-token-meanChange
batch_num_tokenskeeps its meaning (global valid-token count), so nothing changes whenloss_matis already zero outside the mask.Tests
tests/utils/test_functionals.py: masked tokens change neither the value nor the gradient fortoken-mean,seq-mean-token-sumandseq-mean-token-mean;token-meanwith weights divides the local masked sum by the global token count. Bothtoken-meancases fail before the change and pass after; the file passes (8 tests, CPU).A related question (not changed here)
seq-mean-token-sum-normreturnssum(seq_losses) / loss_mask.shape[-1]without dividing by the number of sequences, so its scale grows with the batch size, unlike the otherseq-mean-*modes. This matches the verl version cited in the docstring; verl later added the sequence mean (verl-project/verl#5417). If you'd like the same here I can send a separate PR, since it changes the loss scale for existing configs.🤖 Generated with Claude Code