-
-
Notifications
You must be signed in to change notification settings - Fork 364
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 our terms of service and privacy 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
Conversation
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. I imagine 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.pop
instead of anif key in dict
followed 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 arbitrary value
here? Is it possible to scope this to just Buffer
objects (or maybe Buffer
and NDBuffer
)?
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 with Buffer
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 with self._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 accept Buffer
here, then buffer_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 the Buffer
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 the cache_store
(in this example the MemoryStore). Otherwise the enclosing CacheStore
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 a LocalStore
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 separate CacheStores
.
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 fresh MemoryStore
, 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 an LRUStore
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 implement LRUStore
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
CacheStore
class - Instead all the logic for caching is implemented on
Store
, and there is acache_store
property 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 of experimental
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.
@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! |
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
CacheStore
This 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/*.rst
changes/