-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Add Comms to allow updating plots dynamically #838
Conversation
Can you briefly summarize what new functionality this will support, and/or what types of simplifications to the codebase will be possible, depending on your motivation here? |
Sure, probably the right place to put this even though it's only a first step in that direction. Eventually the aim of Comms is to allow any bidirectional communication between a Python process, whether that is a webserver or a Jupyter kernel, and a frontend. This will allow defining complex interactions using the upcoming Streams interface but should also eventually replace the existing mechanisms on our widgets to communicate widget state to python and update a plot on the frontend. One remaining obstacle will be to appropriately throttle events sent from the frontend to the python process. |
At a higher level, I think of comms as offering a way to standardize communication with HoloViews visualizations that live in the browser (not necessarily the notebook). This should help simplify things and make it easier to implement new functionality whereby plots get updated over some communication channel (Jupyter Comms, websockets, some raw TCP socket, pipes, RPC etc). |
And Bokeh server? |
Most probably! We will definitely find a way to communicate with Bokeh visualizations without relying on Jupyter comms (i.e outside of notebooks). |
@@ -135,6 +141,19 @@ def get_size(self_or_cls, plot): | |||
return (w*dpi, h*dpi) | |||
|
|||
|
|||
def patch(self, plot): |
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.
I think diff
would be a clearer name.
Made a couple of comments regarding naming. Overall, I'm very pleased! This looks a lot clearer to me, the APIs look clean and I feel that I understand how it all works. I realize it is still work-in-progress but it is already looking good! |
A CustomCommSocket is required to delay communication | ||
between the kernel and the canvas element until the widget | ||
has been rendered in the notebook. | ||
""" |
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.
I believe CustomCommSocket
must be an outdated name. This docstring should mention that CommSocket
is a matplotlib concept that you are customizing to support NbAgg.
|
||
|
||
class SimpleJupyterComm(Comm): | ||
""" |
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.
How about calling it JupyterPushComm
? From the sound of it, we may not need it in future one bokeh works with bi-directional comms.
for h in handles] | ||
msg = compute_static_patch(doc, plotobjects) | ||
handle.comms.send(json.dumps(msg)) | ||
return 'Complete' |
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.
I would consider a better name than 'Complete'. Maybe 'Transferred', 'Transfer Complete' or maybe just 'Sent'. Not too important though...
# Define comm targets by mode | ||
comms = {'default': (JupyterComm, mpl_msg_handler), | ||
'nbagg': (NbAggJupyterComm, None), | ||
'mpld3': (JupyterComm, mpld3_msg_handler)} |
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.
In a later PR it would be good to have a nice way for the user to select the appropriate comms. E.g using WebsocketComms
if not using the notebook.
I've now reviewed this PR and made all the comments I want to make. Overall, I'm really pleased with all of this! Once these comments are addressed, I am happy to merge. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR introduces Comms classes which allow bidirectionally communicating between the python process and a frontend. Additionally it gives Plot instances access to the current renderer and instantiates a Comms class, allowing updates to plots to be pushed to the frontend using the new
refresh
andpush
methods. Supports updating regular matplotlib plots, nbagg plots, mpld3 plots and bokeh plots.