-
Notifications
You must be signed in to change notification settings - Fork 531
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
Deprecate output in list_models #1143
Conversation
The documentation is not available anymore as the PR was closed or merged. |
(Internal discussion here) Note: there are 2 different topics here:
This PR currently handles 1. but not 2. |
Codecov ReportBase: 84.55% // Head: 84.69% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1143 +/- ##
==========================================
+ Coverage 84.55% 84.69% +0.13%
==========================================
Files 42 41 -1
Lines 4151 4130 -21
==========================================
- Hits 3510 3498 -12
+ Misses 641 632 -9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, works well! Thanks @Wauplin!
assert ( | ||
"_deprecate" in attrs | ||
), "A `_deprecate` method must be implemented to use `DeprecateListMetaclass`." | ||
assert ( | ||
list in bases | ||
), "Class must inherit from `list` to use `DeprecateListMetaclass`." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I would favor raising errors with the appropriate types rather than assert
, but if you feel like this is more coherent this way we can leave it be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep true. I usually use assertions only for internal logic issues (like here where the metaclass is really not meant to be reused by anyone). But agree on changing this :)
tests/test_hf_api.py
Outdated
@with_production_testing | ||
def test_list_datasets(self): | ||
def test_list_datasets_aaa(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this voluntary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahah, not a all 😄
Fix #1087 and prepare future pagination of
list_models
,list_datasets
andlist_spaces
(cc @Pierrci @julien-c - cf slack thread (internal link)).The idea is to return a custom list on which any method that is specific to lists (e.g. almost everything except
for item in list_models(): ...
) is deprecated. This should encourage users to process the output as an iterator instead of a list. In particular,len(list_models())
is deprecated (as it will not be accurate anymore when the api will be paginated. Listing discussions from a repo is already paginated (python generator inhfh
) which is good for consistency.Implementation-wise, I'm using a metaclass to decorate all methods (so a bit python automagic) but I didn't see another way of doing it. For the record, I couldn't use the
__getattribute__
method as it is not called for magic methods like__getitem__
or__len__
.