Skip to content
This repository has been archived by the owner on Nov 29, 2019. It is now read-only.

Refactored comms to add JupyterLab support #51

Merged
merged 10 commits into from
May 21, 2018
Merged

Conversation

jlstevens
Copy link
Member

@jlstevens jlstevens commented May 21, 2018

This PR is still WIP and there are a lot of issues to address:

  • Made comms code consistent with holoviews.
  • Works in classic notebook
  • Displays widgets in jupyterlab
  • Updates parameter values in jupyterlab
  • All functionality relating to the bokeh comms and plots option.
  • Currently removing duplication of comms code by importing from holoviews. HoloViews should not be a parambokeh dependency but we do need a comms package for pyviz.
  • Currently needs a HoloViews object to be introduced in JS which isn't appropriate for other pyviz projects.

Right now the problem I am trying to address is to get comms working in jupyterlab as the parameter values are not updating.

Note that this requires the pyviz jupyterlab extension. I have the bokeh jupyterlab extension but I'm not yet sure whether or not it is actually required.

js_callback = CustomJS(code=fetch_data+self_callback)
debounce=self.debounce,
plot_id=self.comm.id)
js_callback = CustomJS(code='\n'.join([HOLOVIEWS_PROXY,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting the proxy code in here will reset the comms and kernels registries after they have been set up. At minimum you'd have to ensure that it does not override them if they are already defined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll give that a go.

bokeh_script, bokeh_div, _ = bokeh.embed.notebook.notebook_content(obj, target)
publish_display_data(data={'text/html': encode_utf8(bokeh_div)})
publish_display_data(data={mime: '', 'application/javascript': bokeh_script},
metadata={mime: {'id': target}})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be the self.comm.id rather than the target.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This is what was needed to fix it.

@jlstevens
Copy link
Member Author

The basics now work in both classic notebook and jupyterlab: you can display some widgets, manipulate them and use that to update parameters.

I'll now look into the other uses of comms (e.g relating to View parameters/plots) but after that, the remaining decisions involve changes across repos (making a comms package, renaming the HoloViews object etc).

@jlstevens
Copy link
Member Author

Here is a related pyviz issue: holoviz/pyviz_comms#4

It isn't the whole story as the comms code itself references HoloViews and that is where it currently lives, though it should be pulled out and made general to pyviz.

@jlstevens
Copy link
Member Author

With the commit by @philippjfr, I believe the TODO items have been ticked off except those that require more discussion across pyviz.

@jlstevens
Copy link
Member Author

Updated the imports to use the new pyviz_comms package.

@jlstevens
Copy link
Member Author

jlstevens commented May 21, 2018

Tests are now green and I have ticked off the TODO items above. It isn't perfect but I think it is ready to review and merge before improving upon in subsequent PRs. The pyviz_comms package is now required and available on the pyviz conda channel. This will be available on PyPI soon.

@philippjfr
Copy link
Member

Looks good.

@philippjfr philippjfr merged commit d1d1a47 into master May 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants