Support to ignore specific storage options when tokenizing (#1933)

Author: ap--

fsspec/conftest.py

@@ -3,11 +3,12 @@
 import subprocess
 import sys
 import time
+from collections import deque
+from collections.abc import Generator, Sequence
 
 import pytest
 
 import fsspec
-from fsspec.implementations.cached import CachingFileSystem
 
 
 @pytest.fixture()
@@ -27,16 +28,85 @@ def m():
         m.pseudo_dirs.append("")
 
 
-@pytest.fixture
+class InstanceCacheInspector:
+    """
+    Helper class to inspect instance caches of filesystem classes in tests.
+    """
+
+    def clear(self) -> None:
+        """
+        Clear instance caches of all currently imported filesystem classes.
+        """
+        classes = deque([fsspec.spec.AbstractFileSystem])
+        while classes:
+            cls = classes.popleft()
+            cls.clear_instance_cache()
+            classes.extend(cls.__subclasses__())
+
+    def gather_counts(self, *, omit_zero: bool = True) -> dict[str, int]:
+        """
+        Gather counts of filesystem instances in the instance caches
+        of all currently imported filesystem classes.
+
+        Parameters
+        ----------
+        omit_zero:
+            Whether to omit instance types with no cached instances.
+        """
+        out: dict[str, int] = {}
+        classes = deque([fsspec.spec.AbstractFileSystem])
+        while classes:
+            cls = classes.popleft()
+            count = len(cls._cache)  # there is no public interface for the cache
+            # note: skip intermediate AbstractFileSystem subclasses
+            #   if they proxy the protocol attribute via a property.
+            if isinstance(cls.protocol, (Sequence, str)):
+                key = cls.protocol if isinstance(cls.protocol, str) else cls.protocol[0]
+                if count or not omit_zero:
+                    out[key] = count
+            classes.extend(cls.__subclasses__())
+        return out
+
+
+@pytest.fixture(scope="function", autouse=True)
+def instance_caches() -> Generator[InstanceCacheInspector, None, None]:
+    """
+    Fixture to ensure empty filesystem instance caches before and after a test.
+
+    Used by default for all tests.
+    Clears caches of all imported filesystem classes.
+    Can be used to write test assertions about instance caches.
+
+    Usage:
+
+        def test_something(instance_caches):
+            # Test code here
+            fsspec.open("file://abc")
+            fsspec.open("memory://foo/bar")
+
+            # Test assertion
+            assert instance_caches.gather_counts() == {"file": 1, "memory": 1}
+
+    Returns
+    -------
+    instance_caches: An instance cache inspector for clearing and inspecting caches.
+    """
+    ic = InstanceCacheInspector()
+
+    ic.clear()
+    try:
+        yield ic
+    finally:
+        ic.clear()
+
+
+@pytest.fixture(scope="function")
 def ftp_writable(tmpdir):
     """
     Fixture providing a writable FTP filesystem.
     """
     pytest.importorskip("pyftpdlib")
-    from fsspec.implementations.ftp import FTPFileSystem
 
-    FTPFileSystem.clear_instance_cache()  # remove lingering connections
-    CachingFileSystem.clear_instance_cache()
     d = str(tmpdir)
     with open(os.path.join(d, "out"), "wb") as f:
         f.write(b"hello" * 10000)

fsspec/implementations/cached.py

@@ -61,6 +61,7 @@ class CachingFileSystem(ChainedFileSystem):
     """
 
     protocol: ClassVar[str | tuple[str, ...]] = ("blockcache", "cached")
+    _strip_tokenize_options = ("fo",)
 
     def __init__(
         self,

fsspec/implementations/tests/test_cached.py

@@ -1358,3 +1358,33 @@ def test_local_temp_file_put_by_list2(protocol, mocker, tmp_path) -> None:
     spy_put.assert_called_once_with([file.name], ["/some/file.txt"])
     # which avoids isdir() check
     spy_isdir.assert_not_called()
+
+
+def test_simplecache_tokenization_independent_of_path():
+    # check that the tokenization is independent of the path
+    of0 = fsspec.open("simplecache::memory://foo/bar.txt")
+    of1 = fsspec.open("simplecache::memory://baz/qux.txt")
+    assert of0.path != of1.path
+    assert of0.fs._fs_token_ == of1.fs._fs_token_
+    assert of0.fs is of1.fs
+
+
+def test_simplecache_instance_cache(instance_caches):
+    # check that the simplecache instance cache does not grow with every unique path
+
+    assert instance_caches.gather_counts() == {}
+
+    # check that the cache does not grow with multiple paths
+    fsspec.open("simplecache::memory://foo/bar.txt")
+    fsspec.open("simplecache::memory://bar/baz.txt")
+    fsspec.open("simplecache::memory://baz/qux.txt")
+    fsspec.open("simplecache::file:///foo/bar.txt")
+    fsspec.open("simplecache::memory://bar/baz.txt")
+    fsspec.open("simplecache::https://example.com/")
+
+    assert instance_caches.gather_counts() == {
+        "simplecache": 3,
+        "memory": 1,
+        "file": 1,
+        "http": 1,
+    }

fsspec/spec.py

@@ -67,6 +67,9 @@ def __call__(cls, *args, **kwargs):
         extra_tokens = tuple(
             getattr(cls, attr, None) for attr in cls._extra_tokenize_attributes
         )
+        strip_tokenize_options = {
+            k: kwargs.pop(k) for k in cls._strip_tokenize_options if k in kwargs
+        }
         token = tokenize(
             cls, cls._pid, threading.get_ident(), *args, *extra_tokens, **kwargs
         )
@@ -78,7 +81,7 @@ def __call__(cls, *args, **kwargs):
             cls._latest = token
             return cls._cache[token]
         else:
-            obj = super().__call__(*args, **kwargs)
+            obj = super().__call__(*args, **kwargs, **strip_tokenize_options)
             # Setting _fs_token here causes some static linters to complain.
             obj._fs_token_ = token
             obj.storage_args = args
@@ -115,6 +118,8 @@ class AbstractFileSystem(metaclass=_Cached):
 
     #: Extra *class attributes* that should be considered when hashing.
     _extra_tokenize_attributes = ()
+    #: *storage options* that should not be considered when hashing.
+    _strip_tokenize_options = ()
 
     # Set by _Cached metaclass
     storage_args: tuple[Any, ...]