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 tests involving temp directory on macOS #2052

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/huggingface_hub/file_download.py
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ def _create_symlink(src: str, dst: str, new_blob: bool = False) -> None:

By default, it will try to create a symlink using a relative path. Relative paths have 2 advantages:
- If the cache_folder is moved (example: back-up on a shared drive), relative paths within the cache folder will
not brake.
not break.
- Relative paths seems to be better handled on Windows. Issue was reported 3 times in less than a week when
changing from relative to absolute paths. See https://github.com/huggingface/huggingface_hub/issues/1398,
https://github.com/huggingface/diffusers/issues/2729 and https://github.com/huggingface/transformers/pull/22228.
Expand All @@ -882,7 +882,7 @@ def _create_symlink(src: str, dst: str, new_blob: bool = False) -> None:
cache, the file is duplicated on the disk.

In case symlinks are not supported, a warning message is displayed to the user once when loading `huggingface_hub`.
The warning message can be disable with the `DISABLE_SYMLINKS_WARNING` environment variable.
The warning message can be disabled with the `DISABLE_SYMLINKS_WARNING` environment variable.
"""
try:
os.remove(dst)
Expand Down
4 changes: 2 additions & 2 deletions src/huggingface_hub/utils/_fixes.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def SoftTemporaryDirectory(
prefix: Optional[str] = None,
dir: Optional[Union[Path, str]] = None,
**kwargs,
) -> Generator[str, None, None]:
) -> Generator[Path, None, None]:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's usually dangerous to update the return value of an helper but in this case it looks fine 👍

(+1 on using Path as much as possible. Codebase is currently a mix of str and Path for legacy reasons but 🤷)

"""
Context manager to create a temporary directory and safely delete it.

Expand All @@ -52,7 +52,7 @@ def SoftTemporaryDirectory(
See https://www.scivision.dev/python-tempfile-permission-error-windows/.
"""
tmpdir = tempfile.TemporaryDirectory(prefix=prefix, suffix=suffix, dir=dir, **kwargs)
yield tmpdir.name
yield Path(tmpdir.name).resolve()

try:
# First once with normal cleanup
Expand Down
3 changes: 1 addition & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import os
import shutil
from pathlib import Path
from typing import Generator

import pytest
Expand Down Expand Up @@ -30,7 +29,7 @@ def test_cache_dir(self) -> None:
```
"""
with SoftTemporaryDirectory() as cache_dir:
request.cls.cache_dir = Path(cache_dir).resolve()
request.cls.cache_dir = cache_dir
yield
# TemporaryDirectory is not super robust on Windows when a git repository is
# cloned in it. See https://www.scivision.dev/python-tempfile-permission-error-windows/.
Expand Down
5 changes: 3 additions & 2 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,10 @@ def test_every_as_float(self) -> None:
@patch("huggingface_hub.commands.upload.HfApi.create_repo")
def test_upload_folder_mock(self, create_mock: Mock, upload_mock: Mock, repo_info_mock: Mock) -> None:
with SoftTemporaryDirectory() as cache_dir:
cache_path = cache_dir.absolute().as_posix()
cmd = UploadCommand(
self.parser.parse_args(
["upload", "my-model", cache_dir, ".", "--private", "--include", "*.json", "--delete", "*.json"]
["upload", "my-model", cache_path, ".", "--private", "--include", "*.json", "--delete", "*.json"]
)
)
cmd.run()
Expand All @@ -227,7 +228,7 @@ def test_upload_folder_mock(self, create_mock: Mock, upload_mock: Mock, repo_inf
repo_id="my-model", repo_type="model", exist_ok=True, private=True, space_sdk=None
)
upload_mock.assert_called_once_with(
folder_path=cache_dir,
folder_path=cache_path,
path_in_repo=".",
repo_id=create_mock.return_value.repo_id,
repo_type="model",
Expand Down
5 changes: 2 additions & 3 deletions tests/test_utils_fixes.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ def test_yaml_dump_explicit_no_unicode(self) -> None:

class TestTemporaryDirectory(unittest.TestCase):
def test_temporary_directory(self) -> None:
with SoftTemporaryDirectory(prefix="prefix", suffix="suffix") as tmpdir:
self.assertIsInstance(tmpdir, str)
path = Path(tmpdir)
with SoftTemporaryDirectory(prefix="prefix", suffix="suffix") as path:
self.assertIsInstance(path, Path)
self.assertTrue(path.name.startswith("prefix"))
self.assertTrue(path.name.endswith("suffix"))
self.assertTrue(path.is_dir())
Expand Down
Loading