Repository: huggingface/datasets
Pull request: #2837 Use cache folder for lockfile
Fixes #2837 

Use a cache folder directory to store the FileLock.

The issue was that the lock file was in a readonly folder.


Problem statement:
prepare_module issue when loading from read-only fs
## Describe the bug

When we use prepare_module from a readonly file system, we create a FileLock using the `local_path`.
This path is not necessarily writable.

`lock_path = local_path + ".lock"`


## Steps to reproduce the bug

Run `load_dataset` on a readonly python loader file.
```python
ds = load_dataset(
        python_loader, data_files={"train": train_path, "test": test_path}
    )
```

where `python_loader` is a path to a file located in a readonly folder.

## Expected results
This should work I think?

## Actual results

```python
    return load_dataset(
  File "/usr/local/lib/python3.8/dist-packages/datasets/load.py", line 711, in load_dataset
    module_path, hash, resolved_file_path = prepare_module(
  File "/usr/local/lib/python3.8/dist-packages/datasets/load.py", line 465, in prepare_module
    with FileLock(lock_path):
  File "/usr/local/lib/python3.8/dist-packages/datasets/utils/filelock.py", line 314, in __enter__
    self.acquire()
  File "/usr/local/lib/python3.8/dist-packages/datasets/utils/filelock.py", line 263, in acquire
    self._acquire()
  File "/usr/local/lib/python3.8/dist-packages/datasets/utils/filelock.py", line 378, in _acquire
    fd = os.open(self._lock_file, open_mode)
OSError: [Errno 30] Read-only file system: 'YOUR_FILE.py.lock'
```

## Environment info
<!-- You can run the command `datasets-cli env` and copy-and-paste its output below. -->
- `datasets` version: 1.7.0
- Platform: macOS-10.15.7-x86_64-i386-64bit
- Python version: 3.8.8
- PyArrow version: 3.0.0

Hints from issue discussion:
Hello, I opened #2887 to fix this.

Commit message: #2837 Fix according to review

Patch to review:
diff --git a/src/datasets/load.py b/src/datasets/load.py
index d00aedc0823..d36e7bd002e 100644
--- a/src/datasets/load.py
+++ b/src/datasets/load.py
@@ -671,8 +671,10 @@ def _get_modification_time(module_hash):
     local_file_path = os.path.join(hash_folder_path, name)
     dataset_infos_path = os.path.join(hash_folder_path, config.DATASETDICT_INFOS_FILENAME)
 
+    # Create the lock file where we know we have write permissions.
+    lock_path = (datasets_modules_path if dataset else metrics_modules_path) + f"{name}.lock"
+
     # Prevent parallel disk operations
-    lock_path = local_path + ".lock"
     with FileLock(lock_path):
         # Create main dataset/metrics folder if needed
         if download_mode == GenerateMode.FORCE_REDOWNLOAD and os.path.exists(main_folder_path):
diff --git a/tests/test_load.py b/tests/test_load.py
index 633fde20580..3dfb137e889 100644
--- a/tests/test_load.py
+++ b/tests/test_load.py
@@ -107,6 +107,15 @@ def dataset_loading_script_dir(tmp_path):
     return str(script_dir)
 
 
+@pytest.fixture
+def dataset_loading_script_dir_readonly(dataset_loading_script_dir):
+    # Make this directory readonly
+    script_name = DATASET_LOADING_SCRIPT_NAME
+    os.chmod(dataset_loading_script_dir, 0o555)
+    os.chmod(os.path.join(dataset_loading_script_dir, f"{script_name}.py"), 0o555)
+    return dataset_loading_script_dir
+
+
 class LoadTest(TestCase):
     @pytest.fixture(autouse=True)
     def inject_fixtures(self, caplog):
@@ -405,6 +414,17 @@ def test_load_dataset_then_move_then_reload(dataset_loading_script_dir, data_dir
     assert dataset._fingerprint != fingerprint1
 
 
+def test_load_dataset_readonly(dataset_loading_script_dir, dataset_loading_script_dir_readonly, data_dir, tmp_path):
+    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
+    # Load readonly dataset and check that the fingerprint is the same.
+    dataset = load_dataset(dataset_loading_script_dir_readonly, data_dir=data_dir, split="train", cache_dir=cache_dir2)
+    assert dataset._fingerprint == fingerprint1, "Cannot load a dataset in a readonly folder."
+
+
 @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: @@ -671,8 +671,10 @@ def _get_modification_time(module_hash):
     local_file_path = os.path.join(hash_folder_path, name)
     dataset_infos_path = os.path.join(hash_folder_path, config.DATASETDICT_INFOS_FILENAME)
 
+    # Create the lock file where we know we have write permissions.
+    lock_path = (datasets_modules_path if dataset else metrics_modules_path) + f"{name}.lock"
path: src/datasets/load.py
text: @user1:
```suggestion
    lock_path = (datasets_modules_path if dataset else metrics_modules_path) + f"{short_name}.lock"
```