Repository: huggingface/huggingface_hub
Pull request #1316 (2023-01-31 09:59:15)

fix(utils): try_to_load_from_cache now correctly handles refs/pr revisions
When trying to load a file from cache with a revision of type `refs/pr/<pr-number>`, `try_to_load_from_cache` fails because the revision file is serialized to: `HUGGINGFACE_HUB_CACHE/models--<object-id>/refs/refs/pr/<pr-number>` which is not found by the current implementation using `os.path.listdir` since it only lists files at the current level.

This PR simply reads the file directly if it exists.

Maybe this should also mean that revision should use `replace("/", "--")` during download.

HuggingFaceDocBuilderDev: The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_1316). All of your documentation changes will be reflected on that endpoint.
Wauplin: Thanks for reporting the issue and proposing a fix @OlivierDehaene!
Would you mind adding a test similar to [this one](https://github.com/huggingface/huggingface_hub/blob/b55240e3091c5729e891335d69c8ec72a8021047/tests/test_file_download.py#L410) please ? :pray: 
Something like that: (not tested myself)

```py
    def test_try_to_load_from_cache_specific_pr_revision_exists(self):
        HfApi().create_pull_request(DUMMY_MODEL_ID)
        filepath = hf_hub_download(DUMMY_MODEL_ID, filename=CONFIG_NAME, revision="refs/pr/1")
        attempt = try_to_load_from_cache(DUMMY_MODEL_ID, filename=CONFIG_NAME, revision="refs/pr/1")
        self.assertEqual(filepath, attempt)
```

Patch:
diff --git a/src/huggingface_hub/file_download.py b/src/huggingface_hub/file_download.py
index f8d4ba42..12575a54 100644
--- a/src/huggingface_hub/file_download.py
+++ b/src/huggingface_hub/file_download.py
@@ -1375,9 +1375,9 @@ def try_to_load_from_cache(
 
     # Resolve refs (for instance to convert main to the associated commit sha)
     if os.path.isdir(refs_dir):
-        cached_refs = os.listdir(refs_dir)
-        if revision in cached_refs:
-            with open(os.path.join(refs_dir, revision)) as f:
+        revision_file = os.path.join(refs_dir, revision)
+        if os.path.isfile(revision_file):
+            with open(revision_file) as f:
                 revision = f.read()
 
     # Check if file is cached as "no_exist"


Test patch:
diff --git a/tests/test_file_download.py b/tests/test_file_download.py
index 1e23d276..1abf3196 100644
--- a/tests/test_file_download.py
+++ b/tests/test_file_download.py
@@ -391,6 +391,31 @@ class CachedDownloadTests(unittest.TestCase):
         # Same for uncached models
         self.assertIsNone(try_to_load_from_cache("bert-base", filename=CONFIG_NAME))
 
+    def test_try_to_load_from_cache_specific_pr_revision_exists(self):
+        # Make sure the file is cached
+        file_path = hf_hub_download(
+            DUMMY_MODEL_ID, filename=CONFIG_NAME, revision="refs/pr/1"
+        )
+
+        new_file_path = try_to_load_from_cache(
+            DUMMY_MODEL_ID, filename=CONFIG_NAME, revision="refs/pr/1"
+        )
+        self.assertEqual(file_path, new_file_path)
+
+        # If file is not cached, returns None
+        self.assertIsNone(
+            try_to_load_from_cache(
+                DUMMY_MODEL_ID, filename="conf.json", revision="refs/pr/1"
+            )
+        )
+
+        # If revision does not exist, returns None
+        self.assertIsNone(
+            try_to_load_from_cache(
+                DUMMY_MODEL_ID, filename=CONFIG_NAME, revision="does-not-exist"
+            )
+        )
+
     def test_try_to_load_from_cache_no_exist(self):
         # Make sure the file is cached
         with self.assertRaises(EntryNotFoundError):