Skip to content

feat: Enable SFT and DPO with Megatron backend #525

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 17 commits into from
Jun 20, 2025
Merged

feat: Enable SFT and DPO with Megatron backend #525

merged 17 commits into from
Jun 20, 2025

Conversation

ashors1
Copy link
Contributor

@ashors1 ashors1 commented Jun 17, 2025 •
edited
Loading

What does this PR do ?

Adds Megatron support for SFT and DPO.

  • SFT convergence on SQuAD:
    sft_megatron

  • DPO convergence on HelpSteer-3:
    nemorl_dpo

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

  • ...

ashors1 added 8 commits June 17, 2025 11:33
@ashors1
enable sft and dpo in megatron path
fe91356 
Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1
DPO fixes
46bac4d 
Signed-off-by: Anna Shors <ashors@nvidia.com>
@ashors1
add testing config
afc6891 
Signed-off-by: Anna Shors <ashors@nvidia.com>
@ashors1
fix rebase
8694ae8 
Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1
make sure overlap_param_gather and use_distributed_optimizer are not …
9cc4a51 
…used together

Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1
update config
3dfb616 
Signed-off-by: Anna Shors <ashors@nvidia.com>
@ashors1
fix config
a62ed99 
Signed-off-by: Anna Shors <ashors@nvidia.com>
@ashors1
add unit tests
2a251ca 
Signed-off-by: Anna Shors <ashors@nvidia.com>
@ashors1 ashors1 marked this pull request as ready for review June 17, 2025 22:53
ashors1 added 2 commits June 17, 2025 15:53
@ashors1
remove old comment
8f051ba 
Signed-off-by: Anna Shors <ashors@nvidia.com>
@ashors1
add sft config, minor fixes
cda9eb7 
Signed-off-by: Anna Shors <ashors@nvidia.com>
SahilJain314
SahilJain314 reviewed Jun 17, 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.

minor nits, otherwise lgtm

@ashors1
remove deprecated arg
2b62eb0 
Signed-off-by: ashors1 <ashors@nvidia.com>
SahilJain314
SahilJain314 previously approved these changes Jun 17, 2025
View reviewed changes
@terrykong
Copy link
Contributor

@ashors1 can you add convergence to the description?

parthchadha
parthchadha previously approved these changes Jun 17, 2025
View reviewed changes
@SahilJain314
lint
45491a4 
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
@SahilJain314 SahilJain314 dismissed stale reviews from parthchadha and themself via 45491a4 June 17, 2025 23:26
@SahilJain314
Copy link
Contributor

(lint check failing, ran linter -- please reapprove)

SahilJain314
SahilJain314 previously approved these changes Jun 17, 2025
View reviewed changes
@ashors1
Copy link
Contributor Author

ashors1 commented Jun 17, 2025 •
edited
Loading

@ashors1 can you add convergence to the description?

Yeah, do you mean uploading convergence plots @terrykong?

@ashors1 ashors1 added the CI:L1 Run doctests, unit tests, and functional tests label Jun 17, 2025
@terrykong
Copy link
Contributor

Yea, screenshot is okay; just as a record of dtensor vs. mcore convergence at least on one model

@ashors1
Copy link
Contributor Author

ashors1 commented Jun 17, 2025

Yea, screenshot is okay; just as a record of dtensor vs. mcore convergence at least on one model

Done

@SahilJain314 SahilJain314 enabled auto-merge June 17, 2025 23:49
@SahilJain314 SahilJain314 disabled auto-merge June 17, 2025 23:55
@terrykong terrykong enabled auto-merge June 18, 2025 22:07
@terrykong terrykong added this pull request to the merge queue Jun 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 19, 2025
@ashors1
fix unit test
9d8eebd 
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong
terrykong previously approved these changes Jun 19, 2025
View reviewed changes
@terrykong terrykong added this pull request to the merge queue Jun 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 19, 2025
@ashors1
update test suite
c984d14 
Signed-off-by: ashors1 <ashors@nvidia.com>
@terrykong terrykong enabled auto-merge June 19, 2025 19:01
terrykong
terrykong previously approved these changes Jun 19, 2025
View reviewed changes
@terrykong terrykong added this pull request to the merge queue Jun 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 19, 2025
@ashors1
fix typo
3446cd8 
Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1
fix convergence test suite
af6ba86 
Signed-off-by: Anna Shors <ashors@nvidia.com>
terrykong
terrykong previously approved these changes Jun 19, 2025
View reviewed changes
@terrykong terrykong added this pull request to the merge queue Jun 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 19, 2025
@ashors1
fix dpo config
ba21979 
Signed-off-by: Anna Shors <ashors@nvidia.com>
@terrykong terrykong enabled auto-merge June 20, 2025 00:36
@terrykong terrykong added this pull request to the merge queue Jun 20, 2025
Merged via the queue into main with commit 3d36d0a Jun 20, 2025
13 of 14 checks passed
@terrykong terrykong deleted the ashors/megatron_sft_dpo branch June 20, 2025 02:39
YzjiaoNvd pushed a commit to YzjiaoNvd/NeMo-RL that referenced this pull request Jul 14, 2025
@ashors1 @SahilJain314
feat: Enable SFT and DPO with Megatron backend (NVIDIA-NeMo#525)
810a196 
Signed-off-by: ashors1 <ashors@nvidia.com>
Signed-off-by: Anna Shors <ashors@nvidia.com>
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
Co-authored-by: Sahil Jain <sahilj@nvidia.com>
KiddoZhu pushed a commit that referenced this pull request Jul 28, 2025
@ashors1 @SahilJain314 @KiddoZhu
feat: Enable SFT and DPO with Megatron backend (#525)
375bdd8 
Signed-off-by: ashors1 <ashors@nvidia.com>
Signed-off-by: Anna Shors <ashors@nvidia.com>
Signed-off-by: Sahil Jain <sahilj@nvidia.com>
Co-authored-by: Sahil Jain <sahilj@nvidia.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

@parthchadha parthchadha parthchadha left review comments

@SahilJain314 SahilJain314 SahilJain314 left review comments

Assignees
No one assigned
Labels
CI:docs Run doctest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ashors1 @terrykong @SahilJain314 @parthchadha