[HfFileSystem] Support quoted revisions in path #1888

lhoestq · 2023-12-05T14:51:26Z

Added support quoted revisions in paths in HfFileSystem, and return paths formatted the same way.

This potentially supersedes #1889 (I find it simpler)

Now this code works (otherwise returns empty directory for quoted revision)

from huggingface_hub import HfFileSystem

fs = HfFileSystem()
path = "hf://datasets/squad/*"
print(fs.glob(path))
path = "hf://datasets/squad@refs%2Fconvert%2Fparquet/plain_text/*"
print(fs.glob(path))
path = "hf://datasets/squad@refs/convert/parquet/plain_text/*"
print(fs.glob(path))
path = "hf://datasets/squad/plain_text/*"
print(fs.glob(path, revision="refs/convert/parquet"))

prints

['datasets/squad/.gitattributes', 'datasets/squad/README.md', 'datasets/squad/dataset_infos.json', 'datasets/squad/squad.py']
['datasets/squad@refs%2Fconvert%2Fparquet/plain_text/train', 'datasets/squad@refs%2Fconvert%2Fparquet/plain_text/validation']
['datasets/squad@refs/convert/parquet/plain_text/train', 'datasets/squad@refs/convert/parquet/plain_text/validation']
['datasets/squad@refs/convert/parquet/plain_text/train', 'datasets/squad@refs/convert/parquet/plain_text/validation']

One consequence is that revisions are not unquoted in the returned paths, but I feel it is the expected behavior to return paths in the same format (except if revision is overridden explicitly using a keyword argument)

HuggingFaceDocBuilderDev · 2023-12-05T14:56:36Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

lhoestq · 2023-12-05T16:07:57Z

src/huggingface_hub/hf_file_system.py

@@ -27,7 +27,7 @@
 # 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


(kinda unrelated change)

just in case we add other convert branches 🙃

(we're currently discussing moving the duckdb indexes - could be in the parquet branch but named differently or in another convert/ branch)

lhoestq · 2023-12-05T16:09:11Z

src/huggingface_hub/hf_file_system.py

@@ -379,7 +387,7 @@ def _ls_tree(
    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()


this ensures all the returned paths will start with path (glob filters out paths that don't start with the input path)

mariosasko

LGTM! Two nits:

mariosasko · 2023-12-05T16:37:46Z

src/huggingface_hub/hf_file_system.py

@@ -43,10 +43,17 @@ class HfFileSystemResolvedPath:
    repo_id: str
    revision: str
    path_in_repo: str
+    # The part placer after '@' in the path (can even be a quoted or unquoted special refs revision)
+    _revision_in_path: Optional[str] = None


Suggested change

_revision_in_path: Optional[str] = None

_revision_in_path: Optional[str] = filed(repr=False, default=None)

yes it was fixed to "field" ;)

mariosasko · 2023-12-05T16:41:26Z

tests/test_hf_file_system.py

@@ -310,14 +340,14 @@ def test_find_root_directory_no_revision_with_incomplete_cache(self):
            repo_type="dataset",
        )

-        files = self.hffs.find(self.hf_path, detail=True)
+        files = copy.deepcopy(self.hffs.find(self.hf_path, detail=True))


Why do we need copy.deepcopy here?

In the next lines we .pop() the cache, which also modifies the files dictionary and make the test fail - maybe I can fix find to not have to do that

Wauplin

Thanks for the PR @lhoestq! As discussed on slack, this is indeed the way it should be to ensure robustness. I left a couple of comments straightforward to address otherwise looks good to me.

Wauplin · 2023-12-05T16:40:20Z

src/huggingface_hub/hf_file_system.py

+    # The part placer after '@' in the path (can even be a quoted or unquoted special refs revision)
+    _revision_in_path: Optional[str] = None


I would rename the attribute if that's fine with you to really emphasize it is the revision as provided by the user. I found revision_in_path to be slightly vague. I suggested _raw_revision but happy for other suggestions.

(would have to be updated in the PR)

Suggested change

# The part placer after '@' in the path (can even be a quoted or unquoted special refs revision)

_revision_in_path: Optional[str] = None

# 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] = None

Wauplin · 2023-12-05T16:41:01Z

src/huggingface_hub/hf_file_system.py

-        return f"{repo_path}@{safe_revision(self.revision)}/{self.path_in_repo}".rstrip("/")
+        if self._revision_in_path:
+            return f"{repo_path}@{self._revision_in_path}/{self.path_in_repo}".rstrip("/")
+        elif self.revision != "main":


(nit)

Suggested change

elif self.revision != "main":

elif self.revision != DEFAULT_REVISION:

Wauplin · 2023-12-05T16:42:41Z

src/huggingface_hub/hf_file_system.py

+            resolved_path.repo_type,
+            resolved_path.repo_id,
+            resolved_path.revision,
+            "",


(nit) I know it's not related to your PR but let's be explicit about what the empty string means

Suggested change

"",

path_in_repo="",

Wauplin · 2023-12-05T16:44:08Z

src/huggingface_hub/hf_file_system.py

+                    resolved_path.repo_type,
+                    resolved_path.repo_id,
+                    resolved_path.revision,
+                    "",


(nit) same here

Suggested change

"",

path_in_repo="",

Wauplin · 2023-12-05T16:47:36Z

tests/test_hf_file_system.py

@@ -310,14 +340,14 @@ def test_find_root_directory_no_revision_with_incomplete_cache(self):
            repo_type="dataset",
        )

-        files = self.hffs.find(self.hf_path, detail=True)
+        files = copy.deepcopy(self.hffs.find(self.hf_path, detail=True))


Why do we need a deep copy? Is it because files is a reference to an internal object of self.hffs.find? If that's the case it would be a bit worrying 😕

Maybe I can fix find to return a copy of the cache instead of the cache content itself (that users could modify)

lhoestq · 2023-12-06T11:47:46Z

I ended up moving the copy.deepcopy to the return statements of HfFileSystem._ls_tree and HfFileSystem.info (where we use the dircache) - let me know if it sounds good to you and I can merge.

I also renamed the HfFileSystemResolvedPath attribute _raw_revision

Wauplin

Thanks for making the changes @lhoestq!

(failing tests are unrelated so feel free to merge)

lhoestq added 2 commits December 5, 2023 15:49

support quoted revisions

50e1a36

style

b89ef07

lhoestq changed the title ~~Support quoted revisions~~ [HfFileSystem] Support quoted revisions Dec 5, 2023

lhoestq mentioned this pull request Dec 5, 2023

[HfFileSystem] Sanitize path decorator #1889

Closed

lhoestq changed the title ~~[HfFileSystem] Support quoted revisions~~ [HfFileSystem] Support quoted revisions in path Dec 5, 2023

tests

f9487d4

lhoestq marked this pull request as ready for review December 5, 2023 15:56

lhoestq commented Dec 5, 2023

View reviewed changes

lhoestq requested review from Wauplin and mariosasko December 5, 2023 16:09

mariosasko approved these changes Dec 5, 2023

View reviewed changes

Wauplin reviewed Dec 5, 2023

View reviewed changes

lhoestq added 4 commits December 6, 2023 11:55

return a copy of the cache content

a1336a4

fix revision in path in info()

99b7c4c

rename _raw_revision

d6e0155

fix test

aba76d5

Wauplin approved these changes Dec 6, 2023

View reviewed changes

lhoestq merged commit 082c1ed into main Dec 6, 2023
13 of 16 checks passed

lhoestq deleted the support-quoted-revisions branch December 6, 2023 12:00

mariosasko mentioned this pull request Apr 8, 2024

fix: performance of _ls_tree #2103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HfFileSystem] Support quoted revisions in path #1888

[HfFileSystem] Support quoted revisions in path #1888

lhoestq commented Dec 5, 2023 •

edited

Loading

HuggingFaceDocBuilderDev commented Dec 5, 2023

lhoestq Dec 5, 2023 •

edited

Loading

lhoestq Dec 5, 2023

mariosasko left a comment

mariosasko Dec 5, 2023

severo Dec 11, 2023

lhoestq Dec 11, 2023

mariosasko Dec 5, 2023

lhoestq Dec 5, 2023 •

edited

Loading

Wauplin left a comment

Wauplin Dec 5, 2023

Wauplin Dec 5, 2023

Wauplin Dec 5, 2023

Wauplin Dec 5, 2023

Wauplin Dec 5, 2023

lhoestq Dec 5, 2023

lhoestq commented Dec 6, 2023

Wauplin left a comment •

edited

Loading

	_revision_in_path: Optional[str] = None
	_revision_in_path: Optional[str] = filed(repr=False, default=None)

		# The part placer after '@' in the path (can even be a quoted or unquoted special refs revision)
		_revision_in_path: Optional[str] = None

-    # The part placer after '@' in the path (can even be a quoted or unquoted special refs revision)
-    _revision_in_path: Optional[str] = None
+    # 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] = None

	elif self.revision != "main":
	elif self.revision != DEFAULT_REVISION:

[HfFileSystem] Support quoted revisions in path #1888

[HfFileSystem] Support quoted revisions in path #1888

Conversation

lhoestq commented Dec 5, 2023 • edited Loading

HuggingFaceDocBuilderDev commented Dec 5, 2023

lhoestq Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariosasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhoestq Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Wauplin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhoestq commented Dec 6, 2023

Wauplin left a comment • edited Loading

Choose a reason for hiding this comment

lhoestq commented Dec 5, 2023 •

edited

Loading

lhoestq Dec 5, 2023 •

edited

Loading

lhoestq Dec 5, 2023 •

edited

Loading

Wauplin left a comment •

edited

Loading