Cached widget replay #5298
Cached widget replay #5298
Conversation
Lots of internal docs writing that brought up a number of questions, because I realized a small flaw in the original design and followed the implications.
Not obviously worth it, but easily changed later if desired
No idea why it's this dumb
Otherwise there may be exceptions from lookup failures when reconnecting to a running app. We could instead catch the exception somewhere and treat it as a cache miss, but this way seems more robust, because it handles a conditionally created widget that is not currently being used by allowing caching to still happen based on the other widgets.
It is tripping up the integration test job
tconkling
left a comment
There was a problem hiding this comment.
This looks thoughtful and thorough, BUT it also feels a bit write-only. That is: it's very complex, and it's hard for me (a new reader of the code), to find a way into that complexity, and to understand what's going on and why.
I have a couple general concerns
- Lots of methods are missing docstrings, and also don't have unit tests backing them up, so it's really hard to know "what is the contract this method is making and how do I know if I've broken it". Like, I don't feel confident I could make changes in here.
- Less critically, some of the naming here isn't helping the situation.
In between now and when this is ready to land, can we give any non-obvious, non-trivial method gets a docstring and enough unit tests to make their meaning clear to new readers? (I'm happy to help with this if you'd like to pair on it!)
|
About what I expected. I hadn't spent any time on documenting it since the initial high level overview. Thanks for checking over it, I'll let you know when it's ready for another round. |
The cache contextmanager stack of doom grows
Initial is misleading, and while Multi is a bit vague, that is preferable. Coming up with a good name for this is hard.
tconkling
left a comment
There was a problem hiding this comment.
Looks good! I had a hard time determining this from reading the code, so I want to verify some behavior:
Let's say we have a cached function with disk persistence enabled. We get a MultiCacheResults hit for our function params, but our particular CachedResult isn't yet computed. After it's computed and we update MultiCacheResults, will that updated results object be written back to disk? (In other words: do we properly write mutated MultiCacheResults back out to disk?)
| if self.persist == "disk": | ||
| try: | ||
| multi_cache_results = self._read_multi_results_from_disk_cache(key) | ||
| except CacheKeyNotFoundError: |
Check notice
Code scanning / CodeQL
Empty except
Typechecking plz save me
📚 Context
Replay of cached elements, but for widgets
What kind of change does this PR introduce?
🧠 Description of Changes
Convert memo and singleton to 2 stage caches that treat widget values as inputs
Dynamically track widgets seen when executing a cached function, to allow the above to work
Replay widget registration and messages from cached function results
🧪 Testing Done