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

ENH Deprecate clone_from behavior #952

Merged
merged 29 commits into from
Aug 30, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
1aa664a
deprecate clone_from behavior
merveenoyan Jul 21, 2022
5c78379
added warnings import
merveenoyan Jul 21, 2022
ccbd583
make style
merveenoyan Jul 21, 2022
1bec939
added test
merveenoyan Jul 21, 2022
792af11
added repo creation to test setups
merveenoyan Jul 21, 2022
da18a5a
modified test to skip repo creation during setup
merveenoyan Jul 21, 2022
17deae2
fixed remaining of the tests by adding create_repo
merveenoyan Jul 21, 2022
a82b09d
fixed tests
merveenoyan Jul 22, 2022
b871f3d
fixed tests
merveenoyan Jul 22, 2022
8fd2f74
shifted head and added changes on top
merveenoyan Aug 1, 2022
1953d2a
shifted head and added changes on top
merveenoyan Aug 1, 2022
c2c61a4
Revert "shifted head and added changes on top"
merveenoyan Aug 1, 2022
b179f67
Revert "fixed tests"
merveenoyan Aug 1, 2022
7eff5e3
Revert "fixed tests"
merveenoyan Aug 1, 2022
684d822
Revert "fixed remaining of the tests by adding create_repo"
merveenoyan Aug 1, 2022
d11e626
Revert "modified test to skip repo creation during setup"
merveenoyan Aug 1, 2022
275c757
Revert "added repo creation to test setups"
merveenoyan Aug 1, 2022
9c933ca
reverted changes
merveenoyan Aug 1, 2022
f1704e3
updated version
merveenoyan Aug 1, 2022
79fc043
Merge branch 'main' into clone_from_deprecation
Wauplin Aug 18, 2022
19e82fc
expect deprecation for clone_from in tests
Wauplin Aug 18, 2022
e642740
Merge pull request #1 from huggingface/clone_from_deprecation_add_exp…
merveenoyan Aug 29, 2022
a1898e6
Merge branch 'main' into clone_from_deprecation
merveenoyan Aug 29, 2022
f5351f7
make style
merveenoyan Aug 29, 2022
d05d083
Update tests/test_repository.py
Wauplin Aug 29, 2022
12b47cd
Update tests/test_repository.py
Wauplin Aug 29, 2022
0e82e9e
fixed tests
merveenoyan Aug 29, 2022
30196f4
fixed tests
merveenoyan Aug 29, 2022
801c560
make style
merveenoyan Aug 29, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/huggingface_hub/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import tempfile
import threading
import time
import warnings
from contextlib import contextmanager
from pathlib import Path
from typing import Callable, Dict, Iterator, List, Optional, Tuple, Union
Expand Down Expand Up @@ -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.",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

FutureWarning,
)

self.client.create_repo(
repo_id=repo_id,
token=token,
Expand Down
5 changes: 4 additions & 1 deletion tests/test_repocard.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

repo_id=f"{USER}/{self.REPO_NAME}", token=self._token
)
self.repo = Repository(
self.repo_path / self.REPO_NAME,
clone_from=f"{USER}/{self.REPO_NAME}",
clone_from=url,
use_auth_token=self._token,
git_user="ci",
git_email="ci@dummy.com",
Expand Down
51 changes: 39 additions & 12 deletions tests/test_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,19 @@ def setUp(self):
logger.info(
f"Does {WORKING_REPO_DIR} exist: {os.path.exists(WORKING_REPO_DIR)}"
)

self.REPO_NAME = repo_name()
self._repo_url = self._api.create_repo(
repo_id=self.REPO_NAME, token=self._token
)
self._api.upload_file(
path_or_fileobj=BytesIO(b"some initial binary data: \x00\x01"),
path_in_repo="random_file.txt",
repo_id=f"{USER}/{self.REPO_NAME}",
token=self._token,
)
# skip creation of repository for clone from test
if self.shortDescription() != "SKIP CREATION":
self._repo_url = self._api.create_repo(
repo_id=self.REPO_NAME, token=self._token
)
self._api.upload_file(
path_or_fileobj=BytesIO(b"some initial binary data: \x00\x01"),
path_in_repo="random_file.txt",
repo_id=f"{USER}/{self.REPO_NAME}",
token=self._token,
)

def tearDown(self):
try:
Expand Down Expand Up @@ -140,6 +143,19 @@ def test_clone_from_space(self):
use_auth_token=self._token,
)

def test_clone_from_deprecation_warning(self):
"""SKIP CREATION"""
Copy link
Contributor

Choose a reason for hiding this comment

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

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

with pytest.raises(
FutureWarning,
match="Creating a repository through clone_from is deprecated*",
Wauplin marked this conversation as resolved.
Show resolved Hide resolved
):
Repository(
WORKING_REPO_DIR,
clone_from=f"{USER}/{uuid.uuid4()}",
use_auth_token=self._token,
repo_type="dataset",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

)

def test_clone_from_model(self):
temp_repo_url = self._api.create_repo(
repo_id=f"{self.REPO_NAME}-temp", token=self._token, repo_type="model"
Expand Down Expand Up @@ -437,9 +453,15 @@ def test_clone_with_endpoint(self):

@retry_endpoint
def test_clone_with_repo_name_and_org(self):
repo_url = self._api.create_repo(
token=self._token,
repo_id=f"valid_org/{self.REPO_NAME}",
repo_type="dataset",
)

clone = Repository(
f"{WORKING_REPO_DIR}/{self.REPO_NAME}",
clone_from=f"valid_org/{self.REPO_NAME}",
clone_from=repo_url,
use_auth_token=self._token,
git_user="ci",
git_email="ci@dummy.com",
Expand All @@ -455,7 +477,7 @@ def test_clone_with_repo_name_and_org(self):

Repository(
f"{WORKING_REPO_DIR}/{self.REPO_NAME}",
clone_from=f"valid_org/{self.REPO_NAME}",
clone_from=repo_url,
use_auth_token=self._token,
git_user="ci",
git_email="ci@dummy.com",
Expand Down Expand Up @@ -1698,6 +1720,11 @@ def test_clone_with_endpoint(self):

@retry_endpoint
def test_clone_with_repo_name_and_org(self):
self._api.create_repo(
token=self._token,
repo_id=f"valid_org/{self.REPO_NAME}",
repo_type="dataset",
)
clone = Repository(
f"{WORKING_DATASET_DIR}/{self.REPO_NAME}",
clone_from=f"valid_org/{self.REPO_NAME}",
Expand Down Expand Up @@ -1761,7 +1788,7 @@ def test_clone_with_repo_name_and_no_namespace(self):
self.assertRaises(
OSError,
Repository,
f"{WORKING_DATASET_DIR}/{self.REPO_NAME}",
f"{WORKING_REPO_DIR}/{self.REPO_NAME}",
clone_from=self.REPO_NAME,
repo_type="dataset",
use_auth_token=self._token,
Expand Down
3 changes: 2 additions & 1 deletion tests/test_snapshot_download.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,10 @@ def setUp(self) -> None:
if os.path.exists(REPO_NAME):
shutil.rmtree(REPO_NAME, onerror=set_write_permission_and_retry)
logger.info(f"Does {REPO_NAME} exist: {os.path.exists(REPO_NAME)}")
url = self._api.create_repo(repo_id=f"{USER}/{REPO_NAME}", token=self._token)
repo = Repository(
REPO_NAME,
clone_from=f"{USER}/{REPO_NAME}",
clone_from=url,
use_auth_token=self._token,
git_user="ci",
git_email="ci@dummy.com",
Expand Down