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

Uniformize hf_api a bit and add support for Spaces #792

Merged
merged 26 commits into from
Apr 14, 2022
Merged

Conversation

julien-c
Copy link
Member

No description provided.

@julien-c julien-c requested a review from LysandreJik March 23, 2022 14:53
Copy link
Contributor

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Some of the changes here are breaking changes. I think we should introduce a deprecation cycle like the other PRs if we're breaking API.

@julien-c
Copy link
Member Author

@adrinjalali I think only two lines in this diff are potential breaking changes, so I'll revert them. Other than that i don't think there any BC, but I'll check

@LysandreJik
Copy link
Member

Weird that CI didn't trigger on your PR

@julien-c
Copy link
Member Author

@LysandreJik probably GitHub being a bit down

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.

Other than the sha addition as a kwarg to DatasetInfo to correspond better to the ModelInfo, I don't see a breaking change; did I miss anything @adrinjalali?

src/huggingface_hub/constants.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

We also need to document the new methods in the README here.

src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
@LysandreJik
Copy link
Member

Added documentation for the methods that weren't touched by #782 so that everything is in the same format.

@julien-c
Copy link
Member Author

BTW feel free to rebase this on main if you'd like!

@LysandreJik
Copy link
Member

Yes, will now rebase on main as the big doc PR was just merged.

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 31, 2022

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

Copy link
Contributor

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Should the new methods also be included in a user guide?

src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
@LysandreJik
Copy link
Member

I should have addressed all comments.

Should the new methods also be included in a user guide?

They'll be in the API reference on merge, which I think is a good first step.

@LysandreJik
Copy link
Member

Need to patch the failure.

@LysandreJik
Copy link
Member

The switch from HfFolder.get_token() to _validate_or_retrieve_token in the model_info results in a bunch of failing tests. Taking a step back, I think it would be better to revert it as out of scope for this PR (it currently is like that on main), and address in a specifc PR.

@adrinjalali, please let me know if that works for you, and if so, if we can merge this PR.

Copy link
Contributor

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

mostly cosmetic comments, except HfFolder.get_token(). I think if a new method is failing on using validate_or_retrieve_token, it's in the scope of the same PR to see why it'd fail. We have 9 instances of using _validate_or_retrieve_token and only two HfFolder.get_token(). We don't have to remove the existing ones here, but new code shouldn't be use it.

src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
@LysandreJik
Copy link
Member

Thanks for your feedback. I've taken care of all your comments, except regarding the _validate_or_retrieve_token, as I stand by what I said in the previous comment. I understand how

I think if a new method is failing on using _validate_or_retrieve_token, it's in the scope of the same PR to see why it'd fail.

that would be sensible, but the two methods are not designed to work together. The _validate_or_retrieve_token method exists to either validate the passed token, or retrieve the logged-in token, before validating it. It cannot be used as a no-op in case the passed token is None and the user isn't logged-in.

That's exactly what model_info (from which a direct copy is made for both dataset_info and space_info) is advertised to do, though:

Get info on one specific model on huggingface.co

Model can be private if you pass an acceptable token or are logged in.

In the case where no acceptable token is passed and the user isn't logged-in, then it should still work. Updating to use _validate_or_retrieve_token breaks that, as it raises an error.

Furthermore, I think that since model_info aims to be the exact same as dataset_info and space_info, but for different repository types, they should not use different approaches to retrieve the token, as situations like the one above can arise, which results in a desynchronisation of the three methods and an API behaving a bit differently acorss seemingly identical methods. I think in this case, if it is out of scope to refactor model_info, then dataset_info and space_info ought to be designed exactly like model_info is on main.

@LysandreJik
Copy link
Member

Rebasing on main to fix the tests.

@adrinjalali
Copy link
Contributor

Ok, opened #837 to discuss the token issue. Happy to merge this as is and fix that issue later.

@adrinjalali adrinjalali merged commit f7627b4 into main Apr 14, 2022
@adrinjalali adrinjalali deleted the hf-api-adds branch April 14, 2022 10:46
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