-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
iter states #3769
base: main
Are you sure you want to change the base?
iter states #3769
Conversation
benedikt-bartscher
commented
Aug 9, 2024
•
edited
Loading
edited
eece195
to
f4295df
Compare
@masenf do you have any idea if there is a cool way to automatically detect if |
We could also add a |
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.
Sorry for the delay here. I think the changes look good - we need to rebase this onto main
and implement it for StateManagerDisk
which is becoming the default in 0.7.0
No worries, I just implemented it for |
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.
Code works well for me. I'll wait for @masenf 's final approval to merge. What was the use case for this?
You can use it for cache invalidation, f.e. to have all blog pages as infinitely cached computed vars and invalidate/reload them globally if a page changes. Maybe we could later use it for some kind of state migration. |
Will rebase if there is intention to merge this |
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.
the main problem i forsee with this feature is that it works great in testing/dev mode, and then once it's deployed prod with redis, some clients are not going to get deltas sent when their state is modified, because they're connected to a different instance of the app than where this API is used.
it's not a new problem, we've had it for a long time. but currently there is no mechanism for an app instance to send a delta to a client that is connected to a different instance. @Lendemor had a WiP to handle this, but i think it's closed without merging.
@@ -279,6 +282,18 @@ def get_app(reload: bool = False) -> ModuleType: | |||
raise | |||
|
|||
|
|||
def get_app(reload: bool = False) -> App: |
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 we make this new function get_app_instance
and leave get_app
the way it was, to retain as much compatibility as possible
The state names. | ||
""" | ||
for path in self._iter_pkl_files(): | ||
token = path.stem |
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.
unfortunately this wont work anymore, because we started hashing the token to avoid file path limits on windows.
however, we should be able to use the same implementation here as we do for StateManagerMemory
as the on-disk pickles are only read when the backend starts after a hot reload
Marking this as draft until ready for review. |
Thanks for the review. Yes, that's currently a limitation, but i think it can be implemented with redis pub/sub and some maps. |