Repository: huggingface/huggingface_hub
Pull request #1886 (2023-12-05 10:09:49)

Fix PermissionError between volumes
Fix https://github.com/huggingface/huggingface_hub/issues/1882 cc @vince62s

If 2 different volumes for cache and local dir, and symlinks are support on target dir BUT not between volumes, then copy or move the file.

(**EDIT:** failing tests are unrelated)


Patch:
diff --git a/.github/workflows/delete_doc_comment.yaml b/.github/workflows/delete_doc_comment.yaml
deleted file mode 100644
index 72801c85..00000000
--- a/.github/workflows/delete_doc_comment.yaml
+++ /dev/null
@@ -1,13 +0,0 @@
-name: Delete doc comment
-
-on:
-  workflow_run:
-    workflows: ["Delete doc comment trigger"]
-    types:
-      - completed
-
-jobs:
-  delete:
-    uses: huggingface/doc-builder/.github/workflows/delete_doc_comment.yml@main
-    secrets:
-      comment_bot_token: ${{ secrets.COMMENT_BOT_TOKEN }}
\ No newline at end of file
diff --git a/.github/workflows/delete_doc_comment_trigger.yaml b/.github/workflows/delete_doc_comment_trigger.yaml
deleted file mode 100644
index 5e39e253..00000000
--- a/.github/workflows/delete_doc_comment_trigger.yaml
+++ /dev/null
@@ -1,12 +0,0 @@
-name: Delete doc comment trigger
-
-on:
-  pull_request:
-    types: [ closed ]
-
-
-jobs:
-  delete:
-    uses: huggingface/doc-builder/.github/workflows/delete_doc_comment_trigger.yml@main
-    with:
-      pr_number: ${{ github.event.number }}
\ No newline at end of file
diff --git a/src/huggingface_hub/file_download.py b/src/huggingface_hub/file_download.py
index 95c8beb9..2024cb62 100644
--- a/src/huggingface_hub/file_download.py
+++ b/src/huggingface_hub/file_download.py
@@ -927,26 +927,34 @@ def _create_symlink(src: str, dst: str, new_blob: bool = False) -> None:
         # by the user via `local_dir`. Let's test symlink support there)
         _support_symlinks = are_symlinks_supported(abs_dst_folder)
 
+    # Symlinks are supported => let's create a symlink.
     if _support_symlinks:
         src_rel_or_abs = relative_src or abs_src
         logger.debug(f"Creating pointer from {src_rel_or_abs} to {abs_dst}")
         try:
             os.symlink(src_rel_or_abs, abs_dst)
+            return
         except FileExistsError:
             if os.path.islink(abs_dst) and os.path.realpath(abs_dst) == os.path.realpath(abs_src):
                 # `abs_dst` already exists and is a symlink to the `abs_src` blob. It is most likely that the file has
                 # been cached twice concurrently (exactly between `os.remove` and `os.symlink`). Do nothing.
-                pass
+                return
             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 `abs_src` blob file. Raise exception.
                 raise
-    elif new_blob:
+        except PermissionError:
+            # Permission error means src and dst are not in the same volume (e.g. download to local dir) and symlink
+            # is supported on both volumes but not between them. Let's just make a hard copy in that case.
+            pass
+
+    # Symlinks are not supported => let's move or copy the file.
+    if new_blob:
         logger.info(f"Symlink not supported. Moving file from {abs_src} to {abs_dst}")
-        shutil.move(src, dst)
+        shutil.move(abs_src, abs_dst)
     else:
         logger.info(f"Symlink not supported. Copying file from {abs_src} to {abs_dst}")
-        shutil.copyfile(src, dst)
+        shutil.copyfile(abs_src, abs_dst)
 
 
 def _cache_commit_hash_for_specific_revision(storage_folder: str, revision: str, commit_hash: str) -> None:
diff --git a/src/huggingface_hub/hf_file_system.py b/src/huggingface_hub/hf_file_system.py
index 76668048..89c07fb9 100644
--- a/src/huggingface_hub/hf_file_system.py
+++ b/src/huggingface_hub/hf_file_system.py
@@ -1,8 +1,9 @@
+import copy
 import os
 import re
 import tempfile
 from collections import deque
-from dataclasses import dataclass
+from dataclasses import dataclass, field
 from datetime import datetime
 from itertools import chain
 from typing import Any, Dict, List, NoReturn, Optional, Tuple, Union
@@ -27,7 +28,7 @@ from .utils import (
 # Regex used to match special revisions with "/" in them (see #1710)
 SPECIAL_REFS_REVISION_REGEX = re.compile(
     r"""
-    (^refs\/convert\/parquet) # `refs/convert/parquet` revisions
+    (^refs\/convert\/\w+)     # `refs/convert/parquet` revisions
     |
     (^refs\/pr\/\d+)          # PR revisions
     """,
@@ -43,10 +44,18 @@ class HfFileSystemResolvedPath:
     repo_id: str
     revision: str
     path_in_repo: str
+    # The part placed after '@' in the initial path. It can be a quoted or unquoted refs revision.
+    # Used to reconstruct the unresolved path to return to the user.
+    _raw_revision: Optional[str] = field(default=None, repr=False)
 
     def unresolve(self) -> str:
         repo_path = REPO_TYPES_URL_PREFIXES.get(self.repo_type, "") + self.repo_id
-        return f"{repo_path}@{safe_revision(self.revision)}/{self.path_in_repo}".rstrip("/")
+        if self._raw_revision:
+            return f"{repo_path}@{self._raw_revision}/{self.path_in_repo}".rstrip("/")
+        elif self.revision != DEFAULT_REVISION:
+            return f"{repo_path}@{safe_revision(self.revision)}/{self.path_in_repo}".rstrip("/")
+        else:
+            return f"{repo_path}/{self.path_in_repo}".rstrip("/")
 
 
 class HfFileSystem(fsspec.AbstractFileSystem):
@@ -155,12 +164,12 @@ class HfFileSystem(fsspec.AbstractFileSystem):
                         revision_in_path, path_in_repo = revision_in_path.split("/", 1)
                 else:
                     path_in_repo = ""
-                revision_in_path = unquote(revision_in_path)
-                revision = _align_revision_in_path_with_revision(revision_in_path, revision)
+                revision = _align_revision_in_path_with_revision(unquote(revision_in_path), revision)
                 repo_and_revision_exist, err = self._repo_and_revision_exist(repo_type, repo_id, revision)
                 if not repo_and_revision_exist:
                     _raise_file_not_found(path, err)
             else:
+                revision_in_path = None
                 repo_id_with_namespace = "/".join(path.split("/")[:2])
                 path_in_repo_with_namespace = "/".join(path.split("/")[2:])
                 repo_id_without_namespace = path.split("/")[0]
@@ -182,19 +191,20 @@ class HfFileSystem(fsspec.AbstractFileSystem):
             path_in_repo = ""
             if "@" in path:
                 repo_id, revision_in_path = path.split("@", 1)
-                revision_in_path = unquote(revision_in_path)
-                revision = _align_revision_in_path_with_revision(revision_in_path, revision)
+                revision = _align_revision_in_path_with_revision(unquote(revision_in_path), revision)
+            else:
+                revision_in_path = None
             repo_and_revision_exist, _ = self._repo_and_revision_exist(repo_type, repo_id, revision)
             if not repo_and_revision_exist:
                 raise NotImplementedError("Access to repositories lists is not implemented.")
 
         revision = revision if revision is not None else DEFAULT_REVISION
-        return HfFileSystemResolvedPath(repo_type, repo_id, revision, path_in_repo)
+        return HfFileSystemResolvedPath(repo_type, repo_id, revision, path_in_repo, _raw_revision=revision_in_path)
 
     def invalidate_cache(self, path: Optional[str] = None) -> None:
         if not path:
             self.dircache.clear()
-            self._repository_type_and_id_exists_cache.clear()
+            self._repo_and_revision_exists_cache.clear()
         else:
             path = self.resolve_path(path).unresolve()
             while path:
@@ -235,7 +245,7 @@ class HfFileSystem(fsspec.AbstractFileSystem):
     ) -> None:
         resolved_path = self.resolve_path(path, revision=revision)
         root_path = REPO_TYPES_URL_PREFIXES.get(resolved_path.repo_type, "") + resolved_path.repo_id
-        paths = self.expand_path(path, recursive=recursive, maxdepth=maxdepth, revision=resolved_path.revision)
+        paths = self.expand_path(path, recursive=recursive, maxdepth=maxdepth, revision=revision)
         paths_in_repo = [path[len(root_path) + 1 :] for path in paths if not self.isdir(path)]
         operations = [CommitOperationDelete(path_in_repo=path_in_repo) for path_in_repo in paths_in_repo]
         commit_message = f"Delete {path} "
@@ -258,22 +268,18 @@ class HfFileSystem(fsspec.AbstractFileSystem):
     ) -> List[Union[str, Dict[str, Any]]]:
         """List the contents of a directory."""
         resolved_path = self.resolve_path(path, revision=revision)
-        revision_in_path = "@" + safe_revision(resolved_path.revision)
-        has_revision_in_path = revision_in_path in path
         path = resolved_path.unresolve()
         kwargs = {"expand_info": detail, **kwargs}
         try:
-            out = self._ls_tree(path, refresh=refresh, revision=resolved_path.revision, **kwargs)
+            out = self._ls_tree(path, refresh=refresh, revision=revision, **kwargs)
         except EntryNotFoundError:
             # Path could be a file
             if not resolved_path.path_in_repo:
                 _raise_file_not_found(path, None)
-            out = self._ls_tree(self._parent(path), refresh=refresh, revision=resolved_path.revision, **kwargs)
+            out = self._ls_tree(self._parent(path), refresh=refresh, revision=revision, **kwargs)
             out = [o for o in out if o["name"] == path]
             if len(out) == 0:
                 _raise_file_not_found(path, None)
-        if not has_revision_in_path:
-            out = [{**o, "name": o["name"].replace(revision_in_path, "", 1)} for o in out]
         return out if detail else [o["name"] for o in out]
 
     def _ls_tree(
@@ -287,7 +293,11 @@ class HfFileSystem(fsspec.AbstractFileSystem):
         resolved_path = self.resolve_path(path, revision=revision)
         path = resolved_path.unresolve()
         root_path = HfFileSystemResolvedPath(
-            resolved_path.repo_type, resolved_path.repo_id, resolved_path.revision, ""
+            resolved_path.repo_type,
+            resolved_path.repo_id,
+            resolved_path.revision,
+            path_in_repo="",
+            _raw_revision=resolved_path._raw_revision,
         ).unresolve()
 
         out = []
@@ -339,7 +349,7 @@ class HfFileSystem(fsspec.AbstractFileSystem):
                         common_path,
                         recursive=recursive,
                         refresh=True,
-                        revision=resolved_path.revision,
+                        revision=revision,
                         expand_info=expand_info,
                     )
                 )
@@ -374,12 +384,12 @@ class HfFileSystem(fsspec.AbstractFileSystem):
                 parent_path = self._parent(cache_path_info["name"])
                 self.dircache.setdefault(parent_path, []).append(cache_path_info)
                 out.append(cache_path_info)
-        return out
+        return copy.deepcopy(out)  # copy to not let users modify the dircache
 
     def glob(self, path, **kwargs):
         # Set expand_info=False by default to get a x10 speed boost
         kwargs = {"expand_info": kwargs.get("detail", False), **kwargs}
-        path = self.resolve_path(path).unresolve()
+        path = self.resolve_path(path, revision=kwargs.get("revision")).unresolve()
         return super().glob(path, **kwargs)
 
     def find(
@@ -397,16 +407,13 @@ class HfFileSystem(fsspec.AbstractFileSystem):
                 path, maxdepth=maxdepth, withdirs=withdirs, detail=detail, refresh=refresh, revision=revision, **kwargs
             )
         resolved_path = self.resolve_path(path, revision=revision)
-        revision_in_path = "@" + safe_quote(resolved_path.revision)
-        has_revision_in_path = revision_in_path in path
         path = resolved_path.unresolve()
         kwargs = {"expand_info": detail, **kwargs}
         try:
             out = self._ls_tree(path, recursive=True, refresh=refresh, revision=resolved_path.revision, **kwargs)
         except EntryNotFoundError:
             # Path could be a file
-            if self.info(path, revision=resolved_path.revision, **kwargs)["type"] == "file":
-                path = path.replace(revision_in_path, "", 1) if not has_revision_in_path else path
+            if self.info(path, revision=revision, **kwargs)["type"] == "file":
                 out = {path: {}}
             else:
                 out = {}
@@ -417,8 +424,6 @@ class HfFileSystem(fsspec.AbstractFileSystem):
                 # If `withdirs=True`, include the directory itself to be consistent with the spec
                 path_info = self.info(path, revision=resolved_path.revision, **kwargs)
                 out = [path_info] + out if path_info["type"] == "directory" else out
-            if not has_revision_in_path:
-                out = [{**o, "name": o["name"].replace(revision_in_path, "", 1)} for o in out]
             out = {o["name"]: o for o in out}
         names = sorted(out)
         if not detail:
@@ -473,8 +478,6 @@ class HfFileSystem(fsspec.AbstractFileSystem):
 
     def info(self, path: str, refresh: bool = False, revision: Optional[str] = None, **kwargs) -> Dict[str, Any]:
         resolved_path = self.resolve_path(path, revision=revision)
-        revision_in_path = "@" + safe_revision(resolved_path.revision)
-        has_revision_in_path = revision_in_path in path
         path = resolved_path.unresolve()
         expand_info = kwargs.get(
             "expand_info", True
@@ -518,7 +521,11 @@ class HfFileSystem(fsspec.AbstractFileSystem):
                     _raise_file_not_found(path, None)
                 path_info = paths_info[0]
                 root_path = HfFileSystemResolvedPath(
-                    resolved_path.repo_type, resolved_path.repo_id, resolved_path.revision, ""
+                    resolved_path.repo_type,
+                    resolved_path.repo_id,
+                    resolved_path.revision,
+                    path_in_repo="",
+                    _raw_revision=resolved_path._raw_revision,
                 ).unresolve()
                 if isinstance(path_info, RepoFile):
                     out = {
@@ -541,9 +548,7 @@ class HfFileSystem(fsspec.AbstractFileSystem):
                 if not expand_info:
                     out = {k: out[k] for k in ["name", "size", "type"]}
         assert out is not None
-        if not has_revision_in_path:
-            out["name"] = out["name"].replace(revision_in_path, "", 1)  # type: ignore
-        return out
+        return copy.deepcopy(out)  # copy to not let users modify the dircache
 
     def exists(self, path, **kwargs):
         """Is there a file at the given path"""


Test patch:
diff --git a/tests/test_hf_file_system.py b/tests/test_hf_file_system.py
index 8ad25e09..795b5ebd 100644
--- a/tests/test_hf_file_system.py
+++ b/tests/test_hf_file_system.py
@@ -93,10 +93,34 @@ class HfFileSystemTests(unittest.TestCase):
             sorted(self.hffs.glob(self.hf_path + "@main" + "/*")),
             sorted([self.hf_path + "@main" + "/.gitattributes", self.hf_path + "@main" + "/data"]),
         )
+        self.assertEqual(
+            self.hffs.glob(self.hf_path + "@refs%2Fpr%2F1" + "/data/*"),
+            [self.hf_path + "@refs%2Fpr%2F1" + "/data/binary_data_for_pr.bin"],
+        )
+        self.assertEqual(
+            self.hffs.glob(self.hf_path + "@refs/pr/1" + "/data/*"),
+            [self.hf_path + "@refs/pr/1" + "/data/binary_data_for_pr.bin"],
+        )
+        self.assertEqual(
+            self.hffs.glob(self.hf_path + "/data/*", revision="refs/pr/1"),
+            [self.hf_path + "@refs/pr/1" + "/data/binary_data_for_pr.bin"],
+        )
+
         self.assertIsNone(
             self.hffs.dircache[self.hf_path + "@main"][0]["last_commit"]
         )  # no detail -> no last_commit in cache
 
+        files = self.hffs.glob(self.hf_path + "@main" + "/*", detail=True, expand_info=False)
+        self.assertIsInstance(files, dict)
+        self.assertEqual(len(files), 2)
+        keys = sorted(files)
+        self.assertTrue(
+            files[keys[0]]["name"].endswith("/.gitattributes") and files[keys[1]]["name"].endswith("/data")
+        )
+        self.assertIsNone(
+            self.hffs.dircache[self.hf_path + "@main"][0]["last_commit"]
+        )  # detail but no expand info -> no last_commit in cache
+
         files = self.hffs.glob(self.hf_path + "@main" + "/*", detail=True)
         self.assertIsInstance(files, dict)
         self.assertEqual(len(files), 2)
@@ -247,27 +271,32 @@ class HfFileSystemTests(unittest.TestCase):
         files = self.hffs.ls(self.hf_path + "@refs%2Fpr%2F1" + "/data")
 
         for test_name, files in {
-            "rev_in_path": self.hffs.ls(self.hf_path + "@refs%2Fpr%2F1" + "/data"),
+            "quoted_rev_in_path": self.hffs.ls(self.hf_path + "@refs%2Fpr%2F1" + "/data"),
+            "rev_in_path": self.hffs.ls(self.hf_path + "@refs/pr/1" + "/data"),
             "rev_as_arg": self.hffs.ls(self.hf_path + "/data", revision="refs/pr/1"),
-            "rev_in_path_and_as_arg": self.hffs.ls(self.hf_path + "@refs%2Fpr%2F1" + "/data", revision="refs/pr/1"),
+            "quoted_rev_in_path_and_rev_as_arg": self.hffs.ls(
+                self.hf_path + "@refs%2Fpr%2F1" + "/data", revision="refs/pr/1"
+            ),
         }.items():
             with self.subTest(test_name):
                 self.assertEqual(len(files), 1)  # only one file in PR
                 self.assertEqual(files[0]["type"], "file")
                 self.assertTrue(files[0]["name"].endswith("/data/binary_data_for_pr.bin"))  # PR file
+                if "quoted_rev_in_path" in test_name:
+                    self.assertIn("@refs%2Fpr%2F1", files[0]["name"])
+                elif "rev_in_path" in test_name:
+                    self.assertIn("@refs/pr/1", files[0]["name"])
 
     def test_list_root_directory_no_revision_no_detail_then_with_detail(self):
         files = self.hffs.ls(self.hf_path, detail=False)
         self.assertEqual(len(files), 2)
         self.assertTrue(files[0].endswith("/data") and files[1].endswith("/.gitattributes"))
-        self.assertIsNone(
-            self.hffs.dircache[self.hf_path + "@main"][0]["last_commit"]
-        )  # no detail -> no last_commit in cache
+        self.assertIsNone(self.hffs.dircache[self.hf_path][0]["last_commit"])  # no detail -> no last_commit in cache
 
         files = self.hffs.ls(self.hf_path, detail=True)
         self.assertEqual(len(files), 2)
         self.assertTrue(files[0]["name"].endswith("/data") and files[1]["name"].endswith("/.gitattributes"))
-        self.assertIsNotNone(self.hffs.dircache[self.hf_path + "@main"][0]["last_commit"])
+        self.assertIsNotNone(self.hffs.dircache[self.hf_path][0]["last_commit"])
 
     def test_find_root_directory_no_revision(self):
         files = self.hffs.find(self.hf_path, detail=False)
@@ -313,11 +342,11 @@ class HfFileSystemTests(unittest.TestCase):
         files = self.hffs.find(self.hf_path, detail=True)
 
         # some directories not in cache
-        self.hffs.dircache.pop(self.hf_path + "@main/data/sub_data")
+        self.hffs.dircache.pop(self.hf_path + "/data/sub_data")
         # some files not expanded
-        self.hffs.dircache[self.hf_path + "@main/data"][1]["last_commit"] = None
-
-        self.assertEqual(self.hffs.find(self.hf_path, detail=True), files)
+        self.hffs.dircache[self.hf_path + "/data"][1]["last_commit"] = None
+        out = self.hffs.find(self.hf_path, detail=True)
+        self.assertEqual(out, files)
 
     def test_find_data_file_no_revision(self):
         files = self.hffs.find(self.hf_path + "/data/text_data.txt", detail=False)
@@ -360,8 +389,10 @@ class HfFileSystemTests(unittest.TestCase):
             "refs/convert/parquet",
         ),
         ("gpt2@refs/pr/2", None, "model", "gpt2", "refs/pr/2"),
+        ("gpt2@refs%2Fpr%2F2", None, "model", "gpt2", "refs/pr/2"),
         ("hf://username/my_model@refs/pr/10", None, "model", "username/my_model", "refs/pr/10"),
         ("hf://username/my_model@refs/pr/10", "refs/pr/10", "model", "username/my_model", "refs/pr/10"),
+        ("hf://username/my_model@refs%2Fpr%2F10", "refs/pr/10", "model", "username/my_model", "refs/pr/10"),
     ],
 )
 def test_resolve_path(
@@ -383,6 +414,8 @@ def test_resolve_path(
             resolved_path.revision,
             resolved_path.path_in_repo,
         ) == (repo_type, repo_id, resolved_revision, path_in_repo)
+        if "@" in path:
+            assert resolved_path._raw_revision in path
 
 
 @pytest.mark.parametrize("path_in_repo", ["", "file.txt", "path/to/file"])