Repository: huggingface/huggingface_hub
Pull request #2052 (2024-02-27 23:08:03)

Fix tests involving temp directory on macOS
Tests that use a mixture  `SoftTemporaryDirectory` and the `fx_cache_dir` fixture may fail on Mac OS. 

The failure usually says something like 
```
FAILED tests/test_file_download.py::HfHubDownloadToLocalDir::test_with_local_dir_and_symlinks_and_overwrite - OSError: [Errno 30] Read-only file system: '/tmpgucu6pwx'
```

The problem is that the code trying to create a symlink in `_create_symlink` tries to find a relative path from src to dst in
```
relative_src = os.path.relpath(abs_src, abs_dst_folder)
```
The problem is that on Mac OS, abs_src and abs_dst have a different root path due to the way the OS handles temp directories.
The one from the fixture is a resolved path to `/private/var/folders/…` while the one from `SoftTemporaryDirectory` is still an unresolved path pointing to `/var/folders/` (`/var/` is actually `var -> private/var`). And thus, their common ancestor is `/` which we try use to to write the symlink to (which obviously fails). 

I removed resolving the path from the fixture and instead resolve the path all the time in `SoftTemporaryDirectory` (which is used by the fixture) so all call sites get the same resolved path.

HuggingFaceDocBuilderDev: The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_2052). All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/2052?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 80.40%. Comparing base [(`c32602a`)](https://app.codecov.io/gh/huggingface/huggingface_hub/commit/c32602ac03308812595b271233140710f1865be8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) to head [(`8d717c7`)](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/2052?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #2052      +/-   ##
==========================================
- Coverage   80.64%   80.40%   -0.24%     
==========================================
  Files          71       71              
  Lines        8519     8519              
==========================================
- Hits         6870     6850      -20     
- Misses       1649     1669      +20     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/huggingface/huggingface_hub/pull/2052?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).


Patch:
diff --git a/src/huggingface_hub/file_download.py b/src/huggingface_hub/file_download.py
index 3083518f..f8f79456 100644
--- a/src/huggingface_hub/file_download.py
+++ b/src/huggingface_hub/file_download.py
@@ -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.
@@ -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)
diff --git a/src/huggingface_hub/utils/_fixes.py b/src/huggingface_hub/utils/_fixes.py
index ff4f9e2d..d8df2057 100644
--- a/src/huggingface_hub/utils/_fixes.py
+++ b/src/huggingface_hub/utils/_fixes.py
@@ -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]:
     """
     Context manager to create a temporary directory and safely delete it.
 
@@ -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


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 26a3e695..9e71e8ee 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -1,6 +1,5 @@
 import os
 import shutil
-from pathlib import Path
 from typing import Generator
 
 import pytest
@@ -30,7 +29,7 @@ def fx_cache_dir(request: SubRequest) -> Generator[None, None, 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/.
diff --git a/tests/test_cli.py b/tests/test_cli.py
index bcaf68eb..b0aa4301 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -216,9 +216,10 @@ class TestUploadCommand(unittest.TestCase):
     @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()
@@ -227,7 +228,7 @@ class TestUploadCommand(unittest.TestCase):
                 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",
diff --git a/tests/test_utils_fixes.py b/tests/test_utils_fixes.py
index 582962ae..0501421d 100644
--- a/tests/test_utils_fixes.py
+++ b/tests/test_utils_fixes.py
@@ -17,9 +17,8 @@ class TestYamlDump(unittest.TestCase):
 
 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())