Skip to content

feat: dual-clip in grpo loss #311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
May 9, 2025
Merged

feat: dual-clip in grpo loss #311

merged 11 commits into from
May 9, 2025

Conversation

ZhiyuLi-Nvidia
Copy link
Contributor

@ZhiyuLi-Nvidia ZhiyuLi-Nvidia commented May 3, 2025 •
edited
Loading

What does this PR do ?

Add dual-clip support in grpo loss

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

  • ...

SahilJain314
SahilJain314 requested changes May 3, 2025
View reviewed changes
Copy link
Contributor

@SahilJain314 SahilJain314 left a comment

Choose a reason for hiding this comment

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

small description correctness and grammar changes

@ZhiyuLi-Nvidia ZhiyuLi-Nvidia requested a review from parthchadha May 4, 2025 17:48
SahilJain314
SahilJain314 previously approved these changes May 5, 2025
View reviewed changes
parthchadha
parthchadha reviewed May 5, 2025
View reviewed changes
SahilJain314
SahilJain314 reviewed May 5, 2025
View reviewed changes
@github-actions github-actions bot added the documentation Improvements or additions to documentation label May 5, 2025
@ZhiyuLi-Nvidia ZhiyuLi-Nvidia force-pushed the zhiyul/duel_clip branch 2 times, most recently from d362eb1 to e2a1448 Compare May 5, 2025 20:53
SahilJain314
SahilJain314 previously approved these changes May 5, 2025
View reviewed changes
@terrykong terrykong linked an issue May 6, 2025 that may be closed by this pull request
Enable PPO Dual-Clipping #307
Closed
ZhiyuLi-Nvidia and others added 6 commits May 6, 2025 18:18
@ZhiyuLi-Nvidia
feat: dual-clip in grpo loss
93d0bc0 
Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
@ZhiyuLi-Nvidia
better comment
5a07313 
Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
@SahilJain314 @ZhiyuLi-Nvidia
Apply suggestions from code review
4c53e2a 
Signed-off-by: Sahil Jain <48468750+SahilJain314@users.noreply.github.com>
@ZhiyuLi-Nvidia @parthchadha
Update nemo_rl/algorithms/loss_functions.py
bbae1e9 
Co-authored-by: Parth Chadha <pchadha@nvidia.com>
Signed-off-by: ZhiyuLi-Nvidia <zhiyul@NVIDIA.com>
@SahilJain314 @ZhiyuLi-Nvidia
Update nemo_rl/algorithms/loss_functions.py
9270595 
Signed-off-by: Sahil Jain <48468750+SahilJain314@users.noreply.github.com>
@ZhiyuLi-Nvidia
add in grpo.md
482d058 
Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
@ZhiyuLi-Nvidia
Copy link
Contributor Author

@parthchadha @terrykong do you have any other comments?

terrykong
terrykong reviewed May 7, 2025
View reviewed changes
@ZhiyuLi-Nvidia
address comments
0286b3a 
Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
SahilJain314
SahilJain314 reviewed May 7, 2025
View reviewed changes
SahilJain314 and others added 2 commits May 7, 2025 13:06
@SahilJain314
Update nemo_rl/algorithms/loss_functions.py
88cf04b 
Signed-off-by: Sahil Jain <48468750+SahilJain314@users.noreply.github.com>
@ZhiyuLi-Nvidia
fix lint
9bdb5e5 
Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
parthchadha
parthchadha previously approved these changes May 8, 2025
View reviewed changes
@parthchadha parthchadha added this pull request to the merge queue May 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 9, 2025
@terrykong
fix that test
074f661 
Signed-off-by: Terry Kong <terryk@nvidia.com>
terrykong
terrykong previously approved these changes May 9, 2025
View reviewed changes
@terrykong terrykong enabled auto-merge May 9, 2025 03:35
@terrykong terrykong added this pull request to the merge queue May 9, 2025
@ZhiyuLi-Nvidia
fix tests
706986f 
Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
@terrykong terrykong removed this pull request from the merge queue due to a manual request May 9, 2025
@terrykong terrykong enabled auto-merge May 9, 2025 03:52
@terrykong terrykong added this pull request to the merge queue May 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 9, 2025
@terrykong terrykong added this pull request to the merge queue May 9, 2025
Merged via the queue into main with commit bc8cb65 May 9, 2025
13 checks passed
@terrykong terrykong deleted the zhiyul/duel_clip branch May 9, 2025 11:06
YzjiaoNvd pushed a commit to YzjiaoNvd/NeMo-RL that referenced this pull request Jun 10, 2025
@ZhiyuLi-Nvidia @SahilJain314
@parthchadha @terrykong
feat: dual-clip in grpo loss (NVIDIA-NeMo#311)
e69085e 
Signed-off-by: Zhiyu Li <zhiyul@nvidia.com>
Signed-off-by: Sahil Jain <48468750+SahilJain314@users.noreply.github.com>
Signed-off-by: ZhiyuLi-Nvidia <zhiyul@NVIDIA.com>
Signed-off-by: Terry Kong <terryk@nvidia.com>
Co-authored-by: Sahil Jain <48468750+SahilJain314@users.noreply.github.com>
Co-authored-by: Parth Chadha <pchadha@nvidia.com>
Co-authored-by: Terry Kong <terryk@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SahilJain314 SahilJain314 SahilJain314 left review comments

@terrykong terrykong terrykong approved these changes

@parthchadha parthchadha parthchadha left review comments

@gshennvm gshennvm Awaiting requested review from gshennvm

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable PPO Dual-Clipping
4 participants
@ZhiyuLi-Nvidia @terrykong @parthchadha @SahilJain314