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

Keep lock files in a `/locks` folder to prevent rare concurrency issue
The lock file could be removed when the 2nd process gets the lock.
And then the 3rd process will lock on a different lock file handle.
Although the lock path stays the same.

1st proc gets the lock
2nd proc waits for the lock
1st proc releases the lock
1st proc removes the lock file
2nd proc gets the lock
3rd proc creates a new lock file and gets the lock

Demo code to show the problem:

```py
from filelock import FileLock
import threading
import os
import time

lock_path = "/tmp/test.lock"

def lock1():
   with FileLock(lock_path):
      print("lock acquired in thread1")
      time.sleep(1)

   print("lock released in thread1")
   time.sleep(1)
   os.remove(lock_path)
   print("lock file removed in thread1")

def lock2():
   time.sleep(1)
   with FileLock(lock_path):
      print("lock acquired in thread2")
      time.sleep(10)

   print("lock released in thread2")

def lock3():
   time.sleep(3)
   with FileLock(lock_path):
      print("lock acquired in thread3")
      time.sleep(10)


t1 = threading.Thread(target=lock1)
t2 = threading.Thread(target=lock2)
t3 = threading.Thread(target=lock3)
t1.start()
t2.start()
t3.start()
t1.join()
t2.join()
t3.join()
```

~Add ENV `HF_HUB_KEEP_LOCK_FILES` to give user option to keep the lock files to prevent concurrency issues.~

**EDIT on 2023/1014**
---

Windows doesn't have this problem.

This commit moves the lock files to a subdirectory of the hub cache,
and don't remove it after downloading.

The lock files are named with their `etag` and `.lock` extension, placed
in the `HUGGINGFACE_HUB_CACHE/.locks/repo_folder_name` directory. The
repo_folder_name is generated from `repo_id` and `repo_type`, to avoid
same `etag` in different repos.


Wauplin: Hi @beeender thanks for raising the question here. I think the reason why we want to delete the lock file is to avoid polluting the cache system. Filelock has different ways of handling the file lock depending on the platform:
- on Windows, [it does remove the file](https://github.com/tox-dev/filelock/blob/0a338c11587cb243aa99fcade7bf605a6a6e0725/src/filelock/_windows.py#L48) 
- on Unix that does not support [fcntl](https://docs.python.org/3/library/fcntl.html#fcntl.fcntl), [it does remove the file](https://github.com/tox-dev/filelock/blob/0a338c11587cb243aa99fcade7bf605a6a6e0725/src/filelock/_soft.py#L41)
- on Unix that supports fcntl (most of them?), the [file lock is NOT removed](https://github.com/tox-dev/filelock/blob/0a338c11587cb243aa99fcade7bf605a6a6e0725/src/filelock/_unix.py#L52) by default

What we could do to ensure that the file lock is removed in any case is to use only the `WindowsFileLock` and `SoftFileLock`, instead of `UnixFileLock` (taking the logic from [here](https://github.com/tox-dev/filelock/blob/0a338c11587cb243aa99fcade7bf605a6a6e0725/src/filelock/__init__.py#L25)). Not an expert here so I'd certainly need to double-check the possible side effect. 

cc-ing @julien-c who implemented it in https://github.com/huggingface/huggingface_hub/pull/801

HuggingFaceDocBuilderDev: The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/huggingface_hub/pr_1659). All of your documentation changes will be reflected on that endpoint.
Wauplin: @beeender after thinking about it and some discussions with @julien-c, we ended up with the conclusion:
- We want to use `filelock` with the default Unix and Windows implementations that relies not only on the file existence but also some low-level to check if a process currently owns the lock. Therefore I'd rather not use `SoftFileLock` by default everywhere. It's a good proxy but it'd be quite easy to end-up in a dead-lock.
- By default, `filelock` doesn't remove the lock files. There have been issues with that in the past (see https://github.com/tox-dev/filelock/issues/31) and there are no satisfying good practices to remove them (see https://github.com/tox-dev/filelock/issues/76).
- We still prefer to remove the lock files in order to leave a "clean" cache. Leaving lock files is not wrong but looks messy to users. Also leaving lock _might_ break some downstream tools (at least not scan/delete cache, not snapshot_download but we might be missing something?)
- Deleting lock files as we do it now can lead to concurrency issues but very few cases have been reported in 18 months, so hopefully not very problematic for most of our users.

That being said what we can do is to add a new flag (`HF_HUB_KEEP_LOCK_FILES`?) for power-users like you that have multiple processes downloading a lot of files in parallel. What do you think @beeender ? That would be a bit more work on the PR (both logic-wise and docs-wise) but it seems to me a good compromise between usage and robustness.
beeender: > That being said what we can do is to add a new flag (HF_HUB_KEEP_LOCK_FILES?) 

This sounds good to me! Let me add that in the PR.
Wauplin: Great, thanks! 

To do so, I would:
- add it to the [reference page](https://github.com/huggingface/huggingface_hub/blob/main/docs/source/en/package_reference/environment_variables.md)
- add it [constants.py](https://github.com/huggingface/huggingface_hub/blob/main/src/huggingface_hub/constants.py)
- end then use it in file_download.py
- add a quick test

And that should be it. Please let me know if you need any help. Thanks in advance!
beeender: ```
+    def test_keep_lock_file(self):
+        """Lock files should be kept if HF_HUB_KEEP_LOCK_FILES is True
+        """
+        with SoftTemporaryDirectory() as tmpdir:
+            with patch("huggingface_hub.constants.HF_HUB_KEEP_LOCK_FILES", False):
+                hf_hub_download(DUMMY_MODEL_ID, filename=CONFIG_NAME, cache_dir=tmpdir)
+            for subdir, dirs, files in os.walk(tmpdir):
+                for file in files:
+                    # No lock files should exist
+                    self.assertNotRegex(file, ".*\.lock")
+
+        with SoftTemporaryDirectory() as tmpdir:
+            with patch("huggingface_hub.constants.HF_HUB_KEEP_LOCK_FILES", True):
+                hf_hub_download(DUMMY_MODEL_ID, filename=CONFIG_NAME, cache_dir=tmpdir)
+            lock_file_exist = False
+            for subdir, dirs, files in os.walk(tmpdir):
+                for file in files:
+                    if file.endWith(".lock"):
+                        lock_file_exist = True
+                        break
+            self.assertTrue(lock_file_exist)
```

@Wauplin My constants mocking doesn't work, do you have any idea?
Wauplin: > My constants mocking doesn't work, do you have any idea?

Since you are importing the constant directly in `file_download`, you should patch the value there:

```
with patch("huggingface_hub.file_download.HF_HUB_KEEP_LOCK_FILES", True):
```

and it should work.

beeender: > > My constants mocking doesn't work, do you have any idea?
> 
> Since you are importing the constant directly in `file_download`, you should patch the value there:
> 
> ```
> with patch("huggingface_hub.file_download.HF_HUB_KEEP_LOCK_FILES", True):
> ```
> 
> and it should work.

Magic! And tests have been added.
beeender: In another thought, shall we just create the lock file in the **temp** folder, so it does not have to be deleted.
Like:

```
/tmp/huggingface/<CHECKSUM_FILE_PATH>.lock
```

This may not work if the cache folder is a sharing folder (nfs/samba) and downloading happens on multiple hosts. But I doubt the flock ever works with nfs/samba.
`HF_HUB_KEEP_LOCK_FILES` won't be needed then.

Wauplin: I like the idea! 

What do you think about leaving the lock files in a separate directory but still in the `$HF_HOME` folder? This way we ensure that locks on shared drive still works as they do now (even if it's only based on file existence, it's better than nothing). That would give us something like this:

```
➜  ~ ls -la ~/.cache/huggingface/hub                                  
total 104
drwxrwxr-x 21 wauplin wauplin 16384 sept. 20 08:57 .
drwxrwxr-x  6 wauplin wauplin  4096 sept. 18 16:39 ..
drwxrwxr-x  5 wauplin wauplin  4096 avril  6 17:07 datasets--hf-internal-testing--dummy-flac-single-example
drwxrwxr-x  5 wauplin wauplin  4096 mai   26 10:22 datasets--Narsil--image_dummy
drwxrwxr-x  2 wauplin wauplin  4096 sept. 20 08:50 .locks
drwxrwxr-x  5 wauplin wauplin  4096 avril 25 14:53 models--admruul--anything-v3.0
drwxrwxr-x  5 wauplin wauplin  4096 juil. 10 12:51 models--CompVis--stable-diffusion-v1-4
drwxrwxr-x  5 wauplin wauplin  4096 sept. 11 12:50 models--gpt2
drwxrwxr-x  5 wauplin wauplin  4096 sept. 19 13:43 models--hf-internal-testing--dummy-gated-model
drwxrwxr-x  6 wauplin wauplin  4096 avril 24 10:04 models--hf-internal-testing--tiny-random-bert
(...)

➜  ~ tree -la ~/.cache/huggingface/hub/.locks                                                
/home/wauplin/.cache/huggingface/hub/.locks
└── 46a9d58622b4675b29564da2d9ba73e702241c5fa969f12c387cad4aa984276a.lock

0 directories, 1 file
```
with `lock_path = os.path.join(locks_dir, f"{filename}.lock")`. Filename is based on the etag of the file on the Hub. I'll need to double check that etags are unique across repos (I think so but better to check). In any case it should be easy to build a unique lock path per blob file. 

That way we still have lock files but not messing around everywhere in the cache. We could have an utility to clean the locks folder (either after snapshot_download or when scanning/deleting the cache `huggingface-cli delete-cache`). "Cleaning the locks folder" could be "delete all locks older than 12h" for instance. Whatever, this part is not so critical.

julien-c: yes, great idea!

and a CLI script could take care of deleting .locks from time to time if needed.
beeender: > I'm making some cleaning on the repo so I renamed this PR accordingly with what we discussed in https://github.com/huggingface/huggingface_hub/pull/1659#issuecomment-1727098559. @beeender are you still interested in implementing it? :)

Yes! I was on vacation for the last week. Will create a PR soon.
beeender: @Wauplin PR has been updated. :up: 

Patch:
diff --git a/src/huggingface_hub/file_download.py b/src/huggingface_hub/file_download.py
index 7f540eea..dcfd543f 100644
--- a/src/huggingface_hub/file_download.py
+++ b/src/huggingface_hub/file_download.py
@@ -1197,6 +1197,7 @@ def hf_hub_download(
         cache_dir = str(cache_dir)
     if isinstance(local_dir, Path):
         local_dir = str(local_dir)
+    locks_dir = os.path.join(cache_dir, ".locks")
 
     if subfolder == "":
         subfolder = None
@@ -1410,7 +1411,8 @@ def hf_hub_download(
             return pointer_path
 
     # Prevent parallel downloads of the same file with a lock.
-    lock_path = blob_path + ".lock"
+    # etag could be duplicated across repos,
+    lock_path = os.path.join(locks_dir, repo_folder_name(repo_id=repo_id, repo_type=repo_type), f"{etag}.lock")
 
     # Some Windows versions do not allow for paths longer than 255 characters.
     # In this case, we must specify it is an extended path by using the "\\?\" prefix.
@@ -1420,6 +1422,7 @@ def hf_hub_download(
     if os.name == "nt" and len(os.path.abspath(blob_path)) > 255:
         blob_path = "\\\\?\\" + os.path.abspath(blob_path)
 
+    Path(lock_path).parent.mkdir(parents=True, exist_ok=True)
     with FileLock(lock_path):
         # If the download just completed while the lock was activated.
         if os.path.exists(pointer_path) and not force_download:
@@ -1494,11 +1497,6 @@ def hf_hub_download(
                 _chmod_and_replace(temp_file.name, local_dir_filepath)
             pointer_path = local_dir_filepath  # for return value
 
-    try:
-        os.remove(lock_path)
-    except OSError:
-        pass
-
     return pointer_path
 
 
diff --git a/src/huggingface_hub/inference/_client.py b/src/huggingface_hub/inference/_client.py
index b060a775..837cedb7 100644
--- a/src/huggingface_hub/inference/_client.py
+++ b/src/huggingface_hub/inference/_client.py
@@ -146,7 +146,7 @@ class InferenceClient:
         return f"<InferenceClient(model='{self.model if self.model else ''}', timeout={self.timeout})>"
 
     @overload
-    def post(  # type: ignore
+    def post(  # type: ignore[misc]
         self,
         *,
         json: Optional[Union[str, Dict, List]] = None,
@@ -158,7 +158,7 @@ class InferenceClient:
         pass
 
     @overload
-    def post(  # type: ignore
+    def post(
         self,
         *,
         json: Optional[Union[str, Dict, List]] = None,
diff --git a/src/huggingface_hub/inference/_generated/_async_client.py b/src/huggingface_hub/inference/_generated/_async_client.py
index 3ab4faf4..733fa573 100644
--- a/src/huggingface_hub/inference/_generated/_async_client.py
+++ b/src/huggingface_hub/inference/_generated/_async_client.py
@@ -130,7 +130,7 @@ class AsyncInferenceClient:
         return f"<InferenceClient(model='{self.model if self.model else ''}', timeout={self.timeout})>"
 
     @overload
-    async def post(  # type: ignore
+    async def post(  # type: ignore[misc]
         self,
         *,
         json: Optional[Union[str, Dict, List]] = None,
@@ -142,7 +142,7 @@ class AsyncInferenceClient:
         pass
 
     @overload
-    async def post(  # type: ignore
+    async def post(
         self,
         *,
         json: Optional[Union[str, Dict, List]] = None,
diff --git a/src/huggingface_hub/utils/_cache_manager.py b/src/huggingface_hub/utils/_cache_manager.py
index 16314ae4..e9368811 100644
--- a/src/huggingface_hub/utils/_cache_manager.py
+++ b/src/huggingface_hub/utils/_cache_manager.py
@@ -600,6 +600,8 @@ def scan_cache_dir(cache_dir: Optional[Union[str, Path]] = None) -> HFCacheInfo:
     repos: Set[CachedRepoInfo] = set()
     warnings: List[CorruptedCacheException] = []
     for repo_path in cache_dir.iterdir():
+        if repo_path.name == ".locks":  # skip './.locks/' folder
+            continue
         try:
             repos.add(_scan_cached_repo(repo_path))
         except CorruptedCacheException as e:
diff --git a/utils/generate_async_inference_client.py b/utils/generate_async_inference_client.py
index 967ec929..1bb6e843 100644
--- a/utils/generate_async_inference_client.py
+++ b/utils/generate_async_inference_client.py
@@ -263,7 +263,7 @@ def _make_tasks_methods_async(code: str) -> str:
             def[ ] # def
             [a-z]\w*? # method name (not starting by _)
             \( # parenthesis
-            (\s*\#[ ]type:[ ]ignore)? # optionally type: ignore
+            (\s*\#[ ]type:[ ]ignore(\[misc\])?)? # optionally 'type: ignore' or 'type: ignore[misc]'
             \s*self, # expect self, as first arg
         )""",
         repl=r"\1async \2",  # insert "async" keyword


Test patch:
diff --git a/tests/test_file_download.py b/tests/test_file_download.py
index 63de900c..d44c8ded 100644
--- a/tests/test_file_download.py
+++ b/tests/test_file_download.py
@@ -653,6 +653,20 @@ class CachedDownloadTests(unittest.TestCase):
                     cache_dir=tmpdir,
                 )
 
+    @unittest.skipIf(os.name == "nt", "Lock files are always deleted on Windows.")
+    def test_keep_lock_file(self):
+        """Lock files should not be deleted on Linux."""
+        with SoftTemporaryDirectory() as tmpdir:
+            hf_hub_download(DUMMY_MODEL_ID, filename=CONFIG_NAME, cache_dir=tmpdir)
+            lock_file_exist = False
+            locks_dir = os.path.join(tmpdir, ".locks")
+            for subdir, dirs, files in os.walk(locks_dir):
+                for file in files:
+                    if file.endswith(".lock"):
+                        lock_file_exist = True
+                        break
+            self.assertTrue(lock_file_exist, "no lock file can be found")
+
 
 @with_production_testing
 @pytest.mark.usefixtures("fx_cache_dir")