fix(grpo): replay buffer contract corruptions advantages (#2943) - #2981
Open
n-dlms wants to merge 1 commit into
Open
fix(grpo): replay buffer contract corruptions advantages (#2943)#2981n-dlms wants to merge 1 commit into
n-dlms wants to merge 1 commit into
Conversation
… + data-path hardening Replay buffer storage counts rows, not batches. With max_size=1 and batch_size=16, every training step drew 16 copies of the last surviving sample, producing the 0.6057=AE16 identical advantages reported in meta-pytorch#2943. Fix: store whole batch as one buffer item (extend(traj.unsqueeze(0)) / sample(1).squeeze(0)), remove the list[RewardOutput] field that silently kids batch-dimension mismatches on modern tensordict, and wire reward- function names end-to-end through a shared helper. Co-changes pulled from every coded device path and restored stale FIXMEs (context_length, device hard-code, sequence-ids), plus extracting group_normalized_advantages into a shared rewards utility used by both sync and async branches. Two new CPU-test suites (35 tests) cover: corruption regression, round-trip exactness per every field, gradient-oracle linearity on effects, and fuzz over varied batch/parcel/capacity shapes.
This was referenced Aug 2, 2026
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 #2943.
Root cause
The replay buffer storage counts rows (individual samples) as items, not batches. When
PostProcessingWorkercallsextend(trajectory)wheretrajectoryhas batch[16]and the buffer hasmax_size = replay_buffer_size = ${inference.batch_size} = 1, only the very last row survives.TrainingWorker.sample(batch_size=16)then draws 16 copies with replacement from that single row=E2=80=94producing the diagnostic[0.6057 =C3=97 16]identical advantages reported in #2943.What changes
PostProcessingWorker.runstores whole batch as one buffer item (extend(traj.unsqueeze(0))/sample(1).squeeze(0))RayReplayBufferbatch_size=1(wascfg.training.batch_size)sample()without a specific count draws whatever the constructor said=E2=80=94withbatch_size=16that=E2=80=99s 16 duplicate copies of the whole batch.Trajectorynow carries per-samplerewards,successes,reward_func_names(tensors / NonTensorStack) instead ofreward_outputs: list[RewardOutput]list-typed field caused batch-dimension validation failures in modern tensordict when the buffer stacks items from different iterations=E2=80=94and reward function names round-trip through the queue end-to-end for the debug table.reward_func_names_per_samplein postprocessingRewardOutput.reward_base_nameis available, so it fills the gap. (The oldmetadata["reward_Cmd"][idx]path would crash on ``None```=E2=80=94this gap was latent in the original code.)group_normalized_advantagesinrewards.pyPlus resolved stale FIXMEs:
context_lengthper-batch comment in grpo_step (truncate_sequence_at_first_stop_tokenspads rather than removes columns=E2=80=94pad_output=Trueensures the context length is uniform per batch)torch.device("cuda:0")=E2=80/ cfg-supplied Device in TrainingWorkerAlso added
device: cudato the async GRPO yaml and replaced the misleading buffer-size comment in that yaml.Test coverage (all CPU-runnable, passing)
Two new test suites=E2=80=9429 tests total:
test_replay_buffer_contract.py(5 tests) =E2=80=93 regression showing the exact[value=x_16]corruption; round-trip exactness of every per-sample field (questions, roofprobs, advantages, rewards, successes, heq_ids, answers, policy-ver, seq_lens); capacity-counts-batches, batch-repeat detection, group-normalization properties.test_async_data_path.py(24 tests) =E2=80=93 gradient-oracle telling us advantages always linline through the GRPO loss (``kl_coeff=0=E2=86=92 loss =F1=80=AE-advantages.mean(), grad doubling); fuzzed round-trip with varied (B,G,T,funcs,capacity) shapes; theBug inasync_grpo_full_finetunerecipe: TrainingWorker receives identical advantages #2943` headline assertion =C3=ABadvantages differ across samples after the buffer (=C3=85) and reward-function names survive the buffer.Existing rewards unit tests continue to pass. The GPU-gated
test_postprocessing.pyis mechanically updated for the new Trajectory fields but stays skipped in this environment (no GPU).NOT verified here (cannot without vLLM + GPU + multi-node)
Related follow-ups
torchao incompatibility: current
torchtunebase install on torchao =E2=89=AE 0.9 fails because NF4Tensor moved out oftorchao.quantization. A companion pin clause is warranted (separate PR).torchrl / SyncDataCollector: the async extra pins a stale git commit (<0.13); updating is separately tracked.