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.

Sign up for GitHub

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

Jump to bottom

Fix resizing plotly web clients #4806

Open

timtmok wants to merge 6 commits into main from bugfix/plotly-resize

+96 −31

Contributor

timtmok commented Sep 24, 2024

Address #4245

Sets Plotly to use its browser renderer. When calling show(), we intercept and send an event so that the URL is displayed in a webview in the Plots View instead of the viewer (or in the Viewer if the preference is set).

Plotly's browser renderer fills width and height to 100% when no plot size is defined compared to the JupyterLab renderer (or vscode?) that defaults to Plots View's width and height 450px. Underneath, the browser renderer sets responsive: True for the config, which will set width and height to 100%.

I'm not sure about the proxy part. We don't create one for the Viewer so I'm not sure if this will need more work when using this in Posit Workbench.

QA Notes

Code to generate a Plotly plot. pip install plotly before running.

import plotly.express as px
fig = px.bar(x=["a", "b", "c"], y=[1, 3, 2])
fig.show()

Disable this setting so the plot shows in Plots instead of the Viewer.


          Fix resizing plotly web clients

234140f

Detect plotly plots and send as an HTML plot event
Shows the localhost plot in Plots View

timtmok requested review from jmcphers and seeM

September 24, 2024 20:54


          Check _comm before use

a403876

Contributor Author

timtmok commented Sep 25, 2024

I'm working adding a test similar to what is there for Bokeh

seeM reviewed

View reviewed changes

Contributor

seeM left a comment •

edited

Loading

Confirmed that plotly plots are resizing with the plots pane! My main concern is about circumventing the proxy server.

src/vs/workbench/services/languageRuntime/common/languageRuntimeUiClient.ts

Comment on lines -165 to +171

-              				// Start an HTML proxy server for the file
-              				const uri = await this.startHtmlProxyServer(e.path);
+              				if (e.path.endsWith('.html') || e.path.endsWith('.htm')) {
+              					// Start an HTML proxy server for the file
+              					uri = await this.startHtmlProxyServer(e.path);
+              				} else {
+              					uri = URI.parse(e.path);
+              				}

Contributor

seeM Sep 25, 2024

Does it work without this change? This looks like it won't create a proxy server which I think wouldn't work on Workbench.

Contributor Author

timtmok Sep 27, 2024

This part I wasn't sure if it would work. When opening other URLs in the viewer, it doesn't proxy it. I'll try it out but I agree that I don't think it would work.

Contributor Author

timtmok Sep 27, 2024

It seems to work when running Positron server but it's all local. I think it's only working since the deepest iframe loads the plotly url.

@sharon-wang I think it probably needs a tweak on creating the proxy server or a slightly different type of proxy server. The one used for HTML files didn't seem to work with the localhost URL that plotly produces (file URI vs. localhost URL).

Member

sharon-wang Sep 27, 2024

Do you have some example plotly URI/URLs? On main, python plotly plots work on Server Web and Workbench, so I wonder what changes to the URI/URLs are occurring?

Contributor Author

timtmok Sep 30, 2024

http://127.0.0.1:6728/

It's loading something like that with a random port. Before this change, it would load an HTML file and go through the HTMLProxy. So I think it'll need to load the URL with a proxy as it did before.

Member

sharon-wang Sep 30, 2024

@timtmok could you please verify if reverting this change to bypass the proxy and bringing in the changes from #4855 still works with this PR?

extensions/positron-python/python_files/positron/positron_ipykernel/ui.py Outdated

                               return True
                       # pass back to webbrowser's list of browsers to open up the link
                       return False
+                  def _is_module_function(self, module_name, function_name):

Contributor

seeM Sep 25, 2024

Nit: This could be a standalone function since self is never used.

extensions/positron-python/python_files/positron/positron_ipykernel/ui.py

+                      if module:
+                          for frame_info in inspect.stack():
+                              if (
+                                  inspect.getmodule(frame_info.frame, frame_info.filename) == module

Contributor

seeM Sep 25, 2024

I initially wrote this code, and I've wondered if it would be safer to check only the module part and not the function. What do you think?

Contributor Author

timtmok Sep 26, 2024

It seemed to make sense to check the function too but I don't think I know enough about Python to foresee any problems. I suppose removing the function check would handle any new calls that Bokeh or Plotly might add.

Then it could work in that scenario without having to do anything. Keeping the check would definitely not work in that case so there's nothing to lose?

extensions/positron-python/python_files/positron/positron_ipykernel/ui.py Outdated

+                          if os.name == "nt":
+                              url = urlparse(url).netloc or urlparse(url).path
+                          return self._comm.send_event(

Contributor

seeM Sep 25, 2024

I think webbrowser expects this method to return a boolean: True if we've handled the request.

Contributor

seeM Sep 25, 2024

Should we be calling _send_show_html_event here?

extensions/positron-python/python_files/positron/positron_ipykernel/ui.py Outdated

+                              if is_plot:
+                                  if os.name == "nt":
+                                      url = urlparse(url).netloc or urlparse(url).path
+                                  event = ShowUrlParams(url=url)

Contributor

seeM Sep 25, 2024

Suggested change

event = ShowUrlParams(url=url)

extensions/positron-python/python_files/positron/positron_ipykernel/ui.py

+                  def _send_show_html_event(self, url, is_plot):
+                      if self._comm is None:
+                          return False

Contributor

seeM Sep 25, 2024

We should log a warning here.

Contributor Author

timtmok Sep 27, 2024

Good idea

extensions/positron-python/python_files/positron/positron_ipykernel/ui.py Outdated

		return False


		def _send_show_html_event(self, url, is_plot):

Contributor

seeM Sep 25, 2024

Could you please add type annotations to this and _is_module_function?

Contributor Author

timtmok Sep 27, 2024

Annotations added

extensions/positron-python/python_files/positron/positron_ipykernel/ui.py Outdated

Comment on lines 216 to 217

		if os.name == "nt":
		url = urlparse(url).netloc or urlparse(url).path

Contributor

seeM Sep 25, 2024

Maybe this can be moved to _send_show_html_event?

Contributor Author

timtmok Sep 27, 2024

I've moved it into the new function

timtmok added 2 commits

September 26, 2024 11:09


          unit test

6995edd


          cleanup

0f29518

Contributor Author

timtmok commented Sep 26, 2024

I added a unit test but it doesn't work locally. It hangs after making the call to send the comm event. Not really sure what's going on here?

timtmok added 2 commits

September 27, 2024 09:33


          fix unit test

e65acc7


          formatting

5a9afda

Contributor Author

timtmok commented Sep 27, 2024

I had to mock HTTPServer.handle_request() to stop the test from hanging.

sharon-wang mentioned this pull request

Fix interactive R and Python plots on Server Web and Workbench #4855

Open

jmcphers approved these changes

View reviewed changes

Collaborator

jmcphers left a comment

Did not build/test this since others have.

A little nitpicky, but I'd prefer not to overload ShowHtmlFile if we're sending a URL rather than a HTML file path. There's ShowUrl already; we could add a param to that method to indicate a plot, or create a different event to handle this brand of URLs.

If we do need to overload ShowHtmlFile, maybe we could check for file: / http: URIs rather than checking the file extension?

Member

sharon-wang commented Oct 1, 2024

A little nitpicky, but I'd prefer not to overload ShowHtmlFile if we're sending a URL rather than a HTML file path. There's ShowUrl already; we could add a param to that method to indicate a plot, or create a different event to handle this brand of URLs.

If we do need to overload ShowHtmlFile, maybe we could check for file: / http: URIs rather than checking the file extension?

@jmcphers I think that change will be reverted if the fix in #4855 works for Tim -- the approach in #4855 is to check the URI scheme in startHtmlProxyServer and start the appropriate proxy server: https://github.com/posit-dev/positron/pull/4855/files#diff-b42b14eff9e1b23c8e4bbe70783a6a4065dd0ecc89d250f1012e006367079366

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

Labels

None yet