FIX Avoid creating repository when it exists on remote #900

merveenoyan · 2022-06-10T13:45:15Z

This PR partially fixes #672.

I observed that the following error pops up when I create a repository using huggingface API create_repo and then have a local repository cloning from the URL using a token (here)
ValueError: No space_sdk provided. create_repo expects space_sdk to be one of ['gradio', 'streamlit', 'static'] when repo_type is 'space'
So I noticed the following block was causing the problem:

if token is not None:
                whoami_info = self.client.whoami(token)
                user = whoami_info["name"]
                valid_organisations = [org["name"] for org in whoami_info["orgs"]]

                if namespace is not None:
                    repo_id = f"{namespace}/{repo_id}"
                repo_url += repo_id

                scheme = urlparse(repo_url).scheme
                repo_url = repo_url.replace(f"{scheme}://", f"{scheme}://user:{token}@")

                if namespace == user or namespace in valid_organisations:
                    self.client.create_repo(
                        repo_id=repo_id,
                        token=token,
                        repo_type=self.repo_type,
                        exist_ok=True,
                        private=self.private,
                    )
            else:
                if namespace is not None:
                    repo_url += f"{namespace}/"
                repo_url += repo_id

Only name being valid shouldn't be enough to create a repository, we have to check if the repository exists on remote so I added a small check on that (thanks @muellerzr for pointing out to particular HfApi function that does it).

With this, space_sdk error should be gone. I'll add test once @LysandreJik approves my logic in here.

Also this is separate but IDK if it's good to have an attribute clone_from and the clone_from() function. I can refactor couple of things I saw there that's not good to me as well.

HuggingFaceDocBuilderDev · 2022-06-10T13:48:23Z

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

adrinjalali

Also tests? :D

adrinjalali · 2022-06-10T14:23:24Z

src/huggingface_hub/repository.py

+                    try:
+                        _ = HfApi().repo_info(f"{namespace}/{repo_id}")
+                    except HTTPError:
+                        self.client.create_repo(


I'm very confused, why would clone_from create a repo remotely under any circumstances?

It's because link given to clone_from might not be valid (there might not be a repository in remote under given namespace). Maybe it's better if I put a warning saying the remote repository doesn't exist instead. :)

It's complicated. I really don't think it's appropriate for clone_from to create anything. It should raise an error if the remote repository doesn't exist. Fixing it would be backward incompatible, but I think we can think of it as a bugfix. WDYT @LysandreJik ?

I just realized there are many tests that will fail and we'll break backwards compatibility if we change behavior of clone_from to only clone remote repository, I also thought that it was only used to clone existing repository when creating repository on local. pinging @LysandreJik about why the design is that way 😅

(ok apparently you left a comment but I didn't see it bcs I didn't refresh the page lol)

(i agree that Repository clone_from shouldn't create a repo)

(can't find at the moment the past discussion with a potential explicit Repository.create() -> Repository)

Yes, I also think that clone_from shouldn't create the repository if it doesn't exist, it isn't really its place to do so. I wouldn't say removing it is a bugfix as when I implemented it I shared it as such, but we can deprecate it for sure.

I say we merge this and then deprecate it.

merveenoyan · 2022-06-10T15:25:45Z

Super weird thing
I can do following (which was my original problem) with no issue.

api = HfApi()

temp_repo_url = api.create_repo(
            repo_id="merve/temp", token=token, 
            repo_type="space", space_sdk="gradio"
        )
repo = Repository("./dummy", clone_from=temp_repo_url,
        use_auth_token=token
        )

But when I run the test I've written (below) I still get the same error.

def test_init_clone_from(self):
        temp_repo_url = self._api.create_repo(
            repo_id=f"{self.REPO_NAME}-temp", token=self._token, 
            repo_type="space", space_sdk="gradio"
        )
        repo = Repository(WORKING_REPO_DIR, clone_from=temp_repo_url,
        use_auth_token=self._token
        )
        self._api.delete_repo(
                repo_id=f"{self.REPO_NAME}-temp", token=self._token
            )

I have other stuff to do so will leave this for monday.

osanseviero · 2022-06-10T15:31:01Z

"I can do following (which was my original problem) with no issue."
Are you sure you don't have installed from your git branch? This should not work as Repository calls create_repo without the sdk which was the origin of the issue

merveenoyan · 2022-06-10T16:07:16Z

@osanseviero yup I'm sure (I uninstalled and installed again)
What I'm testing here is that it should bypass creating the repository remotely given there's a repository remotely already (which we check with HfApi.repo_info()) it shouldn't care about space_sdk at all.

When I test this in a separate script, the repo is created remotely and is cloned well (when we instantiate Repository) that I can see there's README file of space which is the behavior we want here.

osanseviero · 2022-06-13T13:44:56Z

This seems to be passing the tests now 😄

adrinjalali

I'm happy to merge and deprecate in a separate PR.

LysandreJik

Looks good to me! Only left a comment regarding space_sdk.

LysandreJik · 2022-06-13T16:10:00Z

src/huggingface_hub/repository.py

+                    try:
+                        _ = HfApi().repo_info(f"{namespace}/{repo_id}")
+                    except HTTPError:
+                        self.client.create_repo(


LysandreJik · 2022-06-13T16:11:34Z

src/huggingface_hub/repository.py

+                            token=token,
+                            repo_type=self.repo_type,
+                            exist_ok=True,
+                            private=self.private,


Wouldn't this also need a space_sdk in case the repository in question is a space? i.e., if the repo is a space that doesn't exist, it'll try to create it but fail at doing so.

As we're deprecating, maybe that's intended and we keep that use-case as it is (i.e., not working)

I will raise a ValueError in case someone tries to create Space that doesn't exist because otherwise I have to add space_sdk as a class attribute to Repository (we're later deprecating the repository creation in clone_from anyway so there's no point to add that). I'll add two tests, one for model repo and one for Space repo to see if the error is raised. We can merge tomorrow ^^

Sounds great, thank you @merveenoyan!

merveenoyan · 2022-06-14T14:01:19Z

Please do not review I know what the problem with tests is :D

merveenoyan · 2022-06-14T15:14:57Z

I added three tests:

test_init_clone_from() checks that when space exists it's cloned well
test_clone_from_space() makes sure the space is not created if it doesn't exist
test_clone_from_model() makes sure that model is created if it doesn't exist.

merveenoyan · 2022-06-14T15:34:14Z

Tests are not passing because moon-landing staging is down 😅

LysandreJik · 2022-06-14T16:31:55Z

@merveenoyan it seems you're using an outdated version of black. Could you updated to the one required by huggingface_hub and run the code quality fix? Should be something like the following, from the root of your clone:

pip install -U -e .[quality]
make style
make quality

@SBrandeis, @XciD, would you have why we're getting errors 500 on the test_init_clone_from test? It seems like creating a space using create_repo doesn't work and throws an error 500. Reproducible code example:

temp_repo_url = create_repo(
    repo_id=f"temp",
    token=token,
    repo_type="space",
    space_sdk="streamlit"
)

I tried with both streamlit, gradio. However, static seems to work just fine!

XciD · 2022-06-14T16:39:12Z

could this be related to new spaces infra @christophe-rannou

christophe-rannou · 2022-06-14T16:49:35Z

@LysandreJik I just created a repo using the CLI with success. Maybe there's an issue with your token ?

LysandreJik · 2022-06-14T16:55:19Z

@christophe-rannou the token is that of the CI, we're using the same token for all CI. Were you on the production endpoint or the staging endpoint?

christophe-rannou · 2022-06-14T16:57:51Z

Production one, it's the staging one that is not working ?

XciD · 2022-06-14T16:58:29Z

Production one, it's the staging one that is not working ?

Seems related to CI, will investigate.

merveenoyan · 2022-06-14T17:03:28Z

This one fails HfApiEndpointsTest.test_create_update_and_delete_space_repo with 500 like it was failing for my PR, see below where the error comes from (due to picking other SDKs for others)

for sdk in SPACES_SDK_TYPES:
            SPACE_REPO_NAME = space_repo_name(sdk)
            self._api.create_repo(
                repo_id=SPACE_REPO_NAME,
                token=self._token,
                repo_type=REPO_TYPE_SPACE,
                space_sdk=sdk,
            )
            res = self._api.update_repo_visibility(
                repo_id=SPACE_REPO_NAME,
                token=self._token,
                private=True,
                repo_type=REPO_TYPE_SPACE,
            )
            self.assertTrue(res["private"])
            res = self._api.update_repo_visibility(
                repo_id=SPACE_REPO_NAME,
                token=self._token,
                private=False,
                repo_type=REPO_TYPE_SPACE,
            )

LysandreJik · 2022-06-15T06:58:05Z

This exact test fails on main since #898 was merged, I would offer to skip that test until this is resolved, and merge this PR. Ok for you @merveenoyan ?

cf https://github.com/huggingface/huggingface_hub/runs/6882266625?check_suite_focus=true

merveenoyan · 2022-06-15T08:41:42Z

@LysandreJik sure!

osanseviero · 2022-06-15T09:28:46Z

Was this supposed to be merged? As @LysandreJik mentioned, I think we should add the flag/decorator to skip the test so the tests are not failing ( @unittest.SkipTest for example)

LysandreJik · 2022-06-15T10:25:55Z

No problem @osanseviero, I opened a PR to fix the problem there: #908

But now we have another problem with our CI because of the new datasets release, so waiting until that is fixed to merge it.

XciD · 2022-06-15T10:47:04Z

About the spaces bug, we found it.
Waiting for https://github.com/huggingface/moon-landing/pull/3252/commits/f2e8068348e225ba1f07c5d27a38843253e23dcb to be merged.

XciD · 2022-06-15T10:54:08Z

About the spaces bug, we found it. Waiting for huggingface/moon-landing@f2e8068 to be merged.

Fixed

* fix for spaces * fix for spaces * removed creating repository and added warning * revert my changes * added tests * removed debugger 😐 * fixed repository removal * Added tests and error * import pytest * fixed tests * fixed tests * style * removed repo removal * make style * fixed test with Lysandres patch * added fix

@XciD

* added autoeval fields to repocard types * modified test, linted * Fix typo (#902) * Refine 404 errors (#878) * Add errors * Style * Review * hf_hub_download support * Errors for hf_hub_download * Typo * Handle 401 error * Tests for 401 error * Typo * Review comments * added documentation for the repocard change in repocard, updated metadata_eval_result, added documentation * 🏗 Use `hub-ci` for tests (#898) * 🏗 Use `hub-ci` for tests cc @XciD * 🩹 Also update URL for staging mode * ✅ 401 is raised when the user is not authenticated * 🗑 Deprecare `identical_ok` * Longer deprecation period * ✅ Fix the last failing test * Warning match docstring * FIX Avoid creating repository when it exists on remote (#900) * fix for spaces * fix for spaces * removed creating repository and added warning * revert my changes * added tests * removed debugger 😐 * fixed repository removal * Added tests and error * import pytest * fixed tests * fixed tests * style * removed repo removal * make style * fixed test with Lysandres patch * added fix * Remove deprecations (#910) * Remove deprecations * Typo * Update src/huggingface_hub/README.md Co-authored-by: Julien Chaumond <julien@huggingface.co> Co-authored-by: Julien Chaumond <julien@huggingface.co> * Add request ID to all requests (#909) * Add request ID to all requests * Typo * Typo * Review comments * Update src/huggingface_hub/utils/_errors.py Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * Invert deprecation for create_repo (#912) * Invert deprecation for create_repo * Set to 0.10 * Constant was accidentally removed during deprecation transition (#913) Co-authored-by: Lyle Nel <lyle@owlin.com> * Update src/huggingface_hub/repocard_types.py Co-authored-by: lewtun <lewis.c.tunstall@gmail.com> * Update repocard_types.py * Update repocard_types.py * reorded metadata_eval_request_docs, added metrics_config and metrics_verified to tests Co-authored-by: lsb <leebutterman@gmail.com> Co-authored-by: Lysandre Debut <lysandre.debut@reseau.eseo.fr> Co-authored-by: Simon Brandeis <33657802+SBrandeis@users.noreply.github.com> Co-authored-by: Merve Noyan <merveenoyan@gmail.com> Co-authored-by: Julien Chaumond <julien@huggingface.co> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Lyle Nel <lyle-nel@users.noreply.github.com> Co-authored-by: Lyle Nel <lyle@owlin.com> Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>

fix for spaces

8b733cb

merveenoyan requested review from LysandreJik and removed request for LysandreJik June 10, 2022 13:47

fix for spaces

dfcec50

adrinjalali reviewed Jun 10, 2022

View reviewed changes

merveenoyan added 2 commits June 10, 2022 16:31

removed creating repository and added warning

1b2bad0

revert my changes

227327d

merveenoyan added 3 commits June 13, 2022 14:40

added tests

90080fa

removed debugger 😐

1528aad

fixed repository removal

dd59ebb

merveenoyan requested review from adrinjalali, julien-c and LysandreJik June 13, 2022 13:16

adrinjalali approved these changes Jun 13, 2022

View reviewed changes

adrinjalali mentioned this pull request Jun 13, 2022

Repository.clone_from shouldn't create a repo on the hub #905

Closed

LysandreJik reviewed Jun 13, 2022

View reviewed changes

merveenoyan added 2 commits June 14, 2022 15:04

Added tests and error

66d831f

import pytest

c7da56f

merveenoyan added 4 commits June 14, 2022 17:03

fixed tests

787e524

Merge branch 'main' of github.com:merveenoyan/huggingface_hub into main

b0cbaf9

fixed tests

d6ab338

style

85126fc

removed repo removal

06bfeb4

merveenoyan added 2 commits June 14, 2022 18:43

make style

9913a99

fixed test with Lysandres patch

78d4314

added fix

550acb6

LysandreJik mentioned this pull request Jun 15, 2022

hub-ci returning error 500 when trying to create a Space #907

Closed

merveenoyan merged commit c9fb53e into huggingface:main Jun 15, 2022

FIX Avoid creating repository when it exists on remote #900

FIX Avoid creating repository when it exists on remote #900

Conversation

merveenoyan commented Jun 10, 2022 • edited Loading

HuggingFaceDocBuilderDev commented Jun 10, 2022 • edited Loading

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merveenoyan Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merveenoyan Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merveenoyan commented Jun 10, 2022 • edited Loading

osanseviero commented Jun 10, 2022

merveenoyan commented Jun 10, 2022 • edited Loading

osanseviero commented Jun 13, 2022

adrinjalali left a comment

Choose a reason for hiding this comment

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merveenoyan commented Jun 14, 2022

merveenoyan commented Jun 14, 2022 • edited Loading

merveenoyan commented Jun 14, 2022 • edited Loading

LysandreJik commented Jun 14, 2022

XciD commented Jun 14, 2022

christophe-rannou commented Jun 14, 2022

LysandreJik commented Jun 14, 2022

christophe-rannou commented Jun 14, 2022

XciD commented Jun 14, 2022

merveenoyan commented Jun 14, 2022

LysandreJik commented Jun 15, 2022 • edited Loading

merveenoyan commented Jun 15, 2022

osanseviero commented Jun 15, 2022

LysandreJik commented Jun 15, 2022

XciD commented Jun 15, 2022

XciD commented Jun 15, 2022

merveenoyan commented Jun 10, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Jun 10, 2022 •

edited

Loading

merveenoyan Jun 10, 2022 •

edited

Loading

merveenoyan Jun 10, 2022 •

edited

Loading

merveenoyan commented Jun 10, 2022 •

edited

Loading

merveenoyan commented Jun 10, 2022 •

edited

Loading

merveenoyan commented Jun 14, 2022 •

edited

Loading

merveenoyan commented Jun 14, 2022 •

edited

Loading

LysandreJik commented Jun 15, 2022 •

edited

Loading