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

Deprecate is_write_action and write_permission=True when login #2632

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Oct 24, 2024

(slightly related to #2631)

This PR takes care of removing any local checks related to tokens. Since finegrained and oauth tokens exist, we don't have a simple distinction between "read" and "write" permission, making a few features useless and outdated in the codebase.

This PR:

  1. deprecates is_write_action in build_hf_headers

=> this argument has close to no usage at all and complexifies the API for nothing. Its first purpose in #1064 was to raise an exception if a local token was not find and the HTTP call specifically needed a token (what I wrongly called "a write action" instead of "an authenticated action"). In the end it's much simpler if we let the server raise the exception when appropriate.
For context, before #1064, we had a distinction between "write actions" and "not write actions" in the sense that only "write actions" was using a token. All read actions where unauthenticated (even if a token was locally saved) which caused troubles when stable diffusion was released as a gated repo where even a read needed authentication. Since then, the locally saved token is always sent to the server if it exists.

  1. deprecates write_permission in login / notebook_login / interpreter_login helpers

=> with finegrained tokens, it doesn't make sense anymore to require a "write" token. Added in #1476.

  1. deprecates positional args in login / notebook_login / interpreter_login helpers

=> otherwise we won't be able to remove them. Always better to enforce keyword arguments for future maintenance.

@HuggingFaceDocBuilderDev

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.

Copy link
Contributor

@hanouticelina hanouticelina left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @Wauplin!

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.

Yes, the change sounds good to me. Thanks @Wauplin!

@Wauplin
Copy link
Contributor Author

Wauplin commented Oct 25, 2024

Thanks for the reviews! I fixed a few mypy errors so we should be good to merge :)

@Wauplin Wauplin merged commit c547c83 into main Oct 25, 2024
18 checks passed
@Wauplin Wauplin deleted the deprecate-is-write-action branch October 25, 2024 09:19
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