Streaming protocol #250
Streaming protocol #250
Conversation
| @@ -179,6 +180,9 @@ def remote_backend_handle_result_value(self, value: Dict[str, Any]) -> None: | |||
| # TODO : graph mismatch handle. hash json ? | |||
| for node_name, node_value in value.items(): | |||
| self.graph.nodes[node_name]._value = node_value | |||
|
|
|||
| def remote_backend_get_stream_node(self, name: str, graph_id: str) -> "Node": | |||
There was a problem hiding this comment.
I get that the function name provides context for when the function is called (and perhaps the only actual usecase), but why not use a more general name, like get_node()? It is just a getter function afterall
There was a problem hiding this comment.
Oh I see, it overwrites the function signature in RemoteBackend, and you addressed it with a comment. Is there a reason why this needs to be inherited from RemoteBackend, instead of from GraphBasedContext or just defined directly in this class? (If there is a good reason, I feel like it should be documented)
|
|
||
| def __call__(self, obj: RemoteMixin): | ||
| def __call__(self, object: RemoteMixin): |
There was a problem hiding this comment.
This function is way easier to read now!
| obj.remote_backend_cleanup() | ||
|
|
||
| def handle_response(self, data: Any) -> "ResponseModel": | ||
| def handle_response(self, response: "ResponseModel") -> None: |
There was a problem hiding this comment.
For readability, I think that blocking_request() and non_blocking_request() should be placed immediately after __call__()
|
|
||
| return apply(fn, *args, **kwargs) | ||
|
|
||
| return inner |
There was a problem hiding this comment.
I see this pattern defined in numerous locations (__init__.py, GraphBasedContext.py). Do you think this could be reused as a util function (with a descriptive docstring)?
My crowning achievement.