[Seq Packing] Loss normalization under packing (P3): unpack aggregation + global grad weighting#1662
Open
yuxuandexter wants to merge 1 commit into
Open
Conversation
…on + global grad weighting Layer 1 (per-micro-batch loss value): aggregate_loss ran on the packed [P, T] tensor and treated each pack row as one sequence, so every sequence-mean loss_agg_mode reduced incorrectly. aggregate_loss_packed now unpacks the per-token [P, T] tensors to rectangular [n_max, T] (row = sequence, veRL pad_input equivalent) via a segment_ids scatter, then runs the original aggregate_loss so all 5 modes reduce per-sequence. segment_ids=None delegates unchanged (non-packed path byte-identical). - common.py: make_unpack_indices + aggregate_loss_packed (with n_max>0 assert); seq-mean-token-mean/scale denominator -> non-empty-row count so unpacked padding rows do not inflate it. - algo_core.py: 4 aggregate_loss call sites -> aggregate_loss_packed. - agentic_grpo_learner.py: pass unpack_n_max = train_micro * num_generations. Layer 2 (cross-micro-batch gradient, b/491970038): peft_trainer accumulated a mean-of-means (local 1/denom scale + denom=1). Now accumulates grad(sum) with the loss's real denominator, so the optimizer sees Sum grads / Sum denom (token-weighted for token-mean, sequence-weighted for sequence-mean). Tests: common_test AggregateLossPackedTest (5 modes packed == rectangular), peft_trainer_test TrainStepGlobalWeightedTest.
yuxuandexter
requested review from
abheesht17,
hgao327,
jiangyangmu,
lc5211,
s-noghabi,
sizhit2,
tianshub and
wang2yn84
as code owners
July 10, 2026 23:55
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
Loss normalization correctness under sequence packing (P3), stacked on the packed-logprob + pack-first PR (#1659). Two independent layers:
Layer 1 — per-sequence aggregation (unpack, veRL
pad_inputequivalent)With packing,
per_token_lossis[num_packs, T]where each row holds ~8 sequences, butaggregate_lossonly knowscompletion_mask— it treats a whole pack row as one sequence, so every sequence-mean mode is wrong (only token-mean is packing-invariant).Fix (mirrors veRL, which unpacks logprobs back to
[batch, resp_len]before the loss so aggregation is transparent to packing):common.py:make_unpack_indices+aggregate_loss_packed. It scatters the packed[num_packs, T]tensor back to a rectangular[n_max, T](row = sequence) via asegment_idsscatter, then runs the originalaggregate_lossso all 5 modes reduce per-sequence.segment_ids=Nonedelegates unchanged (non-packed path byte-identical).common.py:sequence-mean-token-mean/sequence-mean-token-scaledenominators use the non-empty-row count so unpacked padding rows do not inflate them.algo_core.py: 4aggregate_losscall sites (clipfrac / main loss / kl / entropy) →aggregate_loss_packed.agentic_grpo_learner.py: passunpack_n_max = train_micro_batch_size * num_generations(a static config constant).Layer 2 — cross-micro-batch gradient (b/491970038)
peft_traineraccumulated a mean-of-means (local1/denomscale +denom=1). Sincediff_fndifferentiates the unreduced sum,grads = grad(sum). Now it accumulatesgrad(sum)with the loss's real denominator, so the optimizer seesΣ grads / Σ denom— token-weighted for token-mean, sequence-weighted for sequence-mean. This is exact per mode (each mode contributes its own denominator), whereas veRL uses a fixed sequence count.Tests
tests/rl/common_test.py::AggregateLossPackedTest— all 5 modes:aggregate_loss_packed(packed) == aggregate_loss(rectangular)(atol 1e-4); token-mean ground truth; decision-B empty-row protection;segment_ids=Nonematches the original (non-packed unchanged).tests/sft/peft_trainer_test.py::TrainStepGlobalWeightedTest— uneven denominators across micro-batches:get() == Σ grads / Σ denom(global), and differs from mean-of-means.Notes
Stacked on
yuxzhang/unified_seqpack(#1659); depends on itsLossOutput/WeightedMetricinfra. Re-target tomainonce #1659 merges.