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

Unsubscribe deleted plots from streams #2141

Closed
philippjfr opened this Issue Nov 21, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Nov 21, 2017

When you delete or clear a cell containing a HoloViews plot subscribed to streams in the notebook the plot instance sticks around and continues to be subscribed to Stream events, which can slow things down if you redisplay the same plot. This is a major annoyance that has caught me out frequently. It should be possible to keep track of which cell a plot was displayed in and delete it and unsubscribe from any streams the plot was attached to.

The relevant Jupyter.notebook machinery is this:

Jupyter.notebook.events.on('delete.Cell', function(event, cell) { ... })

with other events being listed here. Every plot that uses streams would send back the cell id on initialization and whenever a cell is deleted we could send a message back across a dedicated comm to notify Python of the cell deletion.

I think something similar may be necessary for tidying up after a bokeh server session is closed.

@philippjfr philippjfr added the feature label Nov 21, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Nov 24, 2017

I think this is a good idea though I would want to ensure a few things:

  1. This approach works in JupyterLab.
  2. The code is written such that if this jupyter API breaks, we fall back to the current behavior instead of causing an overt error for the user.
  3. The code is separate and modular i.e we can maintain a clean API at our end for cleaning up the subscribed plots so that we can register this event handler in one place, keeping the callback definition as short as possible. Sending the cell id seems fine as long as the notion of a cell id doesn't clutter up our code too much - I think we should treat it as a generic ID and keep Jupyter concepts out of HoloViews whenever possible!
@philippjfr

This comment has been minimized.

Member

philippjfr commented Nov 24, 2017

This approach works in JupyterLab.

Until we support JupyterLab in other ways I do not think we should let this hold things up. The APIs exist in JupyterLab but without writing an extension there's no way we're going to make this work on both and that work will/should all happen in one chunk.

The code is written such that if this jupyter API breaks, we fall back to the current behavior instead of causing an overt error for the user.

Can't see how it would ever break something, all the execution would be triggered on the JS end so if it fails, at worst nothing happens. Also these APIs have been stable since very early notebook versions <3.0

Sending the cell id seems fine as long as the notion of a cell id doesn't clutter up our code too much - I think we should treat it as a generic ID and keep Jupyter concepts out of HoloViews whenever possible!

This would mostly be handled in the ipython extension, so I think it's perfectly reasonable to specifically handle cell ids, but some of the plot/stream cleanup can of course use general APIs, which will probably also be useful for lifecycle hooks on bokeh server.

@philippjfr philippjfr added this to the v1.10 milestone Dec 4, 2017

@philippjfr philippjfr self-assigned this Mar 14, 2018

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 2, 2018

Implemented in #2500 and in jupyterlab_holoviews 0.3.3

@philippjfr philippjfr closed this Apr 2, 2018

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