ENH Deprecate clone_from behavior #952

merveenoyan · 2022-07-21T16:36:47Z

Solves #905.

osanseviero

Very nice! Thanks for this. Could you also add a small test that checks that a warning is raised please?

Here is an example:

huggingface_hub/tests/test_keras_integration.py

Lines 323 to 326 in 1d3637c

    
           with pytest.warns( 
        
               FutureWarning, 
        
               match="`task_name` input argument is deprecated. Pass `tags` instead.", 
        
           ):

HuggingFaceDocBuilderDev · 2022-07-21T16:43:37Z

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

merveenoyan · 2022-07-21T17:12:43Z

plot twist: this behavior was used mainly for tests? ✨ so many tests depend on clone_from creating repositories ✨
I took a look at the tests and they have setUp (e.g. test_clone_with_repo_name_and_org), which creates a repository remotely so I don't understand why it tries to create a repository with clone_from and raises the warning (which is only raised when you get HTTPError for not having the repository remotely). 🦆 I'll take a look tomorrow.

later edit: some setUps do not create repository remotely that's why they raise HTTPErrors. (e.g. ones in test_repocard.py and snapshot download tests)

merveenoyan · 2022-07-21T18:05:07Z

I added creation of repositories to setups and managed to reduce failing tests to 3, will look for those on why they fail.
later edit: after next commit the test I've written for this has passed though. (needed to skip repo creation during setup.)

LysandreJik

Looks good to me! We should be proactive about reaching out to repositories that have the Repository set to create the repository when it doesn't exist. I think transformers does that under the hood, I'll see to change that there. Do we know any other libraries that leverage Repository to create their repositories on the hub?

merveenoyan · 2022-07-22T15:24:18Z

maybe @osanseviero knows @LysandreJik 🙂

osanseviero

Thanks a lot for this PR! I think we can merge after two things:

I think we should revert the changes in the tests as I would not expect changes in existing tests since we're only adding a warning. That way we're sure nothing changes :)
Once the above is reverted, let's get to green and merge 🔥

osanseviero · 2022-07-23T16:02:56Z

src/huggingface_hub/repository.py

@@ -655,6 +656,12 @@ def clone_from(self, repo_url: str, use_auth_token: Union[bool, str, None] = Non
                                " on Hugging Face Hub."
                            )
                        else:
+                            warnings.warn(
+                                "Creating a repository through clone_from is deprecated"
+                                "will be removed in v0.10.",


Maybe let's change this to v0.11., as 0.9 is already the next release, that way we have 2 releases before deprecating

osanseviero · 2022-07-23T16:05:58Z

tests/test_repocard.py

@@ -204,9 +204,12 @@ def setUpClass(cls):
    def setUp(self) -> None:
        self.repo_path = Path(tempfile.mkdtemp())
        self.REPO_NAME = repo_name()
+        url = self._api.create_repo(


When adding some changes, if you have to change tests, that's a red flag to watch out! As you're just adding a warning, nothing should change in any existing tests. It's ok if current tests depend on clone_from, I would revert the changes to keep them as they were as we're not changing any behaviour now. We should change the text when actually deprecating this. WDYT?

osanseviero · 2022-07-23T16:06:36Z

tests/test_repository.py

@@ -140,6 +143,19 @@ def test_clone_from_space(self):
                use_auth_token=self._token,
            )

+    def test_clone_from_deprecation_warning(self):
+        """SKIP CREATION"""


I would remove this docstring as the name of the test is already great 🔥

osanseviero · 2022-07-23T16:07:09Z

tests/test_repository.py

+                WORKING_REPO_DIR,
+                clone_from=f"{USER}/{uuid.uuid4()}",
+                use_auth_token=self._token,
+                repo_type="dataset",


Nit: feel free to remove repo_type. The simpler the test the easier to understand, so anything not strictly needed I would remove

I did it because there was two of those tests, one was for model repository and one was for dataset repository.

I think that given the behaviour you're changing is ok to just cover one case

osanseviero · 2022-07-23T16:15:11Z

@LysandreJik @merveenoyan quick recap of 3rd party libraries with push_to_hub that I'm aware of:

🟡 Adapter Transformers (note they pin to be below 0.8) cc @calpt, pointer is at https://github.com/adapter-hub/adapter-transformers/blob/master/src/transformers/utils/hub.py#L1014. So this won't break as they pin the version, but we should update this nevertheless to either create the repo before or use the non-git-based methods
🟢 ML Agents: they create the repo before cloning it
🟢 Stable Baselines 3 and RL Zoo: use non-git based methods now! (cc @simoninithomas for visibility)
🟢 Sentence Transformers creates the repo before
🟢 SpaCy uses non-git based methods now! 🥳

osanseviero · 2022-07-29T12:59:20Z

Hey @merveenoyan, we will likely do a release next week. There were some very minor changes to be done in the PR before merging, would be great if we could merge the PR some time early *next week.

merveenoyan · 2022-08-01T13:48:02Z

@osanseviero I created a new branch from 1bec939 I'm testing atm. Once it's done I'll merge to this branch and ask you to review.

This reverts commit 8fd2f74.

This reverts commit b871f3d.

This reverts commit a82b09d.

This reverts commit 17deae2.

This reverts commit da18a5a.

This reverts commit 792af11.

Wauplin · 2022-08-17T10:36:19Z

@merveenoyan I'm finally getting back to you !

PR has been merged and you can now use this decorator to deprecate arguments and this one to test it. Hope it will be easy from now on and sorry for the delay ! 😬

merveenoyan · 2022-08-29T11:59:01Z

@Wauplin I feel like it's best if I just close this PR and start from scratch instead of rewinding and rebasing things 😅

Wauplin · 2022-08-29T12:00:03Z

Yes indeed, sorry for the mess

merveenoyan · 2022-08-29T12:21:55Z

@Wauplin in this case, clone_from as an argument is not really deprecated, what is deprecated inside is using clone_from to create a new repo from scratch (you should still be able to clone a repo using clone_from argument) in that case, I shouldn’t be decorating the function taking clone_from, but only decorating the tests right?
(however, test decoration goes hand in hand with deprecation so this is not really useful)

so for this one, is it the best if I manually edit tests? or would decorating tests only work?

Wauplin · 2022-08-29T12:29:27Z

@merveenoyan Indeed we don't have a decorator for deprecating a specific usage of a valid argument (I don't even know how we could do such a decorator 😁 ).

However you can still use the decorator in the tests. Cherry-picking this commit should solve your problem merveenoyan@19e82fc.

…ected_deprecation_in_tests Clone from deprecation add expected deprecation in tests

merveenoyan · 2022-08-29T14:58:07Z

@Wauplin as said by Simon, -W error::FutureWarning turn matching warnings into exceptions so the future warning is not raised with pytest.warns as used by the decorator you've written thus the tests fail. (we need z for errors instead?)
see

def _inner_decorator(test_function: Callable) -> Callable:
        @wraps(test_function)
        def _inner_test_function(*args, **kwargs):
            with pytest.warns(FutureWarning, match=f".*'{function_name}'.*"):
                return test_function(*args, **kwargs)

        return _inner_test_function

I will have to edit the tests myself or we can change the pytest.warns or how the tests are executed. What do you say?

Wauplin · 2022-08-29T15:12:52Z

In theory pytest.warns is catching during the execution of the test. The -W error::FutureWarning flag turns the warning into an error only at the end when all tests are done and pytest gather the results.

I think if there is still an issue it less in a typo or something 😕 I'm having a quick look at it.

tests/test_repository.py

Wauplin · 2022-08-29T15:45:11Z

Seems like:

test_clone_with_endpoint is not decorated.
test_clone_with_repo_name_and_user_namespace: there are 2 of them. They should not be decorated but one is.
test_clone_with_repo_name_org_and_no_auth_token : there are 2 of them. They should be decorated but one is not.

I can't really make the changes directly because it's on a fork 😕

codecov · 2022-08-29T18:29:41Z

Codecov Report

Merging #952 (801c560) into main (128d114) will increase coverage by 21.17%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main     #952       +/-   ##
===========================================
+ Coverage   60.27%   81.44%   +21.17%     
===========================================
  Files          31       31               
  Lines        3421     3423        +2     
===========================================
+ Hits         2062     2788      +726     
+ Misses       1359      635      -724

Impacted Files	Coverage Δ
src/huggingface_hub/repository.py	`80.00% <100.00%> (+0.58%)`	⬆️
src/huggingface_hub/__init__.py	`78.37% <0.00%> (+2.70%)`	⬆️
src/huggingface_hub/_commit_api.py	`92.24% <0.00%> (+3.44%)`	⬆️
src/huggingface_hub/utils/_deprecation.py	`100.00% <0.00%> (+4.34%)`	⬆️
src/huggingface_hub/commands/user.py	`33.33% <0.00%> (+9.52%)`	⬆️
src/huggingface_hub/community.py	`91.66% <0.00%> (+16.66%)`	⬆️
src/huggingface_hub/lfs.py	`74.52% <0.00%> (+17.83%)`	⬆️
src/huggingface_hub/file_download.py	`85.35% <0.00%> (+22.37%)`	⬆️
src/huggingface_hub/utils/_errors.py	`94.36% <0.00%> (+26.76%)`	⬆️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

osanseviero · 2022-08-30T07:38:42Z

🥳

deprecate clone_from behavior

1aa664a

merveenoyan requested a review from osanseviero July 21, 2022 16:37

merveenoyan added 2 commits July 21, 2022 18:38

added warnings import

5c78379

make style

ccbd583

osanseviero reviewed Jul 21, 2022

View reviewed changes

added test

1bec939

added repo creation to test setups

792af11

merveenoyan added 2 commits July 21, 2022 20:10

modified test to skip repo creation during setup

da18a5a

fixed remaining of the tests by adding create_repo

17deae2

LysandreJik approved these changes Jul 22, 2022

View reviewed changes

fixed tests

a82b09d

fixed tests

b871f3d

osanseviero approved these changes Jul 23, 2022

View reviewed changes

merveenoyan added 10 commits August 1, 2022 16:29

shifted head and added changes on top

8fd2f74

shifted head and added changes on top

1953d2a

Revert "shifted head and added changes on top"

c2c61a4

This reverts commit 8fd2f74.

Revert "fixed tests"

b179f67

This reverts commit b871f3d.

Revert "fixed tests"

7eff5e3

This reverts commit a82b09d.

Revert "fixed remaining of the tests by adding create_repo"

684d822

This reverts commit 17deae2.

Revert "modified test to skip repo creation during setup"

d11e626

This reverts commit da18a5a.

Revert "added repo creation to test setups"

275c757

This reverts commit 792af11.

reverted changes

9c933ca

updated version

f1704e3

Wauplin added 2 commits August 18, 2022 10:12

Merge branch 'main' into clone_from_deprecation

79fc043

expect deprecation for clone_from in tests

19e82fc

merveenoyan closed this Aug 29, 2022

merveenoyan reopened this Aug 29, 2022

merveenoyan added 3 commits August 29, 2022 14:35

Merge pull request #1 from huggingface/clone_from_deprecation_add_exp…

e642740

…ected_deprecation_in_tests Clone from deprecation add expected deprecation in tests

Merge branch 'main' into clone_from_deprecation

a1898e6

make style

f5351f7

Wauplin reviewed Aug 29, 2022

View reviewed changes

tests/test_repository.py Outdated Show resolved Hide resolved

Update tests/test_repository.py

d05d083

Wauplin reviewed Aug 29, 2022

View reviewed changes

tests/test_repository.py Outdated Show resolved Hide resolved

Update tests/test_repository.py

12b47cd

merveenoyan added 3 commits August 29, 2022 20:08

fixed tests

0e82e9e

fixed tests

30196f4

make style

801c560

merveenoyan requested a review from Wauplin August 29, 2022 18:32

Wauplin approved these changes Aug 30, 2022

View reviewed changes

Wauplin merged commit 7b57719 into huggingface:main Aug 30, 2022

merveenoyan deleted the clone_from_deprecation branch August 30, 2022 09:34

merveenoyan mentioned this pull request Aug 30, 2022

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

Closed

Wauplin mentioned this pull request Sep 15, 2022

Deprecate private and repo_type in Repository class #1055

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Deprecate clone_from behavior #952

ENH Deprecate clone_from behavior #952

merveenoyan commented Jul 21, 2022

osanseviero left a comment

HuggingFaceDocBuilderDev commented Jul 21, 2022 •

edited

Loading

merveenoyan commented Jul 21, 2022 •

edited

Loading

merveenoyan commented Jul 21, 2022 •

edited

Loading

LysandreJik left a comment

merveenoyan commented Jul 22, 2022

osanseviero left a comment

osanseviero Jul 23, 2022

osanseviero Jul 23, 2022

osanseviero Jul 23, 2022

osanseviero Jul 23, 2022

merveenoyan Jul 25, 2022

osanseviero Jul 25, 2022

osanseviero commented Jul 23, 2022 •

edited

Loading

osanseviero commented Jul 29, 2022

merveenoyan commented Aug 1, 2022

Wauplin commented Aug 17, 2022

merveenoyan commented Aug 29, 2022

Wauplin commented Aug 29, 2022

merveenoyan commented Aug 29, 2022 •

edited

Loading

Wauplin commented Aug 29, 2022

merveenoyan commented Aug 29, 2022

Wauplin commented Aug 29, 2022

Wauplin commented Aug 29, 2022

codecov bot commented Aug 29, 2022

osanseviero commented Aug 30, 2022

	with pytest.warns(
	FutureWarning,
	match="`task_name` input argument is deprecated. Pass `tags` instead.",
	):

ENH Deprecate clone_from behavior #952

ENH Deprecate clone_from behavior #952

Conversation

merveenoyan commented Jul 21, 2022

osanseviero left a comment

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Jul 21, 2022 • edited Loading

merveenoyan commented Jul 21, 2022 • edited Loading

merveenoyan commented Jul 21, 2022 • edited Loading

LysandreJik left a comment

Choose a reason for hiding this comment

merveenoyan commented Jul 22, 2022

osanseviero left a comment

Choose a reason for hiding this comment

osanseviero Jul 23, 2022

Choose a reason for hiding this comment

osanseviero Jul 23, 2022

Choose a reason for hiding this comment

osanseviero Jul 23, 2022

Choose a reason for hiding this comment

osanseviero Jul 23, 2022

Choose a reason for hiding this comment

merveenoyan Jul 25, 2022

Choose a reason for hiding this comment

osanseviero Jul 25, 2022

Choose a reason for hiding this comment

osanseviero commented Jul 23, 2022 • edited Loading

osanseviero commented Jul 29, 2022

merveenoyan commented Aug 1, 2022

Wauplin commented Aug 17, 2022

merveenoyan commented Aug 29, 2022

Wauplin commented Aug 29, 2022

merveenoyan commented Aug 29, 2022 • edited Loading

Wauplin commented Aug 29, 2022

merveenoyan commented Aug 29, 2022

Wauplin commented Aug 29, 2022

Wauplin commented Aug 29, 2022

codecov bot commented Aug 29, 2022

Codecov Report

osanseviero commented Aug 30, 2022

HuggingFaceDocBuilderDev commented Jul 21, 2022 •

edited

Loading

merveenoyan commented Jul 21, 2022 •

edited

Loading

merveenoyan commented Jul 21, 2022 •

edited

Loading

osanseviero commented Jul 23, 2022 •

edited

Loading

merveenoyan commented Aug 29, 2022 •

edited

Loading