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.

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

Remove argparse from get_client function signature #12

Merged
merged 13 commits into from
May 22, 2024

Conversation

ryantwolf
Copy link
Collaborator

@ryantwolf ryantwolf commented Mar 20, 2024

Main items:

  • Refactor get_client function to not use argparse
  • Add parse_client_args function to bridge argparse to new get_client
  • Update all get_client invocations
  • Allow get_client to be imported from root module like from nemo_curator import get_client
  • Update documentation
  • Test all scripts

Other changes:

  • Modify how find_exact_duplicates.py determines whether to use CPU or GPU.
  • Fix broken links in user guide.

@Maghoumi
Copy link
Collaborator

@ryantwolf I think we should have this functionality. I was trying to use the Curator through a notebook and had to do some gymnastics to create a client using get_client().

If this is too much work for now, maybe a simple workaround would be to have an equivalent of add_distributed_args(), but return a dictionary of the required arguments. This way the user can pass that dict instance to get_client() and still be able to use the code in a notebook environment.

@ryantwolf ryantwolf marked this pull request as ready for review May 20, 2024 23:05
@ryantwolf ryantwolf requested a review from ayushdg May 20, 2024 23:06
Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Thanks @ryantwolf looks good to me.

@ryantwolf ryantwolf merged commit 5e46cd8 into main May 22, 2024
3 checks passed
@ryantwolf ryantwolf deleted the rywolf/remove-argparse branch September 16, 2024 15:37
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.

3 participants