Adding lock mechanism to prevent on_disk_cache downloading twice (#409)

Summary:
Pull Request resolved: #409

Fixes #144

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D36489060

Pulled By: VitalyFedyunin

fbshipit-source-id: d33624d2d38d4756789b1ebb09eb05d2b4efe6a6

Author: VitalyFedyunin

requirements.txt

@@ -1,2 +1,3 @@
 urllib3 >= 1.25
 requests
+portalocker >= 2.0.0

test/test_local_io.py

@@ -5,13 +5,16 @@
 # LICENSE file in the root directory of this source tree.
 
 import bz2
+import functools
 import hashlib
 import io
 import itertools
 import lzma
 import os
 import subprocess
 import tarfile
+import tempfile
+import time
 import unittest
 import warnings
 import zipfile
@@ -22,6 +25,8 @@
 import expecttest
 
 from _utils._common_utils_for_test import create_temp_dir, create_temp_files, get_name, reset_after_n_next_calls
+
+from torch.utils.data import DataLoader
 from torchdata.datapipes.iter import (
     Bz2FileLoader,
     CSVDictParser,
@@ -34,9 +39,11 @@
     IoPathFileOpener,
     IoPathSaver,
     IterableWrapper,
+    IterDataPipe,
     JsonParser,
     RarArchiveLoader,
     Saver,
+    StreamReader,
     TarArchiveLoader,
     WebDataset,
     XzFileLoader,
@@ -77,6 +84,14 @@ def init_fn(worker_id):
     torch.utils.data.graph_settings.apply_sharding(datapipe, num_workers, worker_id)
 
 
+def _unbatch(x):
+    return x[0]
+
+
+def _noop(x):
+    return x
+
+
 class TestDataPipeLocalIO(expecttest.TestCase):
     def setUp(self):
         self.temp_dir = create_temp_dir()
@@ -606,6 +621,31 @@ def _write_text_files(self):
         saver_dp = source_dp.save_to_disk(filepath_fn=partial(filepath_fn, self.temp_dir.name), mode="wb")
         list(saver_dp)
 
+    @staticmethod
+    def _slow_fn(tmpdirname, x):
+        with open(os.path.join(tmpdirname, str(os.getpid())), "w") as pid_fh:
+            pid_fh.write("anything")
+        time.sleep(2)
+        return (x, "str")
+
+    def test_disk_cache_locks(self):
+        with tempfile.TemporaryDirectory() as tmpdirname:
+            file_name = os.path.join(tmpdirname, "test.bin")
+            dp = IterableWrapper([file_name])
+            dp = dp.on_disk_cache(filepath_fn=_noop)
+            dp = dp.map(functools.partial(self._slow_fn, tmpdirname))
+            dp = dp.end_caching(mode="t", filepath_fn=_noop, timeout=120)
+            dp = FileOpener(dp)
+            dp = StreamReader(dp)
+            dl = DataLoader(dp, num_workers=10, multiprocessing_context="spawn", batch_size=1, collate_fn=_unbatch)
+            result = list(dl)
+            all_files = []
+            for (_, _, filenames) in os.walk(tmpdirname):
+                all_files += filenames
+            # We expect only two files, one with pid and 'downloaded' one
+            self.assertEqual(2, len(all_files))
+            self.assertEqual("str", result[0][1])
+
     # TODO(120): this test currently only covers reading from local
     # filesystem. It needs to be modified once test data can be stored on
     # gdrive/s3/onedrive

test/test_remote_io.py

@@ -13,7 +13,8 @@
 
 import torchdata
 
-from _utils._common_utils_for_test import check_hash_fn, create_temp_dir
+from _utils._common_utils_for_test import check_hash_fn, create_temp_dir, IS_WINDOWS
+from torch.utils.data import DataLoader
 
 from torchdata.datapipes.iter import (
     EndOnDiskCacheHolder,
@@ -143,8 +144,9 @@ def _read_and_decode(x):
 
         cached_it = iter(file_cache_dp)
         for expected_csv_path in _gen_filepath_fn(expected_file_name):
-            # File doesn't exist on disk
-            self.assertFalse(os.path.exists(expected_csv_path))
+
+            # Check disabled due to some elements of prefetching inside of on_disck_cache
+            # self.assertFalse(os.path.exists(expected_csv_path))
 
             csv_path = next(cached_it)
 
@@ -167,15 +169,22 @@ def _read_and_decode(x):
         cached_it = iter(file_cache_dp)
         for i in range(3):
             expected_csv_path = os.path.join(self.temp_dir.name, root_dir, f"{i}.csv")
+
             # File doesn't exist on disk
-            self.assertFalse(os.path.exists(expected_csv_path))
+            # Check disabled due to some elements of prefetching inside of on_disck_cache
+            # self.assertFalse(os.path.exists(expected_csv_path))
 
             csv_path = next(cached_it)
 
             # File is cached to disk
             self.assertTrue(os.path.exists(expected_csv_path))
             self.assertEqual(expected_csv_path, csv_path)
 
+        if not IS_WINDOWS:
+            dl = DataLoader(file_cache_dp, num_workers=3, multiprocessing_context="fork", batch_size=1)
+            expected = [[os.path.join(self.temp_dir.name, root_dir, f"{i}.csv")] for i in range(3)] * 3
+            self.assertEqual(sorted(expected), sorted(list(dl)))
+
     def test_s3_io_iterdatapipe(self):
         # sanity test
         file_urls = ["s3://ai2-public-datasets"]

torchdata/datapipes/iter/util/cacheholder.py

@@ -4,14 +4,19 @@
 # This source code is licensed under the BSD-style license found in the
 # LICENSE file in the root directory of this source tree.
 
+import functools
 import hashlib
 import inspect
 import os.path
 import sys
+import time
+import warnings
 
 from collections import deque
 from functools import partial
-from typing import Callable, Deque, Dict, Iterator, Optional, TypeVar
+from typing import Any, Callable, Deque, Dict, Iterator, List, Optional, TypeVar
+
+import portalocker
 
 from torch.utils.data.datapipes.utils.common import _check_lambda_fn, DILL_AVAILABLE
 
@@ -26,6 +31,9 @@
 
 T_co = TypeVar("T_co", covariant=True)
 
+PROMISE_FILE_DELETE_TIMEOUT = 30
+PROMISE_FILE_DELETE_RETRY_INTERVAL = 0.005
+
 
 @functional_datapipe("in_memory_cache")
 class InMemoryCacheHolderIterDataPipe(IterDataPipe[T_co]):
@@ -106,6 +114,9 @@ def _hash_check(filepath, hash_dict, hash_type):
     else:
         hash_func = hashlib.md5()
 
+    # with portalocker.Lock(filepath, "rb", flags=portalocker.LockFlags.SHARED) as f:
+    # TODO(VitalyFedyunin): Line above will require all readers (Win) to obtain proper locks,
+    # I'm putting it on hold as we need to modify PyTorch core codebase heavily.
     with open(filepath, "rb") as f:
         chunk = f.read(1024 ** 2)
         while chunk:
@@ -115,6 +126,10 @@ def _hash_check(filepath, hash_dict, hash_type):
     return hash_func.hexdigest() == hash_dict[filepath]
 
 
+def _promise_filename(filename):
+    return filename + ".promise"
+
+
 @functional_datapipe("on_disk_cache")
 class OnDiskCacheHolderIterDataPipe(IterDataPipe):
     """
@@ -145,7 +160,7 @@ class OnDiskCacheHolderIterDataPipe(IterDataPipe):
         >>> hash_dict = {"expected_filepath": expected_MD5_hash}
         >>> cache_dp = url.on_disk_cache(filepath_fn=_filepath_fn, hash_dict=_hash_dict, hash_type="md5")
         >>> # You must call ``.end_caching`` at a later point to stop tracing and save the results to local files.
-        >>> cache_dp = HttpReader(cache_dp).end_caching(mode="wb". filepath_fn=_filepath_fn)
+        >>> cache_dp = HttpReader(cache_dp).end_caching(mode="wb", filepath_fn=_filepath_fn)
     """
 
     _temp_dict: Dict = {}
@@ -184,22 +199,42 @@ def __add__(self, other_datapipe):
     @staticmethod
     def _cache_check_fn(data, filepath_fn, hash_dict, hash_type, extra_check_fn):
         filepaths = data if filepath_fn is None else filepath_fn(data)
+        result = True
         if not isinstance(filepaths, (list, tuple)):
             filepaths = [
                 filepaths,
             ]
 
         for filepath in filepaths:
+            cached_file_exists = True
             if not os.path.exists(filepath):
-                return False
-
-            if hash_dict is not None and not _hash_check(filepath, hash_dict, hash_type):
-                return False
-
-            if extra_check_fn is not None and not extra_check_fn(filepath):
-                return False
-
-        return True
+                cached_file_exists = False
+            elif hash_dict is not None and not _hash_check(filepath, hash_dict, hash_type):
+                cached_file_exists = False
+            elif extra_check_fn is not None and not extra_check_fn(filepath):
+                cached_file_exists = False
+
+            if not cached_file_exists:
+                promise_filepath = _promise_filename(filepath)
+                dirname = os.path.dirname(promise_filepath)
+                if not os.path.exists(dirname):
+                    os.makedirs(dirname)
+
+                with portalocker.Lock(promise_filepath, "a+", flags=portalocker.LockFlags.EXCLUSIVE) as promise_fh:
+                    promise_fh.seek(0)
+                    data = promise_fh.read()
+                    # TODO(VitalyFedyunin): Potentially there is old .promise file from previous failed run, we
+                    # need to somehow propagate uniq session id for dataloader, save and compare it here,
+                    # raising error
+                    file_exists = len(data) > 0
+                    if not file_exists:
+                        result = False
+                        promise_fh.seek(0)
+                        promise_fh.write("[dataloader session uid]")
+                        promise_fh.truncate()
+                        promise_fh.flush()
+
+        return result
 
     def _end_caching(self):
         filepath_fn, hash_dict, hash_type, extra_check_fn = OnDiskCacheHolderIterDataPipe._temp_dict.pop(self)
@@ -232,6 +267,82 @@ def _read_str(fd):
     return "".join(fd)
 
 
+def _find_promise_file(filename):
+    promise_filename = _promise_filename(filename)
+    while not os.path.exists(promise_filename):
+        dirname = os.path.dirname(promise_filename)
+        if dirname == os.path.dirname(dirname):
+            promise_filename = _promise_filename(filename)
+            break
+        promise_filename = _promise_filename(dirname)
+    return promise_filename
+
+
+def _is_promise_pending(promise_filename):
+    return os.path.exists(promise_filename)
+
+
+def _wait_promise_fn(timeout, filename):
+    promise_filename = _find_promise_file(filename)
+    start = time.time()
+    while _is_promise_pending(promise_filename):
+        time.sleep(0.01)
+        if time.time() - start > timeout:
+            raise Exception(
+                f"OnDiskCache Exception: {filename} expected to be written by different process, "
+                + f"but file is not ready in {timeout} seconds."
+            )
+    return filename
+
+
+class _FulfilledPromisesIterDataPipe(IterDataPipe):
+    def __init__(self, source_datapipe):
+        self.source_datapipe = source_datapipe
+
+    @staticmethod
+    def _del_promise_file(promise_filename, filename):
+        if os.path.exists(promise_filename):
+            retry = True
+            start = time.time()
+            while retry:
+                retry = False
+                try:
+                    os.unlink(promise_filename)
+                except Exception as e:
+                    # Workaround about Windows not letting to delete file, while it is open by another process
+                    retry = True
+                    if time.time() - start > PROMISE_FILE_DELETE_TIMEOUT:
+                        raise Exception("Timeout while trying to recover from the ", type(e), e)
+                    time.sleep(PROMISE_FILE_DELETE_RETRY_INTERVAL)
+        else:
+            warnings.warn(
+                f"Attempt to mark {promise_filename} promise (base of file {filename}) as fulfilled failed. Potentially missmatching filename functions of on_disk_cache and end_cache."
+            )
+
+    def __iter__(self):
+        old_promise_filename = None
+        old_filename = None
+        first_entry = True
+        # TODO(VitalyFedyunin): Limit buffer size here. It is only contains file names from archive,
+        # but better be save than sorry.
+        buffer: List[Any] = []
+        for filename in self.source_datapipe:
+            promise_filename = _find_promise_file(filename)
+            if not first_entry:
+                buffer.append(old_filename)
+                if old_promise_filename != promise_filename:
+                    self._del_promise_file(old_promise_filename, old_filename)
+                    yield from buffer
+                    buffer = []
+            old_promise_filename = promise_filename
+            old_filename = filename
+            first_entry = False
+        if not first_entry:
+            buffer.append(old_filename)
+            self._del_promise_file(old_promise_filename, old_filename)
+            yield from buffer
+
+
 @functional_datapipe("end_caching")
 class EndOnDiskCacheHolderIterDataPipe(IterDataPipe):
     """
@@ -248,6 +359,7 @@ class EndOnDiskCacheHolderIterDataPipe(IterDataPipe):
         same_filepath_fn: Set to ``True`` to use same ``filepath_fn`` from the ``OnDiskCacheHolder``.
         skip_read: Boolean value to skip reading the file handle from ``datapipe``.
             By default, reading is enabled and reading function is created based on the ``mode``.
+        timeout: Integer value of seconds to wait for uncached item to be written to disk
 
     Example:
         >>> from torchdata.datapipes.iter import IterableWrapper, HttpReader
@@ -259,10 +371,10 @@ class EndOnDiskCacheHolderIterDataPipe(IterDataPipe):
         >>> # You must call ``.on_disk_cache`` at some point before ``.end_caching``
         >>> cache_dp = url.on_disk_cache(filepath_fn=_filepath_fn, hash_dict=_hash_dict, hash_type="md5")
         >>> # You must call ``.end_caching`` at a later point to stop tracing and save the results to local files.
-        >>> cache_dp = HttpReader(cache_dp).end_caching(mode="wb". filepath_fn=_filepath_fn)
+        >>> cache_dp = HttpReader(cache_dp).end_caching(mode="wb", filepath_fn=_filepath_fn)
     """
 
-    def __new__(cls, datapipe, mode="wb", filepath_fn=None, *, same_filepath_fn=False, skip_read=False):
+    def __new__(cls, datapipe, mode="wb", filepath_fn=None, *, same_filepath_fn=False, skip_read=False, timeout=300):
         if filepath_fn is not None and same_filepath_fn:
             raise ValueError("`filepath_fn` is mutually exclusive with `same_filepath_fn`")
 
@@ -276,6 +388,7 @@ def __new__(cls, datapipe, mode="wb", filepath_fn=None, *, same_filepath_fn=Fals
 
         _filepath_fn, _hash_dict, _hash_type, _ = OnDiskCacheHolderIterDataPipe._temp_dict[cache_holder]
         cached_dp = cache_holder._end_caching()
+        cached_dp = cached_dp.map(functools.partial(_wait_promise_fn, timeout))
         cached_dp = FileLister(cached_dp, recursive=True)
 
         if same_filepath_fn:
@@ -297,6 +410,7 @@ def __new__(cls, datapipe, mode="wb", filepath_fn=None, *, same_filepath_fn=Fals
             todo_dp = todo_dp.check_hash(_hash_dict, _hash_type)
 
         todo_dp = todo_dp.save_to_disk(mode=mode)
+        todo_dp = _FulfilledPromisesIterDataPipe(todo_dp)
 
         return cached_dp.concat(todo_dp)
 

torchdata/datapipes/iter/util/saver.py

@@ -56,6 +56,8 @@ def __iter__(self) -> Iterator[str]:
             dirname = os.path.dirname(filepath)
             if not os.path.exists(dirname):
                 os.makedirs(dirname)
+            # with portalocker.Lock(filepath, self.mode, flags=portalocker.LockFlags.EXCLUSIVE) as f:
+            # TODO(VitalyFedyunin): Enabling line above will require all read sites to be updated (Win).
             with open(filepath, self.mode) as f:
                 f.write(data)
             yield filepath