Skip to content
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 support for HoloViz/HoloViews plots #4335

Merged
merged 16 commits into from
Aug 15, 2024
Merged

Add support for HoloViz/HoloViews plots #4335

merged 16 commits into from
Aug 15, 2024

Conversation

nstrayer
Copy link
Contributor

Addresses #3880

What

This PR adds a new service that intercepts messages from holoviz plots so they can be replayed when a plot is rendered into the plots pane.

Why

This is necessary because holoviz sends preliminary messages that load assets for displaying plots and these messages need to be sent to the plot webview along with the plotting message when rendering the plot or else various assets will not be available.

This is not a problem in notebooks because all output is rendered into a single large "backlayer" webview and thus the preliminary messages are properly "remembered."

This is specific to holoviz/holoviews plots right now but there is a decent chance this pattern of remembering prerequisite messages and replaying them to the output webview will be useful for other libraries/situations.

Limitations

Right now we store the prereq messages in memory on the UI thread. This is nice and lightweight but has the problem of forgetting those messages if the user reloads the front end. Since hvplots doesn't replay the asset loading messages on reload the plots won't work until the session is restarted. We could do something like make the storage of prereq messages use a non-ui layer cache, but I'm not convinced it will be a big issue.

Results

hvplots now work in the plots pane
image

Behavior in notebooks should be unchanged
image

QA Notes

This code will create a simple hvplot scatter plot. It should work in the plots pane.

import polars as pl
import hvplot
import hvplot.polars
pl.DataFrame(dict(x=[1,2,3], y=[4,5,6])).hvplot.scatter(x="x", y="y")

The same code should work in a notebook. (Although even before this PR I noticed sometimes it was flakey which I think is due to how hvplots loads assets).

seeM
seeM previously approved these changes Aug 14, 2024
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

Very nice @nstrayer! I went through a bunch of plots from https://hvplot.holoviz.org/ and they seem to work well in the console and notebooks! They also reload correctly when changing the active pane or collapsing the plot pane. Also confirmed that pre-existing plot types continue to work (Python/R Plotly, Python ipywidgets).

I noticed two issues which we can address separately:

Plot widths

I noticed the plot is not sized to fit the plots pane. I'm not actually sure what determines the size. It seems like it's being hardcoded to a width that assumes its in a notebook interface? Since the width seems to be the same for different plot types, in the plots pane and the notebook, and also in their docs. Maybe we can make a note in #4245.

image

Plotly backend

IIUC hvplot can use different plotting "extensions" with Bokeh being the default and working well in this branch. However, Plotly does not seem to be working yet, we can track that separately if needed.

import hvplot.pandas
from bokeh.sampledata.penguins import data as df

hvplot.extension('plotly')

df.hvplot.scatter(x='bill_length_mm', y='bill_depth_mm', by='species')

override dispose(): void {
super.dispose();
// Clean up disposables linked to any connected sessions
this._sessionToDisposablesMap.forEach(disposables => disposables.dispose());
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if its necessary, but should we clear the map too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I guess this will automatically get garbage collected. Assuming dispose() only gets called when the service is truly being disposed of.

},
extension: {
id: renderer.extensionId,
// Just choose last renderer for now. This may be insufficient in the future.
id: messagesInfo.at(-1)!.renderer.extensionId,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not actually too sure what this affects.

disposables.add(outputWebview);

// Create a plot client and fire event letting plots pane know it's good to go.
const client = disposables.add(new NotebookOutputPlotClient(outputWebview, displayMessage));
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for using a NotebookOutputPlotClient?

@seeM
Copy link
Contributor

seeM commented Aug 15, 2024

Code changes look good, but I seem to be getting an empty plot on the latest commit when I run:

import polars as pl
import hvplot
import hvplot.polars
pl.DataFrame(dict(x=[1,2,3], y=[4,5,6])).hvplot.scatter(x="x", y="y")

nstrayer and others added 16 commits August 15, 2024 09:53
…oViewsService.ts

Co-authored-by: Wasim Lorgat <mwlorgat@gmail.com>
Signed-off-by: Nick Strayer <nick.strayer@posit.co>
…oViewsService.ts

Co-authored-by: Wasim Lorgat <mwlorgat@gmail.com>
Signed-off-by: Nick Strayer <nick.strayer@posit.co>
…kOutputWebviewService.ts

Co-authored-by: Wasim Lorgat <mwlorgat@gmail.com>
Signed-off-by: Nick Strayer <nick.strayer@posit.co>
…kOutputWebviewServiceImpl.ts

Co-authored-by: Wasim Lorgat <mwlorgat@gmail.com>
Signed-off-by: Nick Strayer <nick.strayer@posit.co>
…etter reflect what it is doing, replaying multiple messages, not showing multiple outputs.
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

Looks good, and working again!

@nstrayer nstrayer merged commit 9537ea3 into main Aug 15, 2024
2 checks passed
@nstrayer nstrayer deleted the bugfix/hvplot branch August 15, 2024 15:27
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2024
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.

3 participants