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 HfApi.create_repo when repo_type is 'space' #394

Merged
merged 13 commits into from Oct 29, 2021

Conversation

nateraw
Copy link
Contributor

@nateraw nateraw commented Oct 6, 2021

Resolves #393

Can't get local tests to work on staging endpoint, but when I switch the code to use the public endpoint + my personal token, the tests run just fine! They're skipped here since spaces is still in beta.

Copy link
Member

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

Great find! Thanks a lot

tests/test_hf_api.py Outdated Show resolved Hide resolved
@julien-c
Copy link
Member

julien-c commented Oct 7, 2021

Also requesting reviews from @cbensimon and @Pierrci re. the server/API side of this

@nateraw
Copy link
Contributor Author

nateraw commented Oct 7, 2021

Just noticed the CLI is broken as well - let's include that fix in this PR as well.

@nateraw nateraw linked an issue Oct 7, 2021 that may be closed by this pull request
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.

Looks good to me from the perspective of hfh! Very clean tests, thanks for working on it!

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
@@ -821,7 +846,8 @@ def update_repo_visibility(
path_prefix += REPO_TYPES_URL_PREFIXES[repo_type]

path = "{}{}/{}/settings".format(path_prefix, namespace, name)
json = {"private": private}
# HACK - hard coded recently added 'gated' param for now. Decide how to deal with this in the future.
json = {"private": private, "gated": False}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SBrandeis Repo updates fail if gated is not included as a parameter. Saw this was added recently, so just hardcoded False for now. Is that ok?

Referring to moon-landing PR: https://github.com/huggingface/moon-landing/pull/1316

CC @julien-c

Copy link
Member

Choose a reason for hiding this comment

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

API endpoint should probably default to false server-side, @SBrandeis

Does this currently break all repo creation from hf_api, @nateraw? not just spaces, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually its only failing for spaces. Can update accordingly here for now

Copy link
Member

Choose a reason for hiding this comment

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

very weird, but good to hear that =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all done 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Super weird that's it's failing only for spaces 🤔

Copy link
Member

Choose a reason for hiding this comment

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

In the meantime, you can also test in production by adding the @with_production_testing decorator to your tests. These will run on the production server instead of the staging, so can be helpful while spaces are unavailable on staging.

Copy link
Member

@julien-c julien-c Oct 28, 2021

Choose a reason for hiding this comment

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

not a great idea in my opinion, but we've just updated the staging env so should might work OTB now

Copy link
Member

Choose a reason for hiding this comment

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

Production tests are better than no tests :) if the staging is available, then that's definitely the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LysandreJik , after this convo earlier, we got the staging endpoint working 😀. All good!

@nateraw nateraw merged commit b514069 into huggingface:main Oct 29, 2021
@nateraw nateraw deleted the fix-create-space-repo branch October 29, 2021 00:17
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.

Cant create new space from huggingface-cli Cannot create new space with HfApi.create_repo
6 participants