Skip to content

feat: Overlong filtering for GRPO - #724

Merged
terrykong merged 28 commits into
NVIDIA-NeMo:mainfrom
jubick1337:mnovikov/grpo_overlong_filtering
Sep 1, 2025
Merged

feat: Overlong filtering for GRPO#724
terrykong merged 28 commits into
NVIDIA-NeMo:mainfrom
jubick1337:mnovikov/grpo_overlong_filtering

Conversation

@jubick1337

@jubick1337 jubick1337 commented Jul 23, 2025

Copy link
Copy Markdown
Contributor
  • Add overlong_filtering flag to GRPOConfig
  • Set loss_multiplier to 0 for truncated samples when flag is enabled
  • Set token_loss_mask to 0 for assistant tokens in truncated samples
  • Add truncated field to batch in both sync and async rollout functions

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

  • ...

@jubick1337
jubick1337 force-pushed the mnovikov/grpo_overlong_filtering branch from b9f88b7 to 0bdd593 Compare July 23, 2025 12:58
@jubick1337 jubick1337 changed the title [feat]: Overlong filtering for GRPO feat: Overlong filtering for GRPO Jul 23, 2025
@jubick1337
jubick1337 marked this pull request as draft July 23, 2025 13:28
@github-actions github-actions Bot added the Documentation Improvements or additions to documentation label Jul 23, 2025
@jubick1337
jubick1337 marked this pull request as ready for review July 23, 2025 13:35
SahilJain314
SahilJain314 reviewed Jul 28, 2025
View reviewed changes
Comment thread nemo_rl/algorithms/grpo.py Outdated
Comment thread nemo_rl/algorithms/grpo.py Outdated
@jubick1337
jubick1337 force-pushed the mnovikov/grpo_overlong_filtering branch from c6b1563 to bf511aa Compare July 29, 2025 01:25
@jubick1337
Add overlong filtering feature to GRPO
5ea6c65 
- Add overlong_filtering config option to filter out samples that reach max_total_sequence_length
- Add truncated field to rollouts to track which samples were truncated
- Update documentation to explain the feature
- Add overlong_filtering: false to all GRPO config files

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
@jubick1337
jubick1337 force-pushed the mnovikov/grpo_overlong_filtering branch from bf511aa to 5ea6c65 Compare July 29, 2025 01:27
@jubick1337

jubick1337 commented Jul 29, 2025

Copy link
Copy Markdown
Contributor Author

@SahilJain314 thank you for the review!
Updated the code

@jubick1337
jubick1337 requested a review from SahilJain314 July 29, 2025 12:58
@jubick1337
jubick1337 requested a review from terrykong August 1, 2025 01:47
ashors1
ashors1 previously approved these changes Aug 7, 2025
View reviewed changes

@ashors1 ashors1 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.

Thank you for the PR @jubick1337! This looks good to me. @SahilJain314 any other comments from your side?

@jubick1337
Add GRPO overlong filtering to skip loss computation for truncated saâ€Ķ
eaf8766 
â€Ķmples

- Add overlong_filtering flag to GRPOConfig
- Set loss_multiplier to 0 for truncated samples when flag is enabled
- Set token_loss_mask to 0 for assistant tokens in truncated samples
- Add truncated field to batch in both sync and async rollout functions

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
@jubick1337
Add overlong_filtering: false to all base GRPO configs
5a52c72 
- Set overlong_filtering to false by default in all base GRPO configurations
- This ensures backward compatibility as the feature is opt-in
- Configs that inherit from these bases will also inherit this setting
- Users can override by setting overlong_filtering: true when needed

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
@jubick1337
jubick1337 marked this pull request as draft August 12, 2025 17:02
@jubick1337
GRPO: require grpo.overlong_filtering (no defaults); remove token_masâ€Ķ
5359fc2 
â€Ķk special-casing and rely on sample_mask only

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
@jubick1337
Deepscaler: add recipe file and point legacy configs to it via defaulâ€Ķ
c5cd0aa 
â€Ķts; include overlong_filtering: false

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
@jubick1337
Deepscaler: remove legacy configs under examples/configs; use recipesâ€Ķ
9c0a892 
â€Ķ/llm versions (8K includes overlong_filtering: false)

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
@jubick1337
Merge branch 'main' into mnovikov/grpo_overlong_filtering
d2135fc 
Signed-off-by: Matvei Novikov <mattyson.so@gmail.com>
ashors1
ashors1 reviewed Aug 15, 2025
View reviewed changes
Comment thread nemo_rl/algorithms/grpo.py Outdated
@jubick1337
Merge branch 'main' into mnovikov/grpo_overlong_filtering
33a028a
@jubick1337
Remove defensive check for 'truncated' field in overlong filtering
1c5fc6e 
- The 'truncated' field is always added by rollout functions (both sync and async)
- Simplifies the code by removing unnecessary defensive programming
- As discussed in PR review, this field should always be present when using the standard pipeline

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
ashors1
ashors1 previously approved these changes Aug 15, 2025
View reviewed changes
terrykong
terrykong previously approved these changes Aug 20, 2025
View reviewed changes
@terrykong
terrykong added this pull request to the merge queue Aug 20, 2025
github-merge-queue Bot pushed a commit that referenced this pull request Aug 20, 2025
@jubick1337
feat: Overlong filtering for GRPO (#724)
0d8687e 
Signed-off-by: jubick1337 <mattyson.so@gmail.com>
Signed-off-by: Matvei Novikov <mattyson.so@gmail.com>
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 20, 2025
@ashors1

ashors1 commented Aug 20, 2025

Copy link
Copy Markdown
Contributor

Hi @jubick1337, the presubmit CI failed because new multimodal recipes were recently added to main: https://github.com/NVIDIA-NeMo/RL/actions/runs/17089287317/job/48467257968#step:3:7379. Would you be able to help resolve this? Then we can merge

@jubick1337

jubick1337 commented Aug 20, 2025

Copy link
Copy Markdown
Contributor Author

Hi @jubick1337, the presubmit CI failed because new multimodal recipes were recently added to main: https://github.com/NVIDIA-NeMo/RL/actions/runs/17089287317/job/48467257968#step:3:7379. Would you be able to help resolve this? Then we can merge

Certainly, I’ll handle this and let you know once it’s ready.

@jubick1337
Merge branch 'main' into mnovikov/grpo_overlong_filtering
40b071b
@jubick1337
Add overlong_filtering: false to new GRPO configs
ab52c3d 
- Added overlong_filtering: false to 6 standalone GRPO configs that were missing it
- Configs that inherit from base configs already have this setting and don't need it added
- This ensures all GRPO configs have consistent overlong filtering behavior

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
@jubick1337
jubick1337 dismissed stale reviews from terrykong and ashors1 via ab52c3d August 29, 2025 22:19
@terrykong
terrykong enabled auto-merge August 29, 2025 23:49
terrykong
terrykong previously approved these changes Aug 29, 2025
View reviewed changes
@terrykong
terrykong added this pull request to the merge queue Aug 29, 2025
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 30, 2025
@jubick1337
Merge branch 'main' into mnovikov/grpo_overlong_filtering
2d1b5db
@jubick1337
Add overlong_filtering: false to VLM GRPO configs
9c5469b 
- Added overlong_filtering: false to 3 VLM GRPO configs that were missing it
- vlm_grpo_3B.yaml
- vlm_grpo-qwen2.5-vl-3b-instruct-clevr-1n2g-dtensor2tp1.v1.yaml
- vlm_grpo-smolvlm2-2.2b-instruct-clevr-1n2g-dtensor2tp1.v1.yaml
- This completes the addition of overlong_filtering to all GRPO configs

Signed-off-by: jubick1337 <mattyson.so@gmail.com>
@jubick1337

jubick1337 commented Sep 1, 2025

Copy link
Copy Markdown
Contributor Author

@terrykong I updated vlm configs, should be good now

@terrykong
terrykong enabled auto-merge September 1, 2025 15:41
@terrykong
terrykong added this pull request to the merge queue Sep 1, 2025
Merged via the queue into NVIDIA-NeMo:main with commit 0358a86 Sep 1, 2025
21 checks passed
@jubick1337
jubick1337 deleted the mnovikov/grpo_overlong_filtering branch September 1, 2025 19:46
guyueh1 pushed a commit to guyueh1/NeMo-RL that referenced this pull request Sep 15, 2025
@jubick1337 @guyueh1
feat: Overlong filtering for GRPO (NVIDIA-NeMo#724)
fc2ed98 
Signed-off-by: jubick1337 <mattyson.so@gmail.com>
Signed-off-by: Matvei Novikov <mattyson.so@gmail.com>
PrinsYin pushed a commit to PrinsYin/RL that referenced this pull request Nov 30, 2025
@jubick1337
feat: Overlong filtering for GRPO (NVIDIA-NeMo#724)
247052d 
Signed-off-by: jubick1337 <mattyson.so@gmail.com>
Signed-off-by: Matvei Novikov <mattyson.so@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terrykong terrykong terrykong approved these changes
@ashors1 ashors1 ashors1 left review comments
@parthchadha parthchadha Awaiting requested review from parthchadha
@SahilJain314 SahilJain314 Awaiting requested review from SahilJain314

Assignees

No one assigned

Labels

community-request Documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jubick1337 @ashors1 @terrykong @SahilJain314