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

Enable ADD ID to work with CPU/GPU both #479

Merged
merged 7 commits into from
Feb 6, 2025
Merged

Enable ADD ID to work with CPU/GPU both #479

merged 7 commits into from
Feb 6, 2025

Conversation

VibhuJawa
Copy link
Collaborator

This PR fixes: #473

@VibhuJawa VibhuJawa requested a review from ayushdg January 10, 2025 18:28
@VibhuJawa VibhuJawa added the gpuci Run GPU CI/CD on PR label Jan 10, 2025
@VibhuJawa VibhuJawa added gpuci Run GPU CI/CD on PR and removed gpuci Run GPU CI/CD on PR labels Jan 10, 2025
@VibhuJawa VibhuJawa added gpuci Run GPU CI/CD on PR and removed gpuci Run GPU CI/CD on PR labels Jan 13, 2025
@sarahyurick sarahyurick mentioned this pull request Jan 22, 2025
Open
@sarahyurick
Copy link
Collaborator

Do you know if this could help fix #142?

@VibhuJawa
Copy link
Collaborator Author

Running into OOM with add id #142

I think if we just do the start_index=None it should be scalable as we pass it to _add_id_fast

@VibhuJawa VibhuJawa requested a review from sarahyurick February 5, 2025 19:47
sarahyurick
sarahyurick reviewed Feb 6, 2025
View reviewed changes
@sarahyurick sarahyurick added gpuci Run GPU CI/CD on PR and removed gpuci Run GPU CI/CD on PR labels Feb 6, 2025
ryantwolf
ryantwolf approved these changes Feb 6, 2025
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.

Wow, didn't realize it all worked OOTB. Good find, glad to have it be faster.

@VibhuJawa VibhuJawa merged commit 1dab545 into NVIDIA:main Feb 6, 2025
6 checks passed
philm001 pushed a commit to philm001/NeMo-Curator that referenced this pull request Feb 10, 2025
@VibhuJawa @philm001
Enable ADD ID to work with CPU/GPU both (NVIDIA#479)
d31a544 
* Enable ADD ID to work with CPU/GPU both

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

* Make Test runable in a CPU only environment

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

* Fix pytest skipping behavior in CPU/GPU environment

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

* Raise error instead of skipping test

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

---------

Signed-off-by: Vibhu Jawa <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>
philm001 pushed a commit to philm001/NeMo-Curator that referenced this pull request Feb 10, 2025
@VibhuJawa @philm001
Enable ADD ID to work with CPU/GPU both (NVIDIA#479)
7604c95 
* Enable ADD ID to work with CPU/GPU both

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

* Make Test runable in a CPU only environment

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

* Fix pytest skipping behavior in CPU/GPU environment

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

* Raise error instead of skipping test

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

---------

Signed-off-by: Vibhu Jawa <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>
ryantwolf added a commit that referenced this pull request Feb 11, 2025
@philm001 @ko3n1g
@github-actions @ryantwolf @VibhuJawa @sarahyurick @lbliii @praateekmahajan
Updated documentation to include packaging requirements (#526)
fdf7c6d 
* ci: Pin twine in release workflow (#512)

* ci: Pin twine in release workflow

Signed-off-by: oliver könig <[email protected]>

* maybe fix?

Signed-off-by: oliver könig <[email protected]>

* fix

Signed-off-by: oliver könig <[email protected]>

---------

Signed-off-by: oliver könig <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>

* ci: Version bump to 0.7.0rc1.dev0 (#513)

Signed-off-by: oliver könig <[email protected]>
Co-authored-by: oliver könig <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>

* Enforce Dataframe Backend Checks (#514)

* Add module and to backend

Signed-off-by: Ryan Wolf <[email protected]>

* Add backend tests

Signed-off-by: Ryan Wolf <[email protected]>

* Fix tests

Signed-off-by: Ryan Wolf <[email protected]>

* Add switch backend tests

Signed-off-by: Ryan Wolf <[email protected]>

* Update modules to use module interface

Signed-off-by: Ryan Wolf <[email protected]>

* Directly invoke module init

Signed-off-by: Ryan Wolf <[email protected]>

* Fix call method

Signed-off-by: Ryan Wolf <[email protected]>

* Fix shuffle call method

Signed-off-by: Ryan Wolf <[email protected]>

* Add docs and more tests

Signed-off-by: Ryan Wolf <[email protected]>

* Fix list formatting in docs

Signed-off-by: Ryan Wolf <[email protected]>

* Address Sarah and Praateek's reviews

Signed-off-by: Ryan Wolf <[email protected]>

* Fix modifier get_backend to backend

Signed-off-by: Ryan Wolf <[email protected]>

* Address Ayush's review

Signed-off-by: Ryan Wolf <[email protected]>

---------

Signed-off-by: Ryan Wolf <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>

* Updated documentation to include packaging requirements

Signed-off-by: Phillip Mobley <[email protected]>

* Fixed formatting issues. Signed-off-by: Phillip Mobley <[email protected]>

Signed-off-by: Phillip Mobley <[email protected]>

* Enable ADD ID to work with CPU/GPU both (#479)

* Enable ADD ID to work with CPU/GPU both

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

* Make Test runable in a CPU only environment

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

* Fix pytest skipping behavior in CPU/GPU environment

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

* Raise error instead of skipping test

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

---------

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

* Add Pooling Strategy Option for embedding creation (#491)

* Add pooling stratedgy

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

* Ensure pytest is importable in a CPU only environment

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

* Fix last token based on Avinash's feedback

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

* Fix indexing issues

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

* Merge in main

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

* Fix Doc-string

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

* Address Sarah's reviews

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

---------

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

* Add Partition On Logic  (#519)

* add partition_on logic

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

* Add Docstring based on Sarah's review

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

* Apply Praateek's suggestion and skip test with using pytest.mark.gpu

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

* Apply Praateek's suggestion and force index=False

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

---------

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

* Add improved cleaning methods from Nemotron-CC (#517)

* Add improved cleaning features

Signed-off-by: Ryan Wolf <[email protected]>

* Fix cleaning tests

Signed-off-by: Ryan Wolf <[email protected]>

* Update documentation and CLI scripts

Signed-off-by: Ryan Wolf <[email protected]>

* Address Sarah and Lawrence's reviews

Signed-off-by: Ryan Wolf <[email protected]>

---------

Signed-off-by: Ryan Wolf <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>

* Update model nomenclature (#497)

* Update model nomenclature

Signed-off-by: Sarah Yurick <[email protected]>

* minor notebook grammar

Signed-off-by: Sarah Yurick <[email protected]>

* add lawrence's suggestion

Signed-off-by: Sarah Yurick <[email protected]>

---------

Signed-off-by: Sarah Yurick <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>

* small add_id backend fix (#525)

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

* benchmark readme updates (#508)

* benchmark readme updates

Signed-off-by: Lawrence Lane <[email protected]>

* benchmark image update

Signed-off-by: Lawrence Lane <[email protected]>

* benchmark text update

Signed-off-by: Lawrence Lane <[email protected]>

---------

Signed-off-by: Lawrence Lane <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>

* Removal logic for fuzzy / exact (no class abstraction) (#509)

Signed-off-by: Phillip Mobley <[email protected]>

* ci: Limit unit-test duration (#534)

Signed-off-by: oliver könig <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>

* Enforce Dataframe Backend Checks (#514)

* Add module and to backend

Signed-off-by: Ryan Wolf <[email protected]>

* Add backend tests

Signed-off-by: Ryan Wolf <[email protected]>

* Fix tests

Signed-off-by: Ryan Wolf <[email protected]>

* Add switch backend tests

Signed-off-by: Ryan Wolf <[email protected]>

* Update modules to use module interface

Signed-off-by: Ryan Wolf <[email protected]>

* Directly invoke module init

Signed-off-by: Ryan Wolf <[email protected]>

* Fix call method

Signed-off-by: Ryan Wolf <[email protected]>

* Fix shuffle call method

Signed-off-by: Ryan Wolf <[email protected]>

* Add docs and more tests

Signed-off-by: Ryan Wolf <[email protected]>

* Fix list formatting in docs

Signed-off-by: Ryan Wolf <[email protected]>

* Address Sarah and Praateek's reviews

Signed-off-by: Ryan Wolf <[email protected]>

* Fix modifier get_backend to backend

Signed-off-by: Ryan Wolf <[email protected]>

* Address Ayush's review

Signed-off-by: Ryan Wolf <[email protected]>

---------

Signed-off-by: Ryan Wolf <[email protected]>

* small add_id backend fix (#525)

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

* Removal logic for fuzzy / exact (no class abstraction) (#509)

Signed-off-by: Phillip Mobley <[email protected]>

---------

Signed-off-by: oliver könig <[email protected]>
Signed-off-by: Phillip Mobley <[email protected]>
Signed-off-by: Ryan Wolf <[email protected]>
Signed-off-by: Vibhu Jawa <[email protected]>
Signed-off-by: Sarah Yurick <[email protected]>
Signed-off-by: Lawrence Lane <[email protected]>
Co-authored-by: oliver könig <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ryan Wolf <[email protected]>
Co-authored-by: Vibhu Jawa <[email protected]>
Co-authored-by: Sarah Yurick <[email protected]>
Co-authored-by: L.B. <[email protected]>
Co-authored-by: Praateek Mahajan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryantwolf ryantwolf ryantwolf approved these changes

@sarahyurick sarahyurick sarahyurick approved these changes

@ayushdg ayushdg Awaiting requested review from ayushdg

@praateekmahajan praateekmahajan Awaiting requested review from praateekmahajan

Assignees
No one assigned
Labels
gpuci Run GPU CI/CD on PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[IMP] Switch to GPU for add_id by default and make that setting configurable
3 participants
@VibhuJawa @sarahyurick @ryantwolf