Repository: huggingface/datasets
Pull request: Fix fingerprint when moving cache dir
The fingerprint of a dataset changes if the cache directory is moved.
I fixed that by setting the fingerprint to be the hash of:
- the relative cache dir (dataset_name/version/config_id)
- the requested split

Close #2496 

I had to fix an issue with the filelock filename that was too long (>255). It prevented the tests to run on my machine. I just added `hash_filename_if_too_long` in case this happens, to not get filenames longer than 255.
We usually have long filenames for filelocks because they are named after the path that is being locked. In case the path is a cache directory that has long directory names, then the filelock filename could en up being very long.

Problem statement:
Dataset fingerprint changes after moving the cache directory, which prevent cache reload when using `map`
`Dataset.map` uses the dataset fingerprint (a hash) for caching.
However the fingerprint seems to change when someone moves the cache directory of the dataset.

This is because it uses the default fingerprint generation:
1. the dataset path is used to get the fingerprint
2. the modification times of the arrow file is also used to get the fingerprint

To fix that we could set the fingerprint of the dataset to be a hash of (<dataset_name>, <config_name>, <version>, <script_hash>), i.e. a hash of the the cache path relative to the cache directory.

Hints from issue discussion:

Commit message: fix windows path

Patch to review:
diff --git a/src/datasets/builder.py b/src/datasets/builder.py
index 36b68210ba7..4e818611503 100644
--- a/src/datasets/builder.py
+++ b/src/datasets/builder.py
@@ -41,7 +41,7 @@
 from .splits import Split, SplitDict, SplitGenerator
 from .utils.download_manager import DownloadManager, GenerateMode
 from .utils.file_utils import DownloadConfig, is_remote_url
-from .utils.filelock import FileLock
+from .utils.filelock import FileLock, hash_filename_if_too_long
 from .utils.info_utils import get_size_checksum_dict, verify_checksums, verify_splits
 from .utils.logging import WARNING, get_logger
 
@@ -258,7 +258,7 @@ def __init__(
         if not is_remote_url(self._cache_dir_root):
             os.makedirs(self._cache_dir_root, exist_ok=True)
         lock_path = os.path.join(self._cache_dir_root, self._cache_dir.replace(os.sep, "_") + ".lock")
-        with FileLock(lock_path):
+        with FileLock(hash_filename_if_too_long(lock_path)):
             if os.path.exists(self._cache_dir):  # check if data exist
                 if len(os.listdir(self._cache_dir)) > 0:
                     logger.info("Overwrite dataset info from restored data version.")
@@ -381,7 +381,7 @@ def builder_configs(cls):
     def cache_dir(self):
         return self._cache_dir
 
-    def _relative_data_dir(self, with_version=True, with_hash=True):
+    def _relative_data_dir(self, with_version=True, with_hash=True) -> str:
         """Relative path of this dataset in cache_dir:
         Will be:
             self.name/self.config.version/self.hash/
@@ -503,7 +503,7 @@ def download_and_prepare(
 
         # Prevent parallel disk operations
         lock_path = os.path.join(self._cache_dir_root, self._cache_dir.replace(os.sep, "_") + ".lock")
-        with FileLock(lock_path):
+        with FileLock(hash_filename_if_too_long(lock_path)):
             data_exists = os.path.exists(self._cache_dir)
             if data_exists and download_mode == GenerateMode.REUSE_DATASET_IF_EXISTS:
                 logger.warning("Reusing dataset %s (%s)", self.name, self._cache_dir)
@@ -689,12 +689,12 @@ def _load_info(self) -> DatasetInfo:
 
     def _save_info(self):
         lock_path = os.path.join(self._cache_dir_root, self._cache_dir.replace(os.sep, "_") + ".lock")
-        with FileLock(lock_path):
+        with FileLock(hash_filename_if_too_long(lock_path)):
             self.info.write_to_directory(self._cache_dir)
 
     def _save_infos(self):
         lock_path = os.path.join(self._cache_dir_root, self._cache_dir.replace(os.sep, "_") + ".lock")
-        with FileLock(lock_path):
+        with FileLock(hash_filename_if_too_long(lock_path)):
             DatasetInfosDict(**{self.config.name: self.info}).write_to_directory(self.get_imported_module_dir())
 
     def _make_split_generators_kwargs(self, prepare_split_kwargs):
@@ -845,7 +845,11 @@ def _as_dataset(self, split: Union[ReadInstruction, Split] = Split.TRAIN, in_mem
             split_infos=self.info.splits.values(),
             in_memory=in_memory,
         )
-        return Dataset(**dataset_kwargs)
+        hasher = Hasher()
+        hasher.update(self._relative_data_dir().replace(os.sep, "/"))
+        hasher.update(split.to_spec() if isinstance(split, ReadInstruction) else str(split))
+        fingerprint = hasher.hexdigest()
+        return Dataset(fingerprint=fingerprint, **dataset_kwargs)
 
     def _post_process(self, dataset: Dataset, resources_paths: Dict[str, str]) -> Optional[Dataset]:
         """Run dataset transforms or add indexes"""
diff --git a/src/datasets/utils/filelock.py b/src/datasets/utils/filelock.py
index 5d4061d2e24..1eb095f97f6 100644
--- a/src/datasets/utils/filelock.py
+++ b/src/datasets/utils/filelock.py
@@ -62,7 +62,15 @@
 
 # Data
 # ------------------------------------------------
-__all__ = ["Timeout", "BaseFileLock", "WindowsFileLock", "UnixFileLock", "SoftFileLock", "FileLock"]
+__all__ = [
+    "Timeout",
+    "BaseFileLock",
+    "WindowsFileLock",
+    "UnixFileLock",
+    "SoftFileLock",
+    "FileLock",
+    "hash_filename_if_too_long",
+]
 
 __version__ = "3.0.12"
 
@@ -96,6 +104,24 @@ def __str__(self):
         return temp
 
 
+# Helpers
+# ------------------------------------------------
+
+
+MAX_LOCK_FILENAME_LENGTH = 255
+
+
+def hash_filename_if_too_long(path: str, max_length=MAX_LOCK_FILENAME_LENGTH) -> str:
+    filename = os.path.basename(path)
+    if len(filename) > max_length:
+        dirname = os.path.dirname(path)
+        hashed_filename = str(hash(filename))
+        new_filename = filename[: max_length - len(hashed_filename) - 8] + "..." + hashed_filename + ".lock"
+        return os.path.join(dirname, new_filename)
+    else:
+        return path
+
+
 # Classes
 # ------------------------------------------------
 
diff --git a/tests/test_load.py b/tests/test_load.py
index 2772745cc35..cd905324b3e 100644
--- a/tests/test_load.py
+++ b/tests/test_load.py
@@ -227,6 +227,21 @@ def assert_auth(url, *args, headers, **kwargs):
         mock_head.assert_called()
 
 
+def test_load_dataset_then_move_then_reload(dataset_loading_script_dir, data_dir, tmp_path, caplog):
+    cache_dir1 = tmp_path / "cache1"
+    cache_dir2 = tmp_path / "cache2"
+    dataset = load_dataset(dataset_loading_script_dir, data_dir=data_dir, split="train", cache_dir=cache_dir1)
+    fingerprint1 = dataset._fingerprint
+    del dataset
+    os.rename(cache_dir1, cache_dir2)
+    caplog.clear()
+    dataset = load_dataset(dataset_loading_script_dir, data_dir=data_dir, split="train", cache_dir=cache_dir2)
+    assert "Reusing dataset" in caplog.text
+    assert dataset._fingerprint == fingerprint1, "for the caching mechanism to work, fingerprint should stay the same"
+    dataset = load_dataset(dataset_loading_script_dir, data_dir=data_dir, split="test", cache_dir=cache_dir2)
+    assert dataset._fingerprint != fingerprint1
+
+
 @pytest.mark.parametrize("max_in_memory_dataset_size", ["default", 0, 50, 500])
 def test_load_dataset_local_with_default_in_memory(
     max_in_memory_dataset_size, dataset_loading_script_dir, data_dir, monkeypatch


Code review comments:
diff_hunk: @@ -96,6 +104,24 @@ def __str__(self):
         return temp
 
 
+# Helpers
+# ------------------------------------------------
+
+
+MAX_LOCK_FILENAME_LENGTH = 255
+
+
+def hash_filename_if_too_long(path: str, max_length=MAX_LOCK_FILENAME_LENGTH) -> str:
path: src/datasets/utils/filelock.py
text: @user1:
As this function is only used by FileLock, maybe it could become a method of that class: it is the class responsibility to check whether the filename is too long and hash it.

@user1:
And I would recommend adding some unit tests for this function (even if they seem quite trivial).

diff_hunk: @@ -96,6 +104,24 @@ def __str__(self):
         return temp
 
 
+# Helpers
+# ------------------------------------------------
+
+
+MAX_LOCK_FILENAME_LENGTH = 255
path: src/datasets/utils/filelock.py
text: @user1:
Is this a parameter that the user should be allowed to customize? If yes, I would suggest moving it to `config`; if not (I think this is the case), then I would suggest to remove the variable and pass the value directly in the function signature of `hash_filename_if_too_long`.

diff_hunk: @@ -845,7 +845,11 @@ def _as_dataset(self, split: Union[ReadInstruction, Split] = Split.TRAIN, in_mem
             split_infos=self.info.splits.values(),
             in_memory=in_memory,
         )
-        return Dataset(**dataset_kwargs)
+        hasher = Hasher()
+        hasher.update(self._relative_data_dir().replace(os.sep, "/"))
+        hasher.update(split.to_spec() if isinstance(split, ReadInstruction) else str(split))
+        fingerprint = hasher.hexdigest()
path: src/datasets/builder.py
text: @user1:
This might be extracted into a funcion/method.

diff_hunk: @@ -845,7 +845,11 @@ def _as_dataset(self, split: Union[ReadInstruction, Split] = Split.TRAIN, in_mem
             split_infos=self.info.splits.values(),
             in_memory=in_memory,
         )
-        return Dataset(**dataset_kwargs)
+        hasher = Hasher()
+        hasher.update(self._relative_data_dir().replace(os.sep, "/"))
+        hasher.update(split.to_spec() if isinstance(split, ReadInstruction) else str(split))
path: src/datasets/builder.py
text: @user1:
`ReadInstruction.__str__` calls `to_spec`, so this line can be replaced with:
 
```suggestion
        hasher.update(str(split))
```

@author:
Nice thanks !