-
Notifications
You must be signed in to change notification settings - Fork 794
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
feature: Add browser renderer to open charts in external browser and update chart.show() to display chart #3379
Conversation
Fix for faiing VegaFusion tests in #3377 |
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 tried several things from command line and notebook through a vscode notebook and was not yet able to break it. So I would say it works as expected.
I hadn't seen it before, but apparently this is a feature missed by people (altair-viz/altair_viewer#59 (comment)).
I don't know the previous behavior, but maybe we should at the meaning of a oneshot request in the docstring of .show()
as I was wondering why the chart was not working anymore after a page refresh or opening the same address:port in an additional tab.
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.
Thanks @jonmmease! This looks good to me an work as expected when I try the PR locally. I only have a few minor inline comments.
I did notice that when trying the first example from https://vegafusion.io/, it works even when I haven't enabled the vegafusion data_transformer (ie doesn't raise the max rows error). I think that's unrelated to this PR and related to how chart.save()
works. I think that makes sense for saving charts since we don't need to worry about large notebook sizes from saving multiple figures. Do you think there are any extra concerns with show
in the sense that it sends the entire (possibly huge) spec to the browser instead of to a file?
Thanks @joelostblom, that's a good call out that this approach disables the max rows error. I think it makes sense to use the same rules here as for the inline html renderer. I'll make this change! |
@mattijn Thanks for the call out that we should document the one-shot implications. |
@mattijn and @joelostblom, I just pushed a change to how this works that I think is a lot nicer. First, I added a alt.renderers.enable("browser", using="chrome", port=1234, inline=True)
...
chart Then, in an IPython setting, the chart will open in a browser tab automatically when it's the last value in a cell/command. Then I updated the When the browser renderer is enabled, calling |
I updated the docs and marked this as ready for review. @mattijn, the change since you approved was to add the |
I like the approach! I noticed that in practice there are still several people liking to explicitly call the |
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.
Only some minor nits. Nice feature and implementation @jonmmease! Good to see that we can remove the mentions of altair_viewer from the docs and provide an alternative to users.
doc/user_guide/display_frontends.rst
Outdated
specified, the system default browser is used. | ||
- The ``offline`` argument may be used to specify whether JavaScript dependencies should | ||
be loaded from an online CDN or embedded alongside the chart specification. When ``offline`` | ||
is ``False`` (The default), JavaScript dependencies are loaded from an online CDN, and so |
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.
is ``False`` (The default), JavaScript dependencies are loaded from an online CDN, and so | |
is ``False`` (the default), JavaScript dependencies are loaded from an online CDN, and so |
doc/user_guide/display_frontends.rst
Outdated
chart in the browser when the chart is the final value of the cell or command. This behavior is not | ||
available in the standard ``python`` REPL. In this case, the ``chart.show()`` method may be used to | ||
manually invoke the active renderer and open the chart in the browser. | ||
- This renderer is not compatible with remote environments like Binder or Colab. |
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.
Could we use links also for "Binder" and "Colab" so that it's consistent with Ipython and Spyder above?
- This renderer is not compatible with remote environments like Binder or Colab. | |
- This renderer is not compatible with remote environments like `Binder`_ or `Colab`_. |
@@ -678,3 +709,7 @@ see :ref:`display-general`. | |||
.. _Vega: https://vega.github.io/vega/ | |||
.. _VSCode-Python: https://code.visualstudio.com/docs/python/python-tutorial | |||
.. _Zeppelin: https://zeppelin.apache.org/ | |||
.. _IPython: https://ipython.org/ |
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.
.. _IPython: https://ipython.org/ | |
.. _Binder: https://mybinder.org/ | |
.. _Colab: https://colab.google/ | |
.. _IPython: https://ipython.org/ |
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.
Thanks for the updates @jonmmease ! When I tested the latest changes locally after, I was initially surprised that it is no longer enough to call .show()
on an individual chart to open it in the browser; but then I read your updates to the docs in more details and realized that I have to enable the new 'browser'
renderer:
The difference between using show
and not become clear when I also test in the in the regular python console:
I would expect that this makes things more convenient overall, since the IPython console is for working interactively and it's helpful that all charts open in the browser without the need to call show
when the new renderer is active. The only thing that becomes less convenient is showing a single chart or switching between showing and not showing charts, but that is probably a rare workflow.
Since this differs from how the old altair_viewer approach worked where it was enough to call show
, but you had to do it for every chart, I think it's great that you have clearly mentioned it as two separate items in the release notes. Maybe we should document the behavior of showing multiple charts from the same cell using show
/display
somewhere in the docs as well (doesn't have to be in this PR though)?
Thanks for reviews everyone. merging for 5.3.0! |
This WIP PR tries out the idea I mentioned in #2866 (comment).
It adds a new
"browser"
renderer that opens the chart in a new browser tab using a one-shot web server and the Python webbrowser module. I like this approach because it avoids the need to write temporary files just to open them in the browser (Although we do need to document that this means that you can't refresh the page or open the url in another browser tab).The browser renderer's
using
argument can be used to configure what browser should be used, theport
argument to configure what port the one-shot server should (defaults to random), and theinline
argument to configure whether to embed all JavaScript dependencies in the html bundle so that the renderer works offline.This PR also updates
chart.show()
to invoke the active renderer as a side-effect. This usesIPython.display.display
for all renderer except the browser renderer. There's a special case for the browser renderer that invokes_repr_mimebundle_
directly so that it works from a standard python repl, without IPython installed.If we want to go forward with this approach, I'll update the docs to replace the Altiar Viewer section.
cc @fhg-isi if you have a chance to give this branch a try.