Repository: huggingface/huggingface_hub
Pull request #1109 (2022-10-12 10:19:56)

More robust create relative symlink in cache
Should fix https://github.com/huggingface/huggingface_hub/issues/1108.

HuggingFaceDocBuilderDev: The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_1109). All of your documentation changes will be reflected on that endpoint.
codecov[bot]: # [Codecov](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) Report
Base: **57.23**% // Head: **84.91**% // Increases project coverage by **`+27.68%`** :tada:
> Coverage data is based on head [(`494c417`)](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) compared to base [(`6415cd1`)](https://codecov.io/gh/huggingface/huggingface_hub/commit/6415cd10b967b594b8a7c9b7e3894e18573dbe73?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).
> Patch coverage: 100.00% of modified lines in pull request are covered.

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


```diff
@@             Coverage Diff             @@
##             main    #1109       +/-   ##
===========================================
+ Coverage   57.23%   84.91%   +27.68%     
===========================================
  Files          38       38               
  Lines        3926     3931        +5     
===========================================
+ Hits         2247     3338     +1091     
+ Misses       1679      593     -1086     
```


| [Impacted Files](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) | Coverage Δ | |
|---|---|---|
| [src/huggingface\_hub/file\_download.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9maWxlX2Rvd25sb2FkLnB5) | `87.57% <100.00%> (+27.64%)` | :arrow_up: |
| [src/huggingface\_hub/repository.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9yZXBvc2l0b3J5LnB5) | `79.72% <0.00%> (+0.86%)` | :arrow_up: |
| [src/huggingface\_hub/\_commit\_api.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9fY29tbWl0X2FwaS5weQ==) | `92.10% <0.00%> (+3.50%)` | :arrow_up: |
| [src/huggingface\_hub/utils/\_subprocess.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi91dGlscy9fc3VicHJvY2Vzcy5weQ==) | `100.00% <0.00%> (+11.11%)` | :arrow_up: |
| [src/huggingface\_hub/utils/\_hf\_folder.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi91dGlscy9faGZfZm9sZGVyLnB5) | `100.00% <0.00%> (+11.53%)` | :arrow_up: |
| [src/huggingface\_hub/utils/\_deprecation.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi91dGlscy9fZGVwcmVjYXRpb24ucHk=) | `100.00% <0.00%> (+12.28%)` | :arrow_up: |
| [src/huggingface\_hub/commands/user.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9jb21tYW5kcy91c2VyLnB5) | `33.75% <0.00%> (+12.50%)` | :arrow_up: |
| [src/huggingface\_hub/utils/\_validators.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi91dGlscy9fdmFsaWRhdG9ycy5weQ==) | `100.00% <0.00%> (+17.24%)` | :arrow_up: |
| [src/huggingface\_hub/community.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi9jb21tdW5pdHkucHk=) | `91.35% <0.00%> (+17.28%)` | :arrow_up: |
| [src/huggingface\_hub/utils/\_headers.py](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface#diff-c3JjL2h1Z2dpbmdmYWNlX2h1Yi91dGlscy9faGVhZGVycy5weQ==) | `100.00% <0.00%> (+18.00%)` | :arrow_up: |
| ... and [16 more](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/huggingface/huggingface_hub/pull/1109?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=huggingface).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](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 f8b73f0c..21c43303 100644
--- a/src/huggingface_hub/file_download.py
+++ b/src/huggingface_hub/file_download.py
@@ -797,7 +797,6 @@ def _create_relative_symlink(src: str, dst: str, new_blob: bool = False) -> None
     when loading `huggingface_hub`. The warning message can be disable with the
     `DISABLE_SYMLINKS_WARNING` environment variable.
     """
-    relative_src = os.path.relpath(src, start=os.path.dirname(dst))
     try:
         os.remove(dst)
     except OSError:
@@ -805,7 +804,20 @@ def _create_relative_symlink(src: str, dst: str, new_blob: bool = False) -> None
 
     cache_dir = os.path.dirname(os.path.commonpath([src, dst]))
     if are_symlinks_supported(cache_dir=cache_dir):
-        os.symlink(relative_src, dst)
+        relative_src = os.path.relpath(src, start=os.path.dirname(dst))
+        try:
+            os.symlink(relative_src, dst)
+        except FileExistsError:
+            if os.path.islink(dst) and os.path.realpath(dst) == os.path.realpath(src):
+                # `dst` already exists and is a symlink to the `src` blob. It is most
+                # likely that the file has been cached twice concurrently (exactly
+                # between `os.remove` and `os.symlink`). Do nothing.
+                pass
+            else:
+                # Very unlikely to happen. Means a file `dst` has been created exactly
+                # between `os.remove` and `os.symlink` and is not a symlink to the `src`
+                # blob file. Raise exception.
+                raise
     elif new_blob:
         os.replace(src, dst)
     else:


Test patch:
diff --git a/tests/test_file_download.py b/tests/test_file_download.py
index 7774dd27..83f84c5b 100644
--- a/tests/test_file_download.py
+++ b/tests/test_file_download.py
@@ -16,6 +16,7 @@ import re
 import unittest
 from pathlib import Path
 from tempfile import TemporaryDirectory
+from unittest.mock import Mock, patch
 
 import pytest
 
@@ -26,6 +27,7 @@ from huggingface_hub.constants import (
 )
 from huggingface_hub.file_download import (
     _CACHED_NO_EXIST,
+    _create_relative_symlink,
     cached_download,
     filename_to_url,
     get_hf_file_metadata,
@@ -370,3 +372,38 @@ class CachedDownloadTests(unittest.TestCase):
             metadata.location,
             url.replace(DUMMY_RENAMED_OLD_MODEL_ID, DUMMY_RENAMED_NEW_MODEL_ID),
         )
+
+
+class CreateSymlinkTest(unittest.TestCase):
+    @patch("huggingface_hub.file_download.are_symlinks_supported")
+    def test_create_relative_symlink_concurrent_access(
+        self, mock_are_symlinks_supported: Mock
+    ) -> None:
+        with TemporaryDirectory() as tmpdir:
+            src = os.path.join(tmpdir, "source")
+            other = os.path.join(tmpdir, "other")
+            dst = os.path.join(tmpdir, "destination")
+
+            # Normal case: symlink does not exist
+            mock_are_symlinks_supported.return_value = True
+            _create_relative_symlink(src, dst)
+            self.assertEqual(os.path.realpath(dst), os.path.realpath(src))
+
+            # Symlink already exists when it tries to create it (most probably from a
+            # concurrent access) but do not raise exception
+            def _are_symlinks_supported(cache_dir: str) -> bool:
+                os.symlink(src, dst)
+                return True
+
+            mock_are_symlinks_supported.side_effect = _are_symlinks_supported
+            _create_relative_symlink(src, dst)
+
+            # Symlink already exists but pointing to a different source file. This should
+            # never happen in the context of HF cache system -> raise exception
+            def _are_symlinks_supported(cache_dir: str) -> bool:
+                os.symlink(other, dst)
+                return True
+
+            mock_are_symlinks_supported.side_effect = _are_symlinks_supported
+            with self.assertRaises(FileExistsError):
+                _create_relative_symlink(src, dst)