Deprecate `ModelFilter/DatasetFilter` #2028

druvdub · 2024-02-15T01:53:35Z

Related to #1676

Moved all attributes from ModelFilter class to list_models arguments and added a filter query builder to build a query. Updated list_models to take both filter as an Iterable and also update the filter list based on other arguments in the method

…method Moved all attributes from ModelFilter class to list_models arguments and added a filter query builder to build a query. Updated list_models to take both filter as an Iterable and also update the filter list based on other arguments in the method

src/huggingface_hub/hf_api.py

Wauplin · 2024-02-15T08:41:52Z

Hi @druvdub! Thanks for the PR ❤️ I've tried to answer all of your questions in #1676 (comment). Please let me know if some parts are still unclear :)

Regarding this PR, would it be possible to fix the styling? The PR change diff is very big at the moment due to some whitespaces/newlines added or removed. This makes the PR quite difficult to review. You can fix this with the following steps:

install the dev dependencies:

pip install -e ".[dev]"

run the make style command (which runs ruff linter and formatter + a few scripts)

make style

(optional) once done, you should be able to run run quality without any error (checks styling + type annotations):

make quality

I believe the current changes were due to a "autoformat" or "format on save" feature in your IDE. Make sure to always run make style before committing changes :) Thanks in advance!

druvdub · 2024-02-15T10:27:25Z

Hi @druvdub! Thanks for the PR ❤️ I've tried to answer all of your questions in #1676 (comment). Please let me know if some parts are still unclear :)

Regarding this PR, would it be possible to fix the styling? The PR change diff is very big at the moment due to some whitespaces/newlines added or removed. This makes the PR quite difficult to review. You can fix this with the following steps:

install the dev dependencies:
pip install -e ".[dev]"
run the make style command (which runs ruff linter and formatter + a few scripts)
make style
(optional) once done, you should be able to run run quality without any error (checks styling + type annotations):
make quality
I believe the current changes were due to a "autoformat" or "format on save" feature in your IDE. Make sure to always run make style before committing changes :) Thanks in advance!

Yup no worries, for some reason the makefile did not work, So I had to now manually run the ruff checks

src/huggingface_hub/hf_api.py

… avoid duplication

src/huggingface_hub/utils/endpoint_helpers.py

src/huggingface_hub/hf_api.py

druvdub · 2024-02-15T14:26:18Z

@Wauplin I think this should be all in terms of functionality changes.

Should I update READMEs and completely remove ModelFilter/DatasetFilter because I believe it might affect the hf_api docs

And finally in terms of test changes should I just update the existing tests to take arguments instead of ModelFilter/DatasetFilter or write some completely new ones

Wauplin · 2024-02-15T16:32:38Z

Thanks for all the modifications and taking care of the special cases @druvdub! I'll have a look at it on tomorrow if that's fine with you :)

druvdub · 2024-02-15T16:54:43Z

Thanks for all the modifications and taking care of the special cases @druvdub! I'll have a look at it on tomorrow if that's fine with you :)

Yea sounds good to me :)

Wauplin

Thanks for the PR @druvdub! Logic-wise it looks good to me! 🔥 I left a couple of comments regarding the code + API but overall very nice one :) Once addressed, we will have to review some tests, especially if ModelFilter / DatasetFilter are used (since deprecation warning make the CI fail, on purpose). I can help on this part if needed (we have a expect_deprecation decorator for tests that are meant to break).

src/huggingface_hub/hf_api.py

src/huggingface_hub/utils/endpoint_helpers.py

src/huggingface_hub/hf_api.py

HuggingFaceDocBuilderDev · 2024-02-16T10:37:54Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

druvdub · 2024-02-16T15:54:14Z

Thanks for the PR @druvdub! Logic-wise it looks good to me! 🔥 I left a couple of comments regarding the code + API but overall very nice one :) Once addressed, we will have to review some tests, especially if ModelFilter / DatasetFilter are used (since deprecation warning make the CI fail, on purpose). I can help on this part if needed (we have a expect_deprecation decorator for tests that are meant to break).

Hey @Wauplin I have addressed all the comments so far, and hopefully I can look at the tests now.

In the meantime, is this expected?

And similarly do we need to update the README.md files as well

UPDATE:
the tests do fail, I can use the decorator but what I am thinking about are the additional scenarios
where we pass the the filter keyword arguments and ModelFilter as well. Do we need them assuming ModelFilter will be removed in sometime

druvdub · 2024-02-16T16:45:05Z

Thanks for the PR @druvdub! Logic-wise it looks good to me! 🔥 I left a couple of comments regarding the code + API but overall very nice one :) Once addressed, we will have to review some tests, especially if ModelFilter / DatasetFilter are used (since deprecation warning make the CI fail, on purpose). I can help on this part if needed (we have a expect_deprecation decorator for tests that are meant to break).

Might need to add another decorator for classes if we plan to use @expect_deprecation or potentially modify the tests to handle warnings with warning.catch_warnings unless we deprecate the filter argument in list_models and list_datasets.

Wauplin

Thanks for iterating over this PR! Implementation looks good to me now :)

In the meantime, is this expected?

Tips might not be formatted correctly if they contains special anchor (like urls). This is a known issue to solve in our parser but a workaround exists. A quick solution is to add newlines between the tag and the text itself (see comments below) and it should fix the documentation page. Thanks for noticing!

And similarly do we need to update the README.md files as well

AFAIK there is no mention of ModelFilter/DatasetFilter in the READMEs. No need to mention them "just" for the deprecation warning.

Might need to add another decorator for classes if we plan to use @expect_deprecation or potentially modify the tests to handle warnings with warning.catch_warnings unless we deprecate the filter argument in list_models and list_datasets.

We can already use @expect_deprecation without needing to add a custom decorator. You just have to update the warning message in the code to add 'quotes'.

what I am thinking about are the additional scenarios
where we pass the the filter keyword arguments and ModelFilter as well. Do we need them assuming ModelFilter will be removed in sometime

No need to test the cases where direct parameters + ModelFilter are passed at the same time. It's counter-intuitive + the user would have a direct deprecation warning when implementing it (+it will only be supported for a few version, as you said).

src/huggingface_hub/utils/endpoint_helpers.py

Fix warning docstring Co-authored-by: Lucain <lucainp@gmail.com>

druvdub · 2024-02-19T22:36:47Z

Sounds good, everything should be done now :)

Wauplin

@expect_deprecation requires an argument (which method/class is deprecated)

tests/test_hf_api.py

codecov · 2024-02-20T09:50:32Z

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (f386b2a) 82.34% compared to head (6fba81a) 82.22%.

Files	Patch %	Lines
src/huggingface_hub/hf_api.py	67.24%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2028      +/-   ##
==========================================
- Coverage   82.34%   82.22%   -0.12%     
==========================================
  Files          66       66              
  Lines        8257     8309      +52     
==========================================
+ Hits         6799     6832      +33     
- Misses       1458     1477      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Wauplin

PR is ready to be merged! Good job @druvdub 🔥 🔥 🔥

druvdub · 2024-02-20T12:13:31Z

woohoo great!!

druvdub marked this pull request as draft February 15, 2024 01:53

druvdub commented Feb 15, 2024

View reviewed changes

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved

Reformat code

8f6df0e

druvdub changed the title ~~Deprecate ModelFilter/DatasetFilter~~ Deprecate ModelFilter/DatasetFilter Feb 15, 2024

Reformat Code

5278995

druvdub commented Feb 15, 2024

View reviewed changes

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved

druvdub added 4 commits February 15, 2024 10:56

Update checks in to be consistent

e39e181

Add build_dataset_filter method and modified list_dataset

d757d8b

Update list_models and list_dataset methods to use set for filters to…

edd7d5a

… avoid duplication

Update warning to use FutureWarning instead of DeprecatedWarning

54210c1

druvdub commented Feb 15, 2024

View reviewed changes

src/huggingface_hub/utils/endpoint_helpers.py Show resolved Hide resolved

druvdub commented Feb 15, 2024

View reviewed changes

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved

druvdub added 3 commits February 15, 2024 14:42

Merge branch 'main' into deprecate-model-filter-and-dateset-filter

f3c239e

Update search helper classes docstrings to follow HF doc-builder syntax

2fbaf9f

Modify warnings to be consistent

1919121

Wauplin reviewed Feb 16, 2024

View reviewed changes

Wauplin marked this pull request as ready for review February 16, 2024 10:30

Merge branch 'main' into deprecate-model-filter-and-dateset-filter

9d06abc

druvdub added 4 commits February 16, 2024 11:28

Move comments to inline to fix mypy issues

18707ed

Move model filter builder to

4804b44

Move dataset filter builder to list_datasets

8d84dad

Address comments and remove stacklevel argument from warnings

56f9e1b

Wauplin approved these changes Feb 19, 2024

View reviewed changes

druvdub and others added 2 commits February 19, 2024 22:20

Apply suggestions from code review

853ee21

Fix warning docstring Co-authored-by: Lucain <lucainp@gmail.com>

Update warnings and add deprecation decorator to tests

2276de3

Wauplin reviewed Feb 20, 2024

View reviewed changes

Apply suggestions from code review

6fba81a

Wauplin approved these changes Feb 20, 2024

View reviewed changes

Wauplin merged commit d01206d into huggingface:main Feb 20, 2024
14 checks passed

Wauplin mentioned this pull request Feb 20, 2024

Deprecate ModelFilter/DatasetFilter and remove GeneralTags/ModelTags/DatasetTags #1676

Closed

julien-c mentioned this pull request Feb 24, 2024

List models tweaks #2044

Merged

Wauplin mentioned this pull request Feb 26, 2024

ModelFilter uses tag rather than pipeline_tag #2009

Closed

druvdub deleted the deprecate-model-filter-and-dateset-filter branch March 1, 2024 09:34

Wauplin mentioned this pull request May 22, 2024

Don't use deprecated DatasetFilter + update deps huggingface/setfit#527

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `ModelFilter/DatasetFilter` #2028

Deprecate `ModelFilter/DatasetFilter` #2028

druvdub commented Feb 15, 2024

Wauplin commented Feb 15, 2024

druvdub commented Feb 15, 2024 •

edited

druvdub commented Feb 15, 2024 •

edited

Wauplin commented Feb 15, 2024

druvdub commented Feb 15, 2024

Wauplin left a comment •

edited

HuggingFaceDocBuilderDev commented Feb 16, 2024

druvdub commented Feb 16, 2024 •

edited

druvdub commented Feb 16, 2024 •

edited

Wauplin left a comment

druvdub commented Feb 19, 2024

Wauplin left a comment

codecov bot commented Feb 20, 2024

Wauplin left a comment

druvdub commented Feb 20, 2024

Deprecate ModelFilter/DatasetFilter #2028

Deprecate ModelFilter/DatasetFilter #2028

Conversation

druvdub commented Feb 15, 2024

Wauplin commented Feb 15, 2024

druvdub commented Feb 15, 2024 • edited

druvdub commented Feb 15, 2024 • edited

Wauplin commented Feb 15, 2024

druvdub commented Feb 15, 2024

Wauplin left a comment • edited

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Feb 16, 2024

druvdub commented Feb 16, 2024 • edited

druvdub commented Feb 16, 2024 • edited

Wauplin left a comment

Choose a reason for hiding this comment

druvdub commented Feb 19, 2024

Wauplin left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 20, 2024

Codecov Report

Wauplin left a comment

Choose a reason for hiding this comment

druvdub commented Feb 20, 2024

Deprecate `ModelFilter/DatasetFilter` #2028

Deprecate `ModelFilter/DatasetFilter` #2028

druvdub commented Feb 15, 2024 •

edited

druvdub commented Feb 15, 2024 •

edited

Wauplin left a comment •

edited

druvdub commented Feb 16, 2024 •

edited

druvdub commented Feb 16, 2024 •

edited