Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork366
CacheStore containing source store and cache store#3366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
CacheStore containing source store and cache store#3366
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedAug 11, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #3366 +/- ##==========================================+ Coverage 61.25% 61.86% +0.61%========================================== Files 84 85 +1 Lines 9949 10110 +161 ==========================================+ Hits 6094 6255 +161 Misses 3855 3855
🚀 New features to boost your workflow:
|
| await self._cache.delete(key) | ||
| self._remove_from_tracking(key) | ||
| def cache_info(self) -> dict[str, Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it would be great to use a typeddict here, so that the return type is known
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the PR. I left a few small comments, but a couple bigger picture things:
- This is perhaps the best demonstration for why#2473 (statically associating a Store with a Buffer type) might be helpful. Iimagine that we don't want to use precious GPU RAM as a cache.
- Do we care about thread safety here at all? Given that the primary interface to concurrency in Zarr is async, I think we're probably OK not worrying about it. But there might be spots where we can do operations atomically (e.g. using
dict.popinstead of anif key in dictfollowed by the operation) with little cost. Trying to synchronize changes to multiple dictionaries would require much more effort.
src/zarr/storage/_caching_store.py Outdated
| except Exception as e: | ||
| logger.warning("_evict_key: failed to evict key %s: %s", key, e) | ||
| def _cache_value(self, key: str, value: Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why are we accepting arbitraryvalue here? Is it possible to scope this to justBuffer objects (or maybeBuffer andNDBuffer)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
At a glance, it looks like we only call this withBuffer so hopefully this is an easy fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
src/zarr/experimental/cache_store.py Outdated
| if key in self._cache_order: | ||
| del self._cache_order[key] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know if this is called from multiple threads, but this could be done atomically withself._cache_order.pop(key, None).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
src/zarr/experimental/cache_store.py Outdated
| def _cache_value(self, key: str, value: Any) -> None: | ||
| """Cache a value with size tracking.""" | ||
| value_size = buffer_size(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we only acceptBuffer here, thenbuffer_size can be removed hopefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
buffer_size is now removed, as we are using theBuffer type
docs/user-guide/cachingstore.rst Outdated
| >>> cached_store = zarr.storage.CacheStore( | ||
| ... store=source_store, | ||
| ... cache_store=cache_store, | ||
| ... max_size=256*1024*1024 # 256MB cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for this PR. I think it would be better to have the LRU functionality on thecache_store (in this example the MemoryStore). Otherwise the enclosingCacheStore would need to keep track of all keys and their access order in the inner store. That could be problematic if the inner store would be shared with other CacheStores or other code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That could be problematic if the inner store would be shared with other CacheStores or other code.
As long as one of the design goals is to use a regular zarr store as the caching layer, there will be nothing we can do to guard against external access to the cache store. For example, if someone uses aLocalStore as a cache, we can't protect the local file system from external modification. I think it's the user's responsibility to ensure that they don't use the same cache for separateCacheStores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but our default behavior could be to create a freshMemoryStore, which would be a safe default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My main concern here is about the abstraction. The LRU fits better in the inner store than in the CacheStore, imo. There could even be anLRUStore that wraps a store and implements the tracking and eviction.
The safety concern is, as you pointed out, something the user should take care of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My main concern here is about the abstraction. The LRU fits better in the inner store than in the CacheStore, imo.
That makes sense, maybe we could implementLRUStore as another store wrapper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it would be better to have the LRU functionality on the cache_store (in this example the MemoryStore)
To understand, is the suggestion here that
- There's no
CacheStoreclass - Instead all the logic for caching is implemented on
Store, and there is acache_storeproperty that can be set to a second store to enable caching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@normanrz unless you have a concrete proposal for a refactor that would be workable in the scope of this PR, I would suggest we move forward with the PR as-is, and use experience to dial in the abstraction in a later PR.
But knowing that design here might change, I think we should introduce an "experimental" storage module, e.g.zarr.storage.experimental, or a top-level experimental module,zarr.experimental, and put this class there until we are sure that the API is final.
Thoughts? I would like to ship this important feature while also retaining the ability to safely adjust it later. An experimental module seems like a safe way to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just commenting that I'd love to have this included sooner rather than later, it will be immediately useful 🎉 Thanks@ruaridhg for taking the initiative and putting this together!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice use ofexperimental here! 🤩
…into rmg/cache_remote_stores_locally
…gic, and avoid calling exists unnecessarily
…t have doctests working)
@ruaridhg I pushed a bunch of changes, please have a look before I merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
2eb89e1 intozarr-developers:mainUh oh!
There was an error while loading.Please reload this page.
@d-v-b Thanks for making changes and merging. I'm on another project with a tight deadline so haven't had time to look at this, much appreciated! |
Uh oh!
There was an error while loading.Please reload this page.
Closes#3357
Store containing 2 stores i.e. primary store (where the data is coming from) and a cache store (where you want to store the data as a cache).
Introduces the class
CacheStoreThis cache wraps any Store implementation and uses a separate Store instance as the cache backend. This provides persistent caching capabilities with time-based expiration, size-based eviction, and flexible cache storage options.
TODO:
docs/user-guide/*.rstchanges/