fix(grpo): update grpo_sync seq_logprob_error_masking call site - #2599
Closed
ZhiyuLi-Nvidia wants to merge 1 commit into
Closed
fix(grpo): update grpo_sync seq_logprob_error_masking call site#2599ZhiyuLi-Nvidia wants to merge 1 commit into
ZhiyuLi-Nvidia wants to merge 1 commit into
Conversation
PR #2559 changed compute_and_apply_seq_logprob_error_masking to return a dict of 8 keys (previously a 3-tuple of max_seq_mult_prob_error, num_masked_seqs, masked_correct_pct). The two call sites in grpo.py were updated, but the call site in grpo_sync.py was missed, causing L1_Functional_Tests_GPU to fail on main with: File ".../nemo_rl/algorithms/grpo_sync.py", line 772, in grpo_train_sync ValueError: too many values to unpack (expected 3) Aligns grpo_sync.py with the dict-based pattern already in use at grpo.py:1935-1963 and grpo.py:3064-3091, and propagates the richer mean/min and after-mask metrics to the sync recipe. Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
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
1 participant
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.
Summary
mainintroduced when PR #2559 changedcompute_and_apply_seq_logprob_error_maskingto return a dict but missed updating the call site ingrpo_sync.py.mainis currently failing withValueError: too many values to unpack (expected 3)atnemo_rl/algorithms/grpo_sync.py:772— see failing job.grpo_sync.pywith the dict-based pattern already in use atgrpo.py:1935-1963andgrpo.py:3064-3091, so the sync recipe now also surfaces the richer mean / min / after-mask metrics.Test plan
/ok to test <sha>) — should turnL1_Functional_Tests_GPUgreen again.🤖 Generated with Claude Code