Add optional file-based listings caching #895

gutzbenj · 2022-01-25T22:10:37Z

Dear @martindurant and fsspec team,

this PR adds an additional file-based listings cache with the help of the beautiful diskcache[1]. This is relevant as the listings cache docs mention that the creation of certain large listings can be really expensive.

Following changes are done:

rename memdircache.py module to dircache.py to represent general diraching (listings caching) and add argument to choose from both (MemDirCache is set to default)
rename DirCache class to MemDirCache
introduce new FileDirCache that implements all abstractmethods using given methods/attributes of diskcache.Cache class[2] and places the cache at a given location (if wanted)
parametrize listings cache tests to test both MemDirCache and FileDirCache
adjust relevant documentation

Please check it out and let me know for required changes!

Cheers
Benjamin

[1] http://www.grantjenks.com/docs/diskcache/
[2] http://www.grantjenks.com/docs/diskcache/api.html#diskcache.Cache

#787

CC @amotl

amotl · 2022-01-28T19:24:34Z

Dear Benjamin,

I want to thank you very much for tackling this improvement to fsspec. In order to shed more light on the background of this, I would like to reference earthobservations/wetterdienst#243, where we are coming from.

This improvement solves a longstanding issue for us, as the dbmfile-based backend of dogpile.cache proved to be too brittle when operating the Wetterdienst library in concurrent workload situations. I used to work with both Beaker and dogpile.cache in the past and they really shine in environments where you can easily afford to run a proper database-based cache backend, like Memcached, Redis, or even MongoDB.

However, this is not the situation with Wetterdienst: It is mostly library- and commandline-driven, so we didn't want to impose the overhead of running a caching database on its users.

As discussed at earthobservations/wetterdienst#243 (comment), I was very happy to learn about DiskCache by @grantjenks (thank you!) from you, as its features promised it to be the absolute right thing to use in a library context.

On my ramblings

It might even be suitable to accompany the fsspec-based caching implementation fsspec/caching.py.

@martindurant answered:

Certainly, fsspec would be happy to see alternative, more fully-featured caching schemes. Integration should generally be pretty simple.

In this manner, thank you again for making a start with this patch. I hope the integration will go along well.

With kind regards,
Andreas.

Further references

I discovered those resources in the issue tracker, which also indicate that the current fsspec/caching.py might have shortcomings in certain situations. Maybe those routines can also be improved by bringing DiskCache to the table?

@nbren12 reported about race conditions in fsspec cache, see race conditions in fsspec cache? #639.
@martindurant took a note »directory and path utilities: make multiple pluggable dircache implementations, including persistence« at Towards a roadmap #787.
Use cases like outlined within Deadlock involving multiprocessing/pandas read_excel #517 or Deadlock involving pandas.read_excel and AWS S3 #890 might also profit from a both process- and thread-safe disk-based cache implementation.

nbren12 · 2022-01-28T20:16:48Z

Should/is this feature disabled by default? I couldn't tell from the code what the default behavior is. I could see it being pretty confusing if not. E.g. an application that monitors for changes in a bucket using fs.ls. Users are always free to wrap operations in their own caching layer no? e.g.

from toolz import memoize
# in memory
ls = memoize(fsspec.ls)
# on disk
from joblib import Memory
cache = Memory(".cache")
ls = cache.cache(fs.ls)

martindurant · 2022-01-28T20:28:11Z

The dircache is used for more operations than simply caching the output of ls, so I don't think the simple approach above does everything you want.

nbren12 · 2022-01-28T20:37:48Z

I'm mainly just cautioning against caching by default...is it a default?

martindurant · 2022-01-28T20:39:27Z

Yes, we cache file listings for remote filesystems, where getting such listings can be pretty expensive.

nbren12 · 2022-01-28T22:11:26Z

So this is the expected default behavior:

>>> fs.ls("gs://some-bucket")
[]
>>> fs.put("local_file", "gs://some-bucket")
>>> fs.ls("gs://some-bucket")
[]

I could imagine losing hours debugging something like that.

martindurant · 2022-01-28T22:22:50Z

put is supposed to explicitly invalidate the parts of the cache it touches, so this is a bug to be fixed.

nbren12 · 2022-01-28T22:38:21Z

I'm not sure it is a bug. I didn't run the code above...I just meant it as an illustrative example. Suppose instead of fsspec.put someone used another tool or that they want to monitor changes made by other machines. The cache can easily become out of sync with reality.

martindurant · 2022-02-01T14:21:43Z

Agreed, the cache can get out of sync when processes are happening elsewhere, so we have the ability to force refresh, set an expiry time or turn the listings cache off. However, the default is to cache for the duration of the python session, which seems (to me) to align with the most common use cases.

ci/environment-win.yml

gutzbenj · 2023-10-09T22:17:43Z

Dear all,

finally updated the PR. Could you please give me a review?

amotl · 2023-10-10T12:56:52Z

Hi Benjamin and Martin,

there is another discussion over at #1127, which I wanted to make you aware about. It may happen that this patch will get rejected, or not merged in its current form.

fsspec is very careful not to require any dependencies, DiskCache should only be optional.

If @martindurant agrees with my evaluation that it is indeed unlikely that it will get merged, I am thinking about shipping it on behalf of a separate utility package, in order to overcome the roadblock in making the recipe reusable beyond the Wetterdienst package, where it is currently used.

What do you think about this proposal?

With kind regards,
Andreas.

martindurant · 2023-10-12T18:08:10Z

docs/source/features.rst

+skip the caching altogether, use ``use_listings_cache=False``. That would be appropriate
+when the target location is known to be volatile because it is being written to from other
+sources. If you want to use the file-based caching, you can also provide the argument
+``listings_cache_location`` to determine where the cache file is stored.


Do you think there's anything that can be done at this stage to simplify all these parameters? Should we have a caching_options dict, or config options (or both) ?

Dear @martindurant ,

I updated the configurations and put them all into one dict named listings_cache_options to make clear what it refers to.

Probably options for [data] caching and listings caching should not be put together to keep it simple and because keywords e.g. "expiriy_time" are similarly named.

fsspec/dircache.py

martindurant · 2023-10-12T18:09:28Z

fsspec/dircache.py

+        listings_expiry_time = listings_expiry_time and float(listings_expiry_time)
+
+        if listings_cache_location:
+            listings_cache_location = Path(listings_cache_location) / str(


Why would we use Path within a filesystem library :)

Can diskcache handle fsspec paths? E.g., memory://cache would be a very similar solution to MemDirCache. Such an option might even be useful for semi-speedy remotes like smb:

fsspec/dircache.py

martindurant · 2023-10-12T18:13:33Z

fsspec/implementations/http.py

@@ -103,6 +103,8 @@ def __init__(
        request_options.pop("listings_expiry_time", None)
        request_options.pop("max_paths", None)
        request_options.pop("skip_instance_cache", None)
+        request_options.pop("listings_cache_type", None)


This is problematic - where else might these kwargs pollute calls? Do we need to change the signatures of all backends?

fsspec/implementations/tests/test_http.py

fsspec/spec.py

tox.ini

martindurant · 2023-10-12T18:16:56Z

Given progress here, I think we can fix this up, and see whether it garners any interest.

gutzbenj · 2024-03-18T17:14:24Z

fsspec/asyn.py

@@ -297,15 +297,15 @@ class AsyncFileSystem(AbstractFileSystem):
    mirror_sync_methods = True
    disable_throttling = False

-    def __init__(self, *args, asynchronous=False, loop=None, batch_size=None, **kwargs):
+    def __init__(self, *args, asynchronous=False, loop=None, batch_size=None, listings_cache_options=None, **kwargs):


Should we name it dir_cache_options instead to adhere with MemoryDirCache/FileDirCache?

fsspec/dircache.py

gutzbenj · 2024-03-18T23:05:20Z

fsspec/spec.py

+        if not listings_cache_options:
+            listings_cache_options = {}
+        elif listings_cache_options is True:
+            listings_cache_options = {"cache_type": CacheType.MEMORY, "expiry_time": None}
+        else:
+            listings_cache_options = {"expiry_time": None} | listings_cache_options
+            cache_type = listings_cache_options.get("cache_type")
+            if cache_type:
+                if isinstance(cache_type, str):
+                    try:
+                        cache_type = CacheType[cache_type.upper()]
+                    except KeyError as e:
+                        raise ValueError(
+                            f"Cache type must be one of {', '.join(ct.name.lower() for ct in CacheType)}") from e
+                listings_cache_options["cache_type"] = cache_type


I think there's too much going on here!

I think it's fine! It could be a norm_cache_options util functions to pull it out of here.

martindurant · 2024-03-26T14:11:20Z

>           self.dircache[path.rstrip("/")] = files
E           TypeError: 'NoneType' object does not support item assignment

It seems like the "no cache" case should still have a no-op dict to cope with this kind of thing. So, there would be a third cache type, one that corresponds to the use_listing_cache = False from before.

martindurant · 2024-04-22T19:53:51Z

@amotl @gutzbenj : this PR seems to have stalled. I know there's quite a thread above, but what do you think we can do from here to get this useful functionality in?

gutzbenj · 2024-04-28T07:04:52Z

Dear @martindurant ,

thanks for the reminder. I think I need just a bit of time. I will work on this in about 2 weeks again and push it actively.

gutzbenj · 2024-05-10T17:02:05Z

Dear @martindurant ,

still on it...

Currently some tests are failing which I believe I'm not responsible for. Maybe some reallife backend has changed or so.

martindurant · 2024-05-13T15:26:49Z

This seems like it might be real:

FAILED fsspec/implementations/tests/test_http.py::test_list_cache_memory[None] - assert (0.002689361572265625 / 0.0018138885498046875) > 1.5

Could it be that the file cache is causing a significant slow-down? Perhaps we just need to alter the test a bit to allow for a smaller speedup factor like 1.1 .

gutzbenj force-pushed the add-local-listings-cache branch 6 times, most recently from f990b3e to 2eb120d Compare January 26, 2022 17:12

amotl mentioned this pull request Jan 28, 2022

Get rid of dbmfile-based dogpile cache earthobservations/wetterdienst#243

Closed

amotl mentioned this pull request Dec 25, 2022

Wrap cache-file-management operations for CachingFileSystem in thread locks #1127

Draft

amotl reviewed Dec 25, 2022

View reviewed changes

ci/environment-win.yml Outdated Show resolved Hide resolved

gutzbenj force-pushed the add-local-listings-cache branch 2 times, most recently from a151320 to 17e926e Compare October 9, 2023 22:03

martindurant reviewed Oct 12, 2023

View reviewed changes

gutzbenj mentioned this pull request Mar 7, 2024

Fix AsyncFileSystem monkeypatching breaking 3rd-party file systems earthobservations/wetterdienst#1241

Merged

gutzbenj force-pushed the add-local-listings-cache branch 3 times, most recently from 4dad114 to 58165e4 Compare March 17, 2024 22:03

gutzbenj commented Mar 18, 2024

View reviewed changes

fsspec/dircache.py Show resolved Hide resolved

gutzbenj commented Mar 18, 2024

View reviewed changes

gutzbenj force-pushed the add-local-listings-cache branch 21 times, most recently from 029d267 to eb42007 Compare May 10, 2024 16:40

Add optional file-based listings caching

6133acb

gutzbenj force-pushed the add-local-listings-cache branch from eb42007 to 6133acb Compare May 10, 2024 17:00

Use fsspec local filesystem instead of pathlib

31da73d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional file-based listings caching #895

Add optional file-based listings caching #895

gutzbenj commented Jan 25, 2022 •

edited

Loading

amotl commented Jan 28, 2022 •

edited

Loading

nbren12 commented Jan 28, 2022 •

edited

Loading

martindurant commented Jan 28, 2022

nbren12 commented Jan 28, 2022

martindurant commented Jan 28, 2022

nbren12 commented Jan 28, 2022

martindurant commented Jan 28, 2022

nbren12 commented Jan 28, 2022

martindurant commented Feb 1, 2022

gutzbenj commented Oct 9, 2023

amotl commented Oct 10, 2023 •

edited

Loading

martindurant Oct 12, 2023

gutzbenj Mar 18, 2024 •

edited

Loading

martindurant Oct 12, 2023

martindurant Oct 12, 2023

martindurant Oct 12, 2023

martindurant commented Oct 12, 2023

gutzbenj Mar 18, 2024

gutzbenj Mar 18, 2024

martindurant Mar 26, 2024

martindurant commented Mar 26, 2024

martindurant commented Apr 22, 2024

gutzbenj commented Apr 28, 2024

gutzbenj commented May 10, 2024

martindurant commented May 13, 2024

Add optional file-based listings caching #895

Are you sure you want to change the base?

Add optional file-based listings caching #895

Conversation

gutzbenj commented Jan 25, 2022 • edited Loading

amotl commented Jan 28, 2022 • edited Loading

Further references

nbren12 commented Jan 28, 2022 • edited Loading

martindurant commented Jan 28, 2022

nbren12 commented Jan 28, 2022

martindurant commented Jan 28, 2022

nbren12 commented Jan 28, 2022

martindurant commented Jan 28, 2022

nbren12 commented Jan 28, 2022

martindurant commented Feb 1, 2022

gutzbenj commented Oct 9, 2023

amotl commented Oct 10, 2023 • edited Loading

martindurant Oct 12, 2023

Choose a reason for hiding this comment

gutzbenj Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

martindurant Oct 12, 2023

Choose a reason for hiding this comment

martindurant Oct 12, 2023

Choose a reason for hiding this comment

martindurant Oct 12, 2023

Choose a reason for hiding this comment

martindurant commented Oct 12, 2023

gutzbenj Mar 18, 2024

Choose a reason for hiding this comment

gutzbenj Mar 18, 2024

Choose a reason for hiding this comment

martindurant Mar 26, 2024

Choose a reason for hiding this comment

martindurant commented Mar 26, 2024

martindurant commented Apr 22, 2024

gutzbenj commented Apr 28, 2024

gutzbenj commented May 10, 2024

martindurant commented May 13, 2024

gutzbenj commented Jan 25, 2022 •

edited

Loading

amotl commented Jan 28, 2022 •

edited

Loading

nbren12 commented Jan 28, 2022 •

edited

Loading

amotl commented Oct 10, 2023 •

edited

Loading

gutzbenj Mar 18, 2024 •

edited

Loading