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

Search by authors and string #531

Merged
merged 14 commits into from
Jan 5, 2022
Merged

Search by authors and string #531

merged 14 commits into from
Jan 5, 2022

Conversation

FrancescoSaverioZuppichini
Copy link
Contributor

@FrancescoSaverioZuppichini FrancescoSaverioZuppichini commented Dec 11, 2021

Why

Searching by authors and search (so just a string) is currently missing, linked to #513

How

I've edited the logic inside hf_api.HfApi.list_models. Pasted below for convenience

  def list_models(....):
     ....
     if filter is not None:
          params.update({"filter": filter})
          params.update({"full": True})
      if author is not None: # <- here
          params.update({"author": author})
      if search is not None: # <- here
          params.update({"search": search})
      .....

[EDIT] After discussing it we also included the same logic in list_datasets

Test it

I've added a few new lines in the tests under HfApiPublicTest, maybe a test with a complex query + authors, search is not a bad idea.

Since I've edited my main branch on the fork this branch has the commits from the main branch that contains the code for
#522. Unfortunately, I am not a git wizard so I've removed that code. I am not sure what's the best way to clean it up while having my other PR open (#522)

Thanks

Francesco

@julien-c
Copy link
Member

The list_models-related changeset of this PR looks good to me, and you could also apply it to list_datasets which has the same API

Co-authored-by: Julien Chaumond <julien@huggingface.co>
Copy link
Contributor

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thank you for this!

I saw your note about some git issues, try accepting my suggestions in the review and that should handle it just fine.

I also have a few suggestions in there

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
tests/test_hf_api.py Outdated Show resolved Hide resolved
tests/test_hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
Co-authored-by: Zachary Mueller <muellerzr@gmail.com>
Co-authored-by: Zachary Mueller <muellerzr@gmail.com>
@FrancescoSaverioZuppichini
Copy link
Contributor Author

The list_models-related changeset of this PR looks good to me, and you could also apply it to list_datasets which has the same API

Changed the signature and the logic of list_datasets to match list_models :) Tests are green on my local machine

Francesco Saverio Zuppichini and others added 2 commits December 26, 2021 11:04
Co-authored-by: Zachary Mueller <muellerzr@gmail.com>
Co-authored-by: Zachary Mueller <muellerzr@gmail.com>
@FrancescoSaverioZuppichini
Copy link
Contributor Author

Thank you for this!

I saw your note about some git issues, try accepting my suggestions in the review and that should handle it just fine.

I also have a few suggestions in there

Hi @muellerzr , I've included your commits, thank you for helping me out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've included the changes

@julien-c
Copy link
Member

LGTM but i'll let @muellerzr and @LysandreJik chime in!

@muellerzr
Copy link
Contributor

@FrancescoSaverioZuppichini looks like you're not passing the code quality tests, can you do the following please? :)

pip install -U black isort flake8

And then from the root of the repository:

make style; make quality

That should make the quality tests pass

tests/test_hf_api.py Show resolved Hide resolved
@muellerzr muellerzr requested a review from LysandreJik January 4, 2022 15:15
Copy link
Member

@LysandreJik LysandreJik 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 to me, thanks @FrancescoSaverioZuppichini!

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