-
Notifications
You must be signed in to change notification settings - Fork 133
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
feat: First-class caching #1104
Conversation
High-level:
A few more user-stories can help me evaluate the use-cases. Seems technically largely solid, although I would want instructions on extending (E.G. adding a global 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.
Lots of thoughts here, exciting stuff
hamilton/driver.py
Outdated
@@ -1696,6 +1709,14 @@ def validate_materialization( | |||
all_nodes = nodes | user_nodes | |||
self.graph_executor.validate(list(all_nodes)) | |||
|
|||
@property | |||
def cache(self) -> lifecycle.SmartCacheAdapter: |
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 this is user-facing add more comments (E.G. many people might not know that caching is implemented by an adapter, why might you want it...)
@@ -0,0 +1,52 @@ | |||
======================= | |||
Caching logic |
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.
Brief overview on what this section is
added docstring to base64 encode fingerprinting notebook section added caching code and example dev checkpoint added Fingerprint class first-class caching v0.1 fixed tests updated module_6 test case updated docstrings documentation WIP added refactor file structure add direct access to cache via Driver remove Support Parallelizable/Collect; removed Fingerprint Refactored the logic of the CacheAdapter to have explicit operations. Many challenges arise from `Parallel` nodes that return sequence of elements, but actually the data version of individual elements is what matters. Also, `Collect` can have difficulties access upstream data version if these were computed in other threads/processes. The `Fingerprint` construct was removed because it obfuscated what information was relevant to pass around. It's name is also less evocative than "data version" and "code version". made SQLMetadataStore threadsafe added structured logging; move class to hamilton.lifecycle.caching structured log; data saver; cache decorator added from_string to CachingBehavior enum added docs and mermaid graph support fixed variable renaming issue updated docs requirements; adapted typing to 3.8 reverted Sequence type to Sequence class for singledispatch fixed typo added deprecation warnings to other caching methods fixed missing kwarg for recursive data versioning output structured log to file updated expand nodes handling; added ignore behavior updated caching behaviors and admonitions updated Sequence import for 3.8 support fixed bugs: sentinel values, log printing, failed nodes updated docs updated docstring; fixed materialization with parallel fixed 3.8 typing guard against setting cache twice registered separate function for versioning bytes refactored adapter to use internal hook replace HamiltonGraph by FunctionGraph add _get_node_role() method switched to internal hooks pre/post node refactored to key on run_id; refactor result_store refactored sqlitestore to hamilton.stores fixed type annotations fixed docs reference and docstrings updated result_store tests fixed materialization from result_store added roadmap to docs refactored to move to hamilton/caching fingerprinting.set_max_depth() added changed cache decorator to a class updated all docstrings for SmartCacheAdapter renamed context_key to cache_key added deprecation warnings using logging improved warning message added TODO
2c244d4
to
998ca73
Compare
Looking good! Does this work with adapters like ray, or the graceful error one? |
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.
Looking great! A few points on documentation. Caveats + testing:
- mlflow adapter
- hamilton UI (curious what this looks like)
But yeah, gotta get this out, merge tonight/tomorrow morning?
from hamilton.caching.stores.base import MetadataStore | ||
|
||
|
||
class SQLiteMetadataStore(MetadataStore): |
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.
Comments on this -- it'll be something people want to look into for debugging
import pathlib | ||
|
||
|
||
def get_directory_size(directory: str) -> float: |
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.
Comments
hamilton/driver.py
Outdated
@@ -1905,6 +1931,11 @@ def with_adapters(self, *adapters: lifecycle_base.LifecycleAdapter) -> "Builder" | |||
:param adapter: Adapter to use. | |||
:return: self | |||
""" | |||
if any(isinstance(adapter, SmartCacheAdapter) for adapter in adapters): |
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.
nit, but we should store cache then add it to the adapters later I think?
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.
Not a blocker
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.
Looking good! Will glance over when we sync tomorrow, then approve + merge.
# solution for `@dataloader` and `from_` | ||
if behaviors.get(main_node, None) is not None: | ||
behaviors[node.name] = behaviors[main_node] | ||
# this hacky section is required to support @load_from and provide |
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.
Hmm, not sure I follow what's happening here. Let's add some more docs.
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.
Specifically, document desired behavior here/how the code maps to it
data_version = self._version_data(node_name=node_name, run_id=run_id, result=result) | ||
|
||
# nodes collected in `._data_savers` return a dictionary of 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.
I'm not convinced we need to special case these... But for now, let's call this a bit experimental (data savers/loaders) -- it's a very odd case that we don't want ot dwell onmore.
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.
Well done!
This follows-up the closed PR #1039
Current TODOs:
Other TODOs:
result_store
andmetadata_store
(closer to S3 lingo),fingerprinting
,fingerprint
run_id
. Passing arun_id
string orlatest
toresult_from
should do the trickStore
subclasses. It's currently a bit confusing thatresult_store
andmetadata_store
both inherit from the sameBaseStore
but they have different method signatures forget
andset
to.file
or other destinations where the suffix doesn't match the ideal file extension.path