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

Merged
merged 20 commits into from Aug 31, 2016

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Aug 30, 2016

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 and push methods. Supports updating regular matplotlib plots, nbagg plots, mpld3 plots and bokeh plots.

@jbednar

This comment has been minimized.

Member

jbednar commented Aug 30, 2016

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?

@philippjfr philippjfr added the WIP label Aug 30, 2016

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 30, 2016

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.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 30, 2016

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).

@jbednar

This comment has been minimized.

Member

jbednar commented Aug 30, 2016

And Bokeh server?

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 30, 2016

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):

This comment has been minimized.

@jlstevens

jlstevens Aug 30, 2016

Member

I think diff would be a clearer name.

"""
class WidgetCommSocket(CommSocket):
"""

This comment has been minimized.

@jlstevens

jlstevens Aug 30, 2016

Member

I suggest calling this NbAggCommSocket...

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 30, 2016

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.
"""

This comment has been minimized.

@jlstevens

jlstevens Aug 30, 2016

Member

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):
"""

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2016

Member

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'

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2016

Member

I would consider a better name than 'Complete'. Maybe 'Transferred', 'Transfer Complete' or maybe just 'Sent'. Not too important though...

target = $('#fig_el{comms_target}');
target.children().each(function () {{ $(this).remove() }});
mpld3.draw_figure("fig_el{comms_target}", data);
"""

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2016

Member

I don't see a bokeh_msg_handler anywhere in the file. Is that because it is already set up for you?

This comment has been minimized.

@philippjfr

philippjfr Aug 31, 2016

Member

Yes, both registration and message handling is provided by bokeh itself, which is why I'm stuck using the JupyterPushComm for now at least.

img = $('<div />').html(data);
target.children().each(function () {{ $(this).remove() }})
target.append(img)
"""

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2016

Member

Given this is the body of the function and not the complete function, I think a comment could help make it clearer. E.g:

mpl_msg_handler = """
/* Body of the msg_handler function that is passed the argument 'msg' */
target = $('#{comms_target}');
img = $('<div />').html(data);
target.children().each(function () {{ $(this).remove() }})
target.append(img)
"""

I think it should be Javascript comment as it is about Javascript code. I think this will help anyone copying and pasting this to write a new message handler.

# Define comm targets by mode
comms = {'default': (JupyterComm, mpl_msg_handler),
'nbagg': (NbAggJupyterComm, None),
'mpld3': (JupyterComm, mpld3_msg_handler)}

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2016

Member

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.

renderer.comms['default'] = JupyterPushComms
def teardown(self):
renderer.comms['default'] = self.default_comm

This comment has been minimized.

@jlstevens

jlstevens Aug 31, 2016

Member

My last comment is that it might be nice to have as many unit tests as possible. I realize it is tricky without the notebook and Javascript but maybe the decode method can be tested (if made into a classmethod). Hopefully, __init__ can also be tested in some cases...

@jlstevens

This comment has been minimized.

Member

jlstevens commented Aug 31, 2016

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.

@philippjfr philippjfr removed the WIP label Aug 31, 2016

@jlstevens jlstevens merged commit fac38de into master Aug 31, 2016

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
coverage/coveralls Coverage decreased (-0.4%) to 69.025%
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens removed the in progress label Aug 31, 2016

@philippjfr philippjfr deleted the comms branch Sep 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment