-
Notifications
You must be signed in to change notification settings - Fork 74
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
Conversation
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.
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.
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')
src/vs/workbench/contrib/positronHoloViews/browser/positronHoloViewsService.ts
Outdated
Show resolved
Hide resolved
override dispose(): void { | ||
super.dispose(); | ||
// Clean up disposables linked to any connected sessions | ||
this._sessionToDisposablesMap.forEach(disposables => disposables.dispose()); |
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.
Not sure if its necessary, but should we clear the map too?
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.
Probably!
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.
Actually, I guess this will automatically get garbage collected. Assuming dispose()
only gets called when the service is truly being disposed of.
src/vs/workbench/contrib/positronHoloViews/browser/positronHoloViewsService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronHoloViews/browser/positronHoloViewsService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronHoloViews/browser/positronHoloViewsService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronHoloViews/test/browser/positronHoloViewsService.test.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/positronOutputWebview/browser/notebookOutputWebviewService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronOutputWebview/browser/notebookOutputWebviewService.ts
Outdated
Show resolved
Hide resolved
}, | ||
extension: { | ||
id: renderer.extensionId, | ||
// Just choose last renderer for now. This may be insufficient in the future. | ||
id: messagesInfo.at(-1)!.renderer.extensionId, |
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'm not actually too sure what this affects.
src/vs/workbench/contrib/positronOutputWebview/browser/notebookOutputWebviewServiceImpl.ts
Outdated
Show resolved
Hide resolved
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)); |
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.
What's the reason for using a NotebookOutputPlotClient?
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") |
…ok webview and show in the plots pane.
…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>
…e final message and prereq ones.
…etter reflect what it is doing, replaying multiple messages, not showing multiple outputs.
…names and format.
51dce50
to
8b495ac
Compare
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.
Looks good, and working again!
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
Behavior in notebooks should be unchanged
QA Notes
This code will create a simple hvplot scatter plot. It should work in the plots pane.
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).