feat(grpo): train all Gym traces per logical rollout - #4078
Open
esarafian wants to merge 6 commits into
Open
Conversation
Signed-off-by: Elad Sarafian <esarafian@nvidia.com>
Signed-off-by: Elad Sarafian <esarafian@nvidia.com>
Signed-off-by: Elad Sarafian <esarafian@nvidia.com>
Signed-off-by: Elad Sarafian <esarafian@nvidia.com>
Signed-off-by: Elad Sarafian <esarafian@nvidia.com>
5 tasks
Signed-off-by: Elad Sarafian <esarafian@nvidia.com>
esarafian
marked this pull request as ready for review
September 10, 2026 12:42
Author
|
/ok to test a425d8b |
Author
|
/claude review |
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 does this PR do ?
Add opt-in synchronous Megatron GRPO training for all independently conditioned Gym traces in a logical rollout, including branches and compacted contexts.
Compute GRPO advantages once per original task's logical rollouts, then expand to physical training rows. Preserve captured token IDs, behavior logprobs, masks, and unique sampled-span ownership. The objective averages unique eligible sampled tokens; physical rows share one optimizer update, while the learning-rate scheduler advances by logical rollout count. Zero-loss padding accommodates unequal trace counts without dropping trailing traces. Overlong traces are masked individually, and invalid rollouts are excluded from the logical baseline. The Gym actor assigns capture identity before synchronous dispatch, preserves supplied IDs, and uses Gym's canonical correlation rules for finalization. Zero-call captures become masked inert rows; missing or malformed correlation remains an error. The existing Gym path remains the default.
Issues
No automatic issue closure. This implements a bounded synchronous multi-trace path; asynchronous/TQ/staged delivery and process-level credit assignment remain outside this change.
Usage
Apply these overrides to a working synchronous Gym Megatron recipe:
Merge dependency: NVIDIA-NeMo/Gym#3212. The RL change should merge after that companion PR. The Gym submodule is pinned to its implementation at 407fcf9f, which supplies the
training_tracesv1 envelope.uv.lockreflects that revision's dependency requirements. See the multi-trace guide for identity, masking, objective, and supported configuration details.Before your PR is "Ready for review"
Pre checks:
Additional Information
Validation on the matching NeMo RL nightly environment:
--noconftest -o addopts=""to avoid starting or stopping Ray during the separate runtime experiment. It is not a full CI shard result.git diff --checkanduv lock --checkpassed.per_requestandprefix_mergingon RLa425d8b3c0bb991caeb40cf31ad6dd995d373df6with Gym407fcf9f55e31bcd62290e6a3050f01ef8dbba3f. Each run completed two updates, saved a checkpoint, advanced Adam to step 2 and the scheduler by 16 logical rollouts, and changed sampled attention weights.per_requestand 9,801 forprefix_merging, with no excluded, invalid, overlong, or padding rows. All had positive importance-sampling weights. The tool-call portion contributed 672 eligible tokens in each run; assistant answers contributed 8,560 and 9,129 respectively.per_requesttokens and 2,320prefix_mergingtokens had nonzero advantages, including 168 tool-call tokens in each. Token inclusion and positive weights do not assert nonzero gradient for every token; PPO clipping and zero advantages remain distinct.uv lock --check; no lockfile changes were required.Setup explicitly rejects unsupported asynchronous/TQ/staged capture, non-Megatron or multimodal policies, alternative advantage estimators, sequence-level loss/ratios, active
seq-mask-tis, and legacy filtering/penalty options that would change the logical multi-trace semantics. Outcome reward broadcast is not process credit assignment or session-normalized weighting.Public CI status: all displayed checks on the final head passed, but the NVIDIA main test workflow remains behind maintainer approval and the required
CI:L2label. Our label request was denied by repository permissions. The official Claude review workflow was triggered and failed before reviewing because its app-token exchange requires repository write access; no review findings were produced. Local tests and independent review do not substitute for these pending official checks.