[REF-1885] Shard Substates when serializing to Redis #2574
Conversation
Avoid leaking sharding implementation details all over the State class and breaking the API
ensure that we don't end up with a broken tree
refactor upload tests to suck less
reflex/state.py
Outdated
| @@ -1405,6 +1419,24 @@ def on_load_internal(self) -> list[Event | EventSpec] | None: | |||
| type(self).set_is_hydrated(True), # type: ignore | |||
| ] | |||
|
|
|||
| def __getstate__(self): | |||
There was a problem hiding this comment.
This leaves router_data on each substate. Is that the intended behavior?
There was a problem hiding this comment.
For now, yes. But I might be able to get away with stripping it out.
There was a problem hiding this comment.
Maybe router_data turns out to be a substate on its own in the end 😄
There was a problem hiding this comment.
@abulvenz not a bad idea actually.. it may involve having to save two states each time as a downside, but it might be a cleaner separation in the future.
masenf
force-pushed
the
masenf/state-shard-redis2
branch
from
cdf4593
to
498153d
Compare
| @@ -1002,7 +1002,8 @@ async def upload_file(request: Request, files: List[UploadFile]): | |||
| ) | |||
|
|
|||
| # Get the state for the session. | |||
| state = await app.state_manager.get_state(token) | |||
| substate_token = token + "_" + handler.rpartition(".")[0] | |||
There was a problem hiding this comment.
nit: maybe we can pull this out into a function get_substater_token(token, handler)
| Returns: | ||
| The substate token. | ||
| """ | ||
| substate = self.name.rpartition(".")[0] |
There was a problem hiding this comment.
if we pull it out, then we can reuse the function here
| @@ -1479,6 +1511,8 @@ async def __aenter__(self) -> StateProxy: | |||
| """ | |||
| self._self_actx = self._self_app.modify_state( | |||
| self.__wrapped__.router.session.client_token | |||
| + "_" | |||
There was a problem hiding this comment.
same comment about pulling this out
There was a problem hiding this comment.
adding a utility for this in the followup PR
| parent_state_key, top_level=False, get_substates=False | ||
| ) | ||
| # Set up Bidirectional linkage between this state and its parent. | ||
| if parent_state is not None: |
There was a problem hiding this comment.
this can just be an else:
There was a problem hiding this comment.
i don't think so, because we may or may not set parent_state in the previous condition (for example, when retrieving the base state of the app, parent_state will still be None as this point, even though we entered the previous condition)
This is a performance optimization for apps that have many smaller states that end up taking excessive time to serialize/deserialize for every event, like reflex-web.
Instead of only serializing the top-level
rx.Stateand implicitly all substates by virtue of their presence in theself.substatesdictionary, theStateManagerRedisapplies special logic to persist substates into their own keys. When retrieving a particular state, it's parent and all of it's substates (recursively) are fetched from redis and assembled into a structure resembling the previous implementation. The main difference is that when using redis, sibling states are not directly accessible. This will be addressed in a followup patch, that allows arbitrary access to any state defined in the app.Fixes #2554