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

Fix list_models bool parameters #1152

Merged
merged 8 commits into from
Nov 7, 2022
Merged

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Nov 3, 2022

Fix #1146.

With this PR, params fetch_config and cardData are not sent when value is False in list_models list_datasets. Also switched their type from Optional[bool] = None to bool = False for simplicity.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 3, 2022

The documentation is not available anymore as the PR was closed or merged.

@Wauplin Wauplin changed the title [WIP] Fix list_models bool parameters Fix list_models bool parameters Nov 3, 2022
@Wauplin Wauplin marked this pull request as ready for review November 3, 2022 15:25
Comment on lines +943 to +947
@_deprecate_arguments(
version="0.14",
deprecated_args={"cardData"},
custom_message="Use 'full' instead.",
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cardData and full had exactly the same behavior so I squeezed cardData.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I tried it as well and it returns the same values. I'm not sure if this is intended though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the codebase it was literally:

if full is not None:
    if full:
        params.update({"full": True})
if cardData is not None:
    if cardData:
        params.update({"full": True})

And I tested in on the API, "cardData" doesn't seem to be a valid parameter for the /datasets endpoint. That's why I just removed the cardData param. I tend to think that it comes from a copy-paste of the arguments from list_models and not adapted correctly.

https://huggingface.co/api/datasets?limit=1 => base
https://huggingface.co/api/datasets?limit=1&cardData=1 => base
https://huggingface.co/api/datasets?limit=1&full=1 => full

Copy link
Member

Choose a reason for hiding this comment

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

ah lol indeed!

Copy link
Contributor Author

@Wauplin Wauplin Nov 4, 2022

Choose a reason for hiding this comment

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

Apart from that, are you ok with the changes in the PR @LysandreJik ? 🙂

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Base: 84.59% // Head: 84.57% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (e66f143) compared to base (c2dbfd7).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1152      +/-   ##
==========================================
- Coverage   84.59%   84.57%   -0.02%     
==========================================
  Files          42       42              
  Lines        4187     4182       -5     
==========================================
- Hits         3542     3537       -5     
  Misses        645      645              
Impacted Files Coverage Δ
src/huggingface_hub/_login.py 47.05% <100.00%> (ø)
src/huggingface_hub/hf_api.py 87.56% <100.00%> (-0.09%) ⬇️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

LGTM!

Comment on lines +943 to +947
@_deprecate_arguments(
version="0.14",
deprecated_args={"cardData"},
custom_message="Use 'full' instead.",
)
Copy link
Member

Choose a reason for hiding this comment

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

ah lol indeed!

@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 7, 2022

Thanks for the review and approval @LysandreJik, I'm merging :)

@Wauplin Wauplin merged commit 9ccdf02 into main Nov 7, 2022
@Wauplin Wauplin deleted the 1146-fix-bool-params-in-list-methods branch November 7, 2022 09:40
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.

At least some boolean arguments in query functions are not working correctly
4 participants