Skip to content

fix: use prompt token length for advantage group extraction and fix token mask#2176

Merged
yuki-97 merged 3 commits into
mainfrom
yifu/fix-prompt-extraction-multi-turn
May 27, 2026
Merged

fix: use prompt token length for advantage group extraction and fix token mask#2176
yuki-97 merged 3 commits into
mainfrom
yifu/fix-prompt-extraction-multi-turn

Conversation

@yfw

@yfw yfw commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

This PR fixes two multi-turn GRPO training issues:

  1. The previous role-based extraction (_extract_prompt_only_messages) broke on multi-turn prompts containing assistant messages in the conversation history — it would strip them, corrupting the prompt IDs used for advantage estimation.
    Replace with extract_initial_prompt_messages() which uses the length field to identify the original prompt boundary. Applied to both sync and async GRPO paths.

  2. GRPO token loss masks previously unmasked every message with role == "assistant". In multi-turn data, assistant messages can be part of the prompt history, not generated rollout output, so those tokens should not contribute to the policy loss. This PR updates masking so only assistant messages produced by generation, identified by existing generation_logprobs, are trainable. Missing generation_logprobs are still filled with zeros for downstream tensorization.

Closes #1960 and #1956

What does this PR do ?

Add a one line overview of what this PR aims to accomplish.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

@yfw
yfw requested a review from a team as a code owner March 30, 2026 23:17
@yfw yfw added the super-v3 label Mar 30, 2026
@yfw
yfw requested a review from a team as a code owner March 30, 2026 23:17
@copy-pr-bot

copy-pr-bot Bot commented Mar 30, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

yuki-97
yuki-97 previously approved these changes Mar 31, 2026
@yuki-97 yuki-97 added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Mar 31, 2026
@yuki-97

yuki-97 commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

/ok to test 628a248

@macandro96
macandro96 force-pushed the yifu/fix-prompt-extraction-multi-turn branch from 628a248 to 7d96ad3 Compare May 21, 2026 23:07
@macandro96
macandro96 requested a review from yuki-97 May 21, 2026 23:18
@macandro96
macandro96 force-pushed the yifu/fix-prompt-extraction-multi-turn branch from 7d96ad3 to d5961e5 Compare May 21, 2026 23:20
@macandro96

Copy link
Copy Markdown
Contributor

/ok to test 7d96ad3

@copy-pr-bot

copy-pr-bot Bot commented May 21, 2026

Copy link
Copy Markdown

/ok to test 7d96ad3

@macandro96, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

@macandro96

Copy link
Copy Markdown
Contributor

/ok to test d5961e5

@macandro96 macandro96 changed the title fix: use prompt token length for advantage group extraction fix: use prompt token length for advantage group extraction and fix token mask May 21, 2026
@macandro96
macandro96 force-pushed the yifu/fix-prompt-extraction-multi-turn branch from d5961e5 to 20adf67 Compare May 22, 2026 14:30
Comment thread nemo_rl/algorithms/grpo.py
Comment thread nemo_rl/algorithms/grpo.py Outdated
Comment thread nemo_rl/algorithms/grpo.py Outdated
@yuki-97

yuki-97 commented May 22, 2026

Copy link
Copy Markdown
Contributor

@yfw @HeyyyyyyG could you help to take a review as well?

@yuki-97
yuki-97 requested a review from HeyyyyyyG May 22, 2026 14:54
@macandro96
macandro96 force-pushed the yifu/fix-prompt-extraction-multi-turn branch from 20adf67 to 49601f1 Compare May 22, 2026 19:26
@HeyyyyyyG

Copy link
Copy Markdown
Contributor

LGTM

@yuki-97

yuki-97 commented May 25, 2026

Copy link
Copy Markdown
Contributor

/ok to test 49601f1

@macandro96

Copy link
Copy Markdown
Contributor

/ok to test 18a05b0

yfw and others added 3 commits May 26, 2026 14:49
The previous role-based extraction (`_extract_prompt_only_messages`)
broke on multi-turn prompts containing assistant messages in the
conversation history — it would strip them, corrupting the prompt IDs
used for advantage estimation.

Replace with `extract_initial_prompt_messages()` which uses the
`length` field to identify the original prompt boundary. Applied to
both sync and async GRPO paths.

Closes #1960

Co-Authored-By: Jiaqi Zeng <jiaqiz@nvidia.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
Signed-off-by: Anish Mahishi <amahishi@cw-dfw-cs-001-vscode-02.cm.cluster>
Signed-off-by: Anish Mahishi <amahishi@cw-dfw-cs-001-vscode-02.cm.cluster>
@macandro96
macandro96 force-pushed the yifu/fix-prompt-extraction-multi-turn branch from d01bc6c to 8e1c343 Compare May 26, 2026 18:49
@macandro96

Copy link
Copy Markdown
Contributor

/ok to test 8e1c343

@macandro96

Copy link
Copy Markdown
Contributor

@yuki-97 fixed! can you reapprove please?

@yuki-97 yuki-97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @macandro96

@ZhiyuLi-Nvidia could you help to take a check at TQ path?
as I know we'll always have generation_logprobs in rollout, so I think it should be fine to remove setdefault generation_logprobs in TQ path, but better to double check.

@yuki-97
yuki-97 requested a review from ZhiyuLi-Nvidia May 27, 2026 00:48
@ZhiyuLi-Nvidia

ZhiyuLi-Nvidia commented May 27, 2026

Copy link
Copy Markdown
Contributor

@ZhiyuLi-Nvidia could you help to take a check at TQ path?
as I know we'll always have generation_logprobs in rollout, so I think it should be fine to remove setdefault generation_logprobs in TQ path, but better to double check.

Thank you @yuki-97 @macandro96 ! LGTM and thank you for the change and review for data plane coverage as well.

Out of scope for this PR, but I have some concerns about the message_logs: list data type, it creates friction for both data transfer and storage with additional preprocessing/postprocessing overhead.

  • The type is quite loose and too general to allow proper memory budgeting up front. It's unclear whether it's nested lists of objects or something else.
  • For future GPU-based RDMA communication, a list of tensors likely implies padding overhead. With the loose type, I am not even sure how to properly handle it.

I think we should eventually move to nested jagged tensors: a single concatenated tensor paired with a sequence-length tensor to mark boundaries. I double-checked, and veRL uses nested jagged tensors as well. Curious if you agree with this direction.

@yuki-97
yuki-97 merged commit c3bb1d7 into main May 27, 2026
37 checks passed
@yuki-97
yuki-97 deleted the yifu/fix-prompt-extraction-multi-turn branch May 27, 2026 02:27
@yuki-97

yuki-97 commented May 27, 2026

Copy link
Copy Markdown
Contributor

thanks @ZhiyuLi-Nvidia @macandro96 , I've merged but feel free to continue discussing here or in slack.

besides, message_logs: list has both tensor and string in it now, I feel we need to see how to structure them if we eventually move to nested jagged tensors.

yfw added a commit that referenced this pull request May 27, 2026
…oken mask (#2176)

Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
Signed-off-by: Anish Mahishi <amahishi@cw-dfw-cs-001-vscode-02.cm.cluster>
Co-authored-by: Jiaqi Zeng <jiaqiz@nvidia.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Anish Mahishi <amahishi@cw-dfw-cs-001-vscode-02.cm.cluster>
vigneshwaran pushed a commit to vigneshwaran/RL that referenced this pull request May 28, 2026
…oken mask (NVIDIA-NeMo#2176)

Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
Signed-off-by: Anish Mahishi <amahishi@cw-dfw-cs-001-vscode-02.cm.cluster>
Co-authored-by: Jiaqi Zeng <jiaqiz@nvidia.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Anish Mahishi <amahishi@cw-dfw-cs-001-vscode-02.cm.cluster>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) super-v3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[super-pr] Use prompt length to find groups for advantage calculation

5 participants