chore: linking ComponentManager to session #248
Conversation
Making ComponentManager a part of session in order to link the state of the UI to each particular session instead of the whole app.
|
Whoah, checks don't look good 😢 |
src/streamsync/core.py
Outdated
| @@ -723,7 +724,8 @@ class Evaluator: | |||
|
|
|||
| template_regex = re.compile(r"[\\]?@{([\w\s.]*)}") | |||
|
|
|||
| def __init__(self, session_state: StreamsyncState): | |||
| def __init__(self, session_id: str, session_state: StreamsyncState): | |||
| self.session_id = session_id | |||
There was a problem hiding this comment.
Why are we passing an id instead of following what was done with session_state and passing component like session_components?
There was a problem hiding this comment.
The id is currently needed to access component manager inside session(s) – previously, Evaluator used "application-wide" ComponentManager in the evaluate_field method; to replace it with session-linked manager, we first need to know what session we're working on specifically. I might've misunderstood the purpose of the StreamsyncState, but I couldn't see a way to utilize it for this.
There was a problem hiding this comment.
But what I mean is, follow exactly what has been done with State. Rather than getting the session by id and then getting the component manager, pass the reference to the component manager when instantiating the Evaluator, similarly to how we pass session_state.
There was a problem hiding this comment.
Ah, I see, I've misunderstood your comment, apologies.
Good point, as session context is accessible during the creation of Evaluator, we might as well just pass the manager, without having the overhead of resolving it each time. I'll adjust it.
There was a problem hiding this comment.
I would advocate to keep the ComponentManager as high level class and to store inside the list of components associate with a session id. Having a Manager inside an instance feels wrong regards to the existing architecture.
On this strategy, we would have to change slightly the SessionPruner
while True:
self.is_session_pruner_terminated.wait(
timeout=SessionPruner.PRUNE_SESSIONS_INTERVAL_SECONDS)
if self.is_session_pruner_terminated.is_set():
return
expired_sessions = streamsync.session_manager.expired_sessions()
for session in expired_sessions:
streamsync.session_manager.prune_session(session)
streamsync.component_manager.prune_components(session)
src/streamsync/core.py
Outdated
| @@ -879,6 +885,13 @@ def __init__(self, session_id: str, cookies: Optional[Dict[str, str]], headers: | |||
| new_state.user_state.mutated = set() | |||
| self.session_state = new_state | |||
| self.event_handler = EventHandler(self) | |||
| self.component_manager = ComponentManager() | |||
There was a problem hiding this comment.
I am not convince about the pattern integrating a manager in an instanced class.
I would advocate to keep the list of components the ComponentManager but we would ingest them with the session id. The component manager would stay a high level class we can rely on.
I would make the signature of ComponentManager evolve to keep the list of component per session id. It would be more easier to inspect application memory model and to make it evolve. We know all the components from all the session are in the ComponentManager. It may also allow advanced optimization as storing the diff between static component declaration and dynamic declaration.
We would also keep the ability to query the ComponentManager application wide. It may be interesting for some pattern.
There was a problem hiding this comment.
I think it's aligned with the current architecture. We're already managing sessions for State, my idea is to just put components there so we can manage them in the same way. There's a mechanism for example to clear out expired sessions and we'd have to replicate it for components.
src/streamsync/core.py
Outdated
| @@ -1075,7 +1088,7 @@ def handle(self, ev: StreamsyncEvent) -> StreamsyncEventResult: | |||
| try: | |||
| instance_path = ev.instancePath | |||
| target_id = instance_path[-1]["componentId"] | |||
| target_component = component_manager.components[target_id] | |||
| target_component = self.session.component_manager.components[target_id] | |||
There was a problem hiding this comment.
The code would look like this.
component_manager.get_components(target_id, self.session.session_id)
src/streamsync/core.py
Outdated
| @@ -928,7 +941,7 @@ def _check_proposed_session_id(self, proposed_session_id: Optional[str]) -> bool | |||
| return True | |||
| return False | |||
|
|
|||
| def get_new_session(self, cookies: Optional[Dict] = None, headers: Optional[Dict] = None, proposed_session_id: Optional[str] = None) -> Optional[StreamsyncSession]: | |||
| def get_new_session(self, cookies: Optional[Dict] = None, headers: Optional[Dict] = None, proposed_session_id: Optional[str] = None, components: Optional[Dict[str, Any]] = None) -> Optional[StreamsyncSession]: | |||
There was a problem hiding this comment.
We won't have to give the list of component instanciating the session.
There was a problem hiding this comment.
I agree that this shouldn't happen, should we mirrored in the same way that we handle initial state.
|
|
||
| for i in range(len(instance_path)): |
There was a problem hiding this comment.
the code stay almost the same except on line 764
component_manager.get_components(component_id, self.session_id)
| @@ -336,14 +332,14 @@ def _main(self) -> None: | |||
| if self.mode == "run": | |||
| terminate_early = True | |||
|
|
|||
| try: | |||
| streamsync.component_manager.ingest(self.components) | |||
There was a problem hiding this comment.
streamsync.component_manager.ingest(self.components, self.session_id)
Making ComponentManager a part of session as a prerequisite to link the state of the UI to each particular session instead of the whole app.