Skip to content
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

Integrating custom metrics and inference parameters #82

Merged
merged 27 commits into from
Jan 26, 2024
Merged

Integrating custom metrics and inference parameters #82

merged 27 commits into from
Jan 26, 2024

Conversation

Kipok
Copy link
Contributor

@Kipok Kipok commented Jan 16, 2024

What does this PR do ?

Besides adding support for metrics/inference params, this PR has the following 2 small changes:

  1. Making scheduler optional in SFT
  2. Copy-pasting a fix for the scheduler's number of steps into the train_gpt_sft.py
    (let me know if you'd like me to split the PR into multiple independent pieces)

There are a few things that I didn't finish yet, but want to push this out in case there is additional refactoring that needs to be done based on the feedback. Here is what I'm going to do next

  1. Add one of the metrics from NeMo (probably exact string match) to give an example of usage
  2. Add tests
  3. Fix an issue with SFT config not being updated by the time model is created (there is a TODO in the code)
  4. Update the changelog / docs

Changelog

  • Please update the CHANGELOG.md under next version with high level changes in this PR.

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:

Checklist when contributing a new algorithm

  • Does the trainer resume and restore model state all states?
  • Does the trainer support all parallelism techniques(PP, TP, DP)?
  • Does the trainer support max_steps=-1 and validation?
  • Does the trainer only call APIs defined in alignable_interface.py?
  • Does the trainer have proper logging?

Additional Information

  • Related to # (issue)

@Kipok
Copy link
Contributor Author

Kipok commented Jan 18, 2024

The new "include_text" parameter will only work after NVIDIA/NeMo#8198 is merged on the nemo side (and included in the aligner Dockerfile)

odelalleau
odelalleau reviewed Jan 18, 2024
View reviewed changes
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

I did a first quick review -- I'm still missing the bigger picture and how everything fits together, but I'll come back to it once it's closer to completion.

examples/nlp/gpt/train_gpt_sft.py Outdated Show resolved Hide resolved
examples/nlp/gpt/train_gpt_sft.py Outdated Show resolved Hide resolved
nemo_aligner/data/nlp/builders.py Outdated Show resolved Hide resolved
nemo_aligner/data/nlp/builders.py Outdated Show resolved Hide resolved
nemo_aligner/metrics/common.py Outdated Show resolved Hide resolved
nemo_aligner/models/nlp/gpt/gpt_sft_model.py Outdated Show resolved Hide resolved
nemo_aligner/models/nlp/gpt/gpt_sft_model.py Outdated Show resolved Hide resolved
nemo_aligner/models/nlp/gpt/gpt_sft_model.py Outdated Show resolved Hide resolved
nemo_aligner/models/nlp/gpt/megatron_gpt_reward_model.py Outdated Show resolved Hide resolved
nemo_aligner/utils/train_utils.py Outdated Show resolved Hide resolved
odelalleau
odelalleau reviewed Jan 19, 2024
View reviewed changes
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Just a minor cleanup in latest commit

nemo_aligner/models/nlp/gpt/gpt_sft_model.py Outdated Show resolved Hide resolved
@Kipok Kipok force-pushed the igitman/metrics-integration branch from 2ad0c96 to e2bc490 Compare January 24, 2024 23:14
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 24, 2024
@Kipok
Copy link
Contributor Author

Kipok commented Jan 24, 2024

Added a new cherry-picked commit to Dockerfile for this PR which is required for metrics to work: NVIDIA/NeMo@7d3d9ac

@Kipok Kipok force-pushed the igitman/metrics-integration branch from d98591c to 77e457a Compare January 25, 2024 00:25
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Jan 25, 2024
gshennvm
gshennvm reviewed Jan 25, 2024
View reviewed changes
Dockerfile Show resolved Hide resolved
@gshennvm
Copy link
Collaborator

your MR LGTM, I'm running the SFT nemo-ci to make sure this didn't break anything

gshennvm
gshennvm requested changes Jan 25, 2024
View reviewed changes
nemo_aligner/metrics/__init__.py Outdated Show resolved Hide resolved
@Kipok Kipok force-pushed the igitman/metrics-integration branch from c70227a to 1f9107d Compare January 25, 2024 23:51
odelalleau
odelalleau reviewed Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for cleaning things up and adding this new feature.

I believe it's just missing an entry in CHANGELOG to mention this addition.

(side note, regarding the question "should Aligner hold metrics?" => I think so, yes, as long as we believe they may be useful to users in general -- fancy custom metrics specific to a single use case should stay in users' own codebases)

@Kipok Kipok force-pushed the igitman/metrics-integration branch from 398ad45 to f1aec3e Compare January 26, 2024 19:49
@Kipok
Copy link
Contributor Author

Kipok commented Jan 26, 2024

@odelalleau thanks for the reminder! I updated the changelog, but please let me know if any more details are needed there

gshennvm
gshennvm previously approved these changes Jan 26, 2024
View reviewed changes
@gshennvm
Copy link
Collaborator

CI passed on this branch, you can merge once @odelalleau resolves the conversation

odelalleau
odelalleau requested changes Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Would just like to suggest a clarification to the changelog

CHANGELOG.md Outdated Show resolved Hide resolved
gshennvm and others added 26 commits January 26, 2024 13:43
@gshennvm @Kipok
add sft with generate
e214943 
Signed-off-by: Gerald Shen <geshen@nvidia.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@gshennvm @Kipok
fix small bug
ca19bec 
Signed-off-by: Gerald Shen <geshen@nvidia.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@gshennvm @Kipok
add metrics
92256fd 
Signed-off-by: Gerald Shen <geshen@nvidia.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Make scheduler optional
23c5446 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Pass in inference params to generate
d3801ba 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Working version with a hardcoded metric
73dd3e3 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Clean up metrics logic and pass from config
2952d7b 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Fix issues + more cleanups
a2f49b7 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Add model arg to strategy
62116c9 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
More fixes
362a5d0 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Add notes for implementation
db444f9 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Running precommit
0f303ba 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Add exact string match metric and fix bugs
60babe8 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Remove unused parameters and add include_text to val dl
e18e0bb 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Address comments
ccd8aea 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@pre-commit-ci @Kipok
[pre-commit.ci] auto fixes from pre-commit.com hooks
7df2a7f 
for more information, see https://pre-commit.ci

Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Precommit
6428c72 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Remove dummy DL + handle checkpoint granularity
c4d806a 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok @odelalleau
Update nemo_aligner/models/nlp/gpt/gpt_sft_model.py
69fa54b 
Co-authored-by: Olivier Delalleau <507137+odelalleau@users.noreply.github.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Addess PR comments
169f47c 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Cleanup
68d5f30 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Precommit
50324d4 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Adding a required cherry-picked commit from nemo
c2a0ab3 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok @gshennvm
Update nemo_aligner/metrics/__init__.py
e137fda 
Signed-off-by: Igor Gitman <igitman@nvidia.com>

Co-authored-by: Gerald Shen <119401249+gshennvm@users.noreply.github.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Update changelog
7fc6381 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok
Rephrase the changelog
27ca524 
Signed-off-by: Igor Gitman <igitman@nvidia.com>
@Kipok Kipok force-pushed the igitman/metrics-integration branch from 37bbd18 to 27ca524 Compare January 26, 2024 21:44
@Kipok
Copy link
Contributor Author

Kipok commented Jan 26, 2024

I don't have write access, @odelalleau or @gshennvm can one of you please help to merge?

@odelalleau odelalleau merged commit d06b23f into NVIDIA:main Jan 26, 2024
3 checks passed
rohitrango pushed a commit to rohitrango/NeMo-Aligner that referenced this pull request Jun 25, 2024
@Kipok @gshennvm
@pre-commit-ci @odelalleau
Integrating custom metrics and inference parameters (NVIDIA#82)
3cea7af 
Signed-off-by: Gerald Shen <geshen@nvidia.com>
Signed-off-by: Igor Gitman <igitman@nvidia.com>
Co-authored-by: Gerald Shen <geshen@nvidia.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Olivier Delalleau <507137+odelalleau@users.noreply.github.com>
Co-authored-by: Gerald Shen <119401249+gshennvm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trias702 trias702 trias702 left review comments

@odelalleau odelalleau odelalleau approved these changes

@gshennvm gshennvm gshennvm left review comments

Assignees
No one assigned
Labels
Algorithms Utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Kipok @gshennvm @odelalleau @trias702