Skip to content

fix: Do not initialize reference model for sft#71

Merged
terrykong merged 12 commits intomainfrom
ashors/sft-remove-ref-model
Apr 1, 2025
Merged

fix: Do not initialize reference model for sft#71
terrykong merged 12 commits intomainfrom
ashors/sft-remove-ref-model

Conversation

@ashors1
Copy link
Copy Markdown
Contributor

@ashors1 ashors1 commented Mar 24, 2025

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

  • ...

Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1 ashors1 changed the title Do not initialize reference model for sft fix: Do not initialize reference model for sft Mar 24, 2025
parthchadha
parthchadha previously approved these changes Mar 24, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
terrykong
terrykong previously approved these changes Mar 26, 2025
Copy link
Copy Markdown
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.

Can you modify SFT tests to use init_reference_model=False?

@ashors1
Copy link
Copy Markdown
Contributor Author

ashors1 commented Mar 27, 2025

Can you modify SFT tests to use init_reference_model=False?

Do you mean adding a new test? The SFT functional test will of course already use init_reference_model=False since we just run the loop in sft.py

Signed-off-by: ashors1 <ashors@nvidia.com>
Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1 ashors1 added Run CICD and removed Run CICD labels Mar 31, 2025
@ashors1 ashors1 added Run CICD and removed Run CICD labels Mar 31, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1 ashors1 added Run CICD and removed Run CICD labels Apr 1, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1 ashors1 added Run CICD and removed Run CICD labels Apr 1, 2025
ashors1 added 2 commits April 1, 2025 08:51
Signed-off-by: Anna Shors <ashors@nvidia.com>
@ashors1 ashors1 removed the Run CICD label Apr 1, 2025
@ashors1 ashors1 added Run CICD and removed Run CICD labels Apr 1, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1 ashors1 added Run CICD and removed Run CICD labels Apr 1, 2025
Comment thread tests/unit/models/policy/test_hf_ray_policy.py Outdated
@ashors1 ashors1 added Run CICD and removed Run CICD labels Apr 1, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
@ashors1 ashors1 force-pushed the ashors/sft-remove-ref-model branch from 522633f to 122f37a Compare April 1, 2025 16:55
@ashors1 ashors1 added Run CICD and removed Run CICD labels Apr 1, 2025
Copy link
Copy Markdown
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.

Tests look good, thanks

@terrykong terrykong enabled auto-merge (squash) April 1, 2025 19:25
@terrykong terrykong merged commit 640297c into main Apr 1, 2025
11 checks passed
@terrykong terrykong deleted the ashors/sft-remove-ref-model branch April 1, 2025 19:44
yfw pushed a commit that referenced this pull request Apr 2, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
Signed-off-by: Anna Shors <ashors@nvidia.com>
Co-authored-by: Terry Kong <terrycurtiskong@gmail.com>
KiddoZhu pushed a commit that referenced this pull request May 6, 2025
Signed-off-by: ashors1 <ashors@nvidia.com>
Signed-off-by: Anna Shors <ashors@nvidia.com>
Co-authored-by: Terry Kong <terrycurtiskong@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants