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

Add fineweb classifier #168

Merged
merged 12 commits into from
Aug 21, 2024
Merged

Add fineweb classifier #168

merged 12 commits into from
Aug 21, 2024

Conversation

VibhuJawa
Copy link
Collaborator

@VibhuJawa VibhuJawa commented Jul 24, 2024 •
edited
Loading

Description

This PR adds fineweb clasifier.

Usage

python3 fineweb_edu_classifier_example.py --input-data-dir input_data --output-data-dir "test_classifier"

Checklist

  • I am familiar with the Contributing Guide.
  • New or Existing tests cover these changes.
  • The documentation is up to date with these changes.

VibhuJawa added 2 commits July 23, 2024 22:07
@VibhuJawa
Add fineweb classifier
627e883 
Signed-off-by: Vibhu Jawa <[email protected]>
@VibhuJawa
Fix data-quality
afa8e55 
Signed-off-by: Vibhu Jawa <[email protected]>
@VibhuJawa
Copy link
Collaborator Author

Merge after we switch this repo to 0.0.3 https://github.com/rapidsai/crossfit/releases/tag/0.0.3 , PIP process is still ongoing.

@VibhuJawa VibhuJawa marked this pull request as ready for review July 25, 2024 03:24
@VibhuJawa VibhuJawa requested a review from sarahyurick July 25, 2024 03:25
sarahyurick
sarahyurick reviewed Jul 25, 2024
View reviewed changes
Copy link
Collaborator

@sarahyurick sarahyurick left a comment

Choose a reason for hiding this comment

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

Runs fine for me. I was just wondering why we are adding this to Curator? Also, will this be added to the docs somewhere?

@VibhuJawa
Copy link
Collaborator Author

Runs fine for me. I was just wondering why we are adding this to Curator? Also, will this be added to the docs somewhere?

@sarahyurick . The scope is to provide an example of using a model which we dont release but maintain

@VibhuJawa VibhuJawa requested a review from ryantwolf July 29, 2024 20:22
ryantwolf
ryantwolf requested changes Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@ryantwolf ryantwolf left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks for adding this. Just some minor nits and potential reformatting.

@sarahyurick sarahyurick mentioned this pull request Aug 5, 2024
Merged
3 tasks
sarahyurick
sarahyurick previously requested changes Aug 7, 2024
View reviewed changes
ryantwolf
ryantwolf requested changes Aug 16, 2024
View reviewed changes
Copy link
Collaborator

@ryantwolf ryantwolf left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a few minor concerns.

@VibhuJawa
Address Reviews
e889f51 
Signed-off-by: Vibhu Jawa <[email protected]>
@VibhuJawa
Copy link
Collaborator Author

@ryantwolf , Thanks for the careful review. Made relevant changes and followed up with questions. Let me know what else to change.

ryantwolf
ryantwolf reviewed Aug 20, 2024
View reviewed changes
Copy link
Collaborator

@ryantwolf ryantwolf left a comment

Choose a reason for hiding this comment

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

Ok looks good. Just need to figure out the AEGIS memory thing.

@VibhuJawa
Fix aegis model
18d93da 
Signed-off-by: Vibhu Jawa <[email protected]>
@VibhuJawa
Copy link
Collaborator Author

Ok looks good. Just need to figure out the AEGIS memory changes

@ryantwolf , Pushed the memory changes 18d93da

Please take a another look

@VibhuJawa VibhuJawa mentioned this pull request Aug 20, 2024
Open
ryantwolf
ryantwolf approved these changes Aug 21, 2024
View reviewed changes
Copy link
Collaborator

@ryantwolf ryantwolf left a comment

Choose a reason for hiding this comment

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

Good on my end. Thank you!

@VibhuJawa VibhuJawa requested a review from sarahyurick August 21, 2024 16:58
@VibhuJawa VibhuJawa dismissed sarahyurick’s stale review August 21, 2024 19:30

Have addressed the review, dismissing to merge this PR in, thanks again Sarah for the review

@VibhuJawa VibhuJawa merged commit 9a424c7 into NVIDIA:main Aug 21, 2024
3 checks passed
@sarahyurick sarahyurick mentioned this pull request Aug 26, 2024
Merged
sarahyurick
sarahyurick reviewed Aug 26, 2024
View reviewed changes
Copy link
Collaborator

@sarahyurick sarahyurick left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Found a minor bug and opened a PR for it.

examples/classifiers/fineweb_edu_example.py
Comment on lines +60 to +64
return argumentHelper.parser.parse_args()


if __name__ == "__main__":
main(attach_args().parse_args())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like there is an unnecessary parse_args() in attach_args() and main(), which is also in our other classifier examples too. I opened #217 to resolve this.

yyu22 pushed a commit to yyu22/NeMo-Curator that referenced this pull request Oct 9, 2024
@VibhuJawa @yyu22
Add fineweb classifier (NVIDIA#168)
020954c 
* Add fineweb classifier

Signed-off-by: Vibhu Jawa <[email protected]>

* Fix data-quality

Signed-off-by: Vibhu Jawa <[email protected]>

* Move fineweb to the mainline classifiers and some code cleanup

Signed-off-by: Vibhu Jawa <[email protected]>

* Add fineweb examples/scripts

Signed-off-by: Vibhu Jawa <[email protected]>

* Remove changes to Aegis classifier

Signed-off-by: Vibhu Jawa <[email protected]>

* Address Reviews

Signed-off-by: Vibhu Jawa <[email protected]>

* Fix aegis model

Signed-off-by: Vibhu Jawa <[email protected]>

* Fix minor bug in docstring

Signed-off-by: Vibhu Jawa <[email protected]>

* Fix minor bug in max_mem_gb_classifier scripts.py

Signed-off-by: Vibhu Jawa <[email protected]>

* Pass in max_mem_gb_classifier scripts/classifiers/aegis_classifier_inference.py

Signed-off-by: Vibhu Jawa <[email protected]>

---------

Signed-off-by: Vibhu Jawa <[email protected]>
Signed-off-by: Yang Yu <[email protected]>
yyu22 pushed a commit to yyu22/NeMo-Curator that referenced this pull request Oct 10, 2024
@VibhuJawa @yyu22
Add fineweb classifier (NVIDIA#168)
6510a7f 
* Add fineweb classifier

Signed-off-by: Vibhu Jawa <[email protected]>

* Fix data-quality

Signed-off-by: Vibhu Jawa <[email protected]>

* Move fineweb to the mainline classifiers and some code cleanup

Signed-off-by: Vibhu Jawa <[email protected]>

* Add fineweb examples/scripts

Signed-off-by: Vibhu Jawa <[email protected]>

* Remove changes to Aegis classifier

Signed-off-by: Vibhu Jawa <[email protected]>

* Address Reviews

Signed-off-by: Vibhu Jawa <[email protected]>

* Fix aegis model

Signed-off-by: Vibhu Jawa <[email protected]>

* Fix minor bug in docstring

Signed-off-by: Vibhu Jawa <[email protected]>

* Fix minor bug in max_mem_gb_classifier scripts.py

Signed-off-by: Vibhu Jawa <[email protected]>

* Pass in max_mem_gb_classifier scripts/classifiers/aegis_classifier_inference.py

Signed-off-by: Vibhu Jawa <[email protected]>

---------

Signed-off-by: Vibhu Jawa <[email protected]>
@sarahyurick sarahyurick mentioned this pull request Dec 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarahyurick sarahyurick sarahyurick left review comments

@arhamm1 arhamm1 arhamm1 left review comments

@ryantwolf ryantwolf ryantwolf approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@VibhuJawa @ryantwolf @sarahyurick @arhamm1