-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add key_storage
, isolation_level
and lock_manager
configuration to cache policies
#15382
Conversation
storage
, isolation_level
and
locks` configuration to cache policiesstorage
, isolation_level
and locks
configuration to cache policies
storage
, isolation_level
and locks
configuration to cache policiesstorage
, isolation_level
and locks
configuration to cache policies
CodSpeed Performance ReportMerging #15382 will not alter performanceComparing Summary
|
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.
One note on locks
as a kwarg name, otherwise this is great
@@ -195,7 +195,9 @@ def __enter__(self): | |||
and not self.store.supports_isolation_level(self.isolation_level) | |||
): | |||
raise ValueError( | |||
f"Isolation level {self.isolation_level.name} is not supported by provided result store." | |||
f"Isolation level {self.isolation_level.name} is not supported by provided " |
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.
note for future: I think we should refactor many of these errors into special prefect.exceptions.ConfigurationError
exceptions for clarity and consistency
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.
this looks awesome -- left one small comment about the deepcopy
thing, and one question that isn't super important but I wanted to give it some airtime just in case. cc: @zzstoatzz in case you have thoughts about the kwarg name storage
"IsolationLevel", | ||
None, | ||
] = None | ||
lock_manager: Optional["LockManager"] = 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.
yea it's longer but I do like this more - very clear
src/prefect/cache_policies.py
Outdated
|
||
@dataclass | ||
class CachePolicy: | ||
""" | ||
Base class for all cache policies. | ||
""" | ||
|
||
storage: Union["WritableFileSystem", str, Path, None] = 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.
Only because now is the time to ask these questions - are there any other name options we have for this one, just to make crystal clear it's not the same thing as result storage?
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.
Another possibility could be cache_storage
. It'd be a tad redundant, but it'd be explicit.
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.
A few other ideas just to explore our options:
key_storage
metadata_storage
(mirrors the internal API)key_files
metadata_files
cache_files
cache_data
metadata
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.
key_storage
jumps out as a good option to me. I think I'll change to that.
storage
, isolation_level
and locks
configuration to cache policieskey_storage
, isolation_level
and lock_manager
configuration to cache policies
This PR adds the ability to specify a storage location for cache entries via a
CachePolicy
. The isolation level for the cache and locking mechanism are also configurable via aCachePolicy
.Example
Cache task results based on the input and source code a task. Store those cache entries at
~/.prefect/cache
. Allow only one actor to access a cache entry at a time and store lock files to enforce that at~.prefect/locks
.Checklist
<link to issue>
"mint.json
.