-
Notifications
You must be signed in to change notification settings - Fork 448
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
feature: allow persistent_cache to be used as a decorator #3550
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Clean implementation! Couple questions on signature changes.
```python | ||
with persistent_cache(name="my_cache"): | ||
variable = expensive_function() # This will be cached to disk. | ||
print("hello, cache") # this will be skipped on cache hits | ||
``` |
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.
Can you add a decorator example as well?
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.
Let me know what you think of the overloading? Could walk it back a little too
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 being said, I added examples in some of the overload docs)
|
||
def cache( | ||
name: Union[str, Optional[Callable[..., Any]]] = None, | ||
*args: 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.
Why change the signature to use varargs/varkwargs?
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.
IIUC this can dispatch to either _cache_call
or _cache_context
, but they take different args/kwargs, so variability is needed. Rather than using Any
, tho, I think this could have more specific type annotations using unpack and a union those two signatures' args/kwargs?
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.
Any
is pretty consistent with other uses of *
packing in the codebase, but I did leave a comment, since *
is more to keep values ambiguous until they're fed into the correct constructors
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, to be clear, wasn't saying that needed to happen, just that more specific typing was an option if desired -- while a nice addition, unpack
is pretty recent and not super widely used (in marimo and beyond, ime)
Thanks for your work on this! I'm trying this branch out on a couple long computations rn 🚀
marimo/_save/save.py
Outdated
*args: Any, | ||
**kwargs: 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.
Same question, why varargs/kwargs?
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.
In addition to matching the signature throughout, extra kwargs
are now used by Loaders to enable:
with MyLoader.cache("my_cache", constructor_values=123) as c:
...
or
@MyLoader.cache
def my_fn(args):
...
a bit of feature creep, but even when testing these changes, I noticed how cumbersome
with cache(_loader=MyLoader.partial(constructor_values=123)) as c:
...
was, and seemed like low hanging fruit.
marimo/_save/save.py
Outdated
@singledispatch | ||
def _cache_invocation( | ||
arg: Any, | ||
loader: Union[LoaderPartial, Loader, LoaderType], | ||
*args: Any, | ||
frame_offset: int = 1, | ||
**kwargs: Any, | ||
) -> Union[_cache_call, _cache_context]: | ||
del loader, args, kwargs, frame_offset | ||
raise TypeError(f"Invalid type for cache: {type(arg)}") |
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.
Pretty clean!
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.
Cheers!
Tried to throw in some overloading to clean up the type signatures. From a comment I left:
That being said, overloading seems to do interesting things in Jedi. Cursor over persistent cache gives the implementation doc string But opening it as a function gives the first overload Also unsure what will happen when docs tries to deploy this. Updated docs are a little half baked (but acceptable) until we decide how we want to handle this, or whether the signature hacks are going a bit too far. |
Hmm, the docs aren't rendered using For completion, I think I wouldn't worry too much about what how |
I was saying showing both doc strings in the documentation panel. |
But yes, that makes sense to me. |
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 happy with the overloading approach! LGTM modulo moving main docstrings to the implementation, if that's what you think is best.
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.10.17-dev12 |
📝 Summary
fixes #2653 #3471
🔍 Description of Changes
Enables
persistent_cache
to be used as a decorator for functions, andcache
to be used as a context block. e.g.cache
is also used as the general entry point for custom "Loaders"The breadth of the API makes the implementation a bit hairy, but I think that if it's a smooth experience for the user then it's worth it.
@akshayka