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

feature: Add browser renderer to open charts in external browser and update chart.show() to display chart #3379

Merged
merged 11 commits into from
Mar 29, 2024

Conversation

jonmmease
Copy link
Contributor

@jonmmease jonmmease commented Mar 23, 2024

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, the port argument to configure what port the one-shot server should (defaults to random), and the inline 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 uses IPython.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.

@jonmmease
Copy link
Contributor Author

Fix for faiing VegaFusion tests in #3377

Copy link
Contributor

@mattijn mattijn left a 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.

Copy link
Contributor

@joelostblom joelostblom left a 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?

altair/vegalite/v5/api.py Outdated Show resolved Hide resolved
altair/utils/_show.py Outdated Show resolved Hide resolved
altair/utils/_show.py Show resolved Hide resolved
@jonmmease
Copy link
Contributor Author

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!

@jonmmease
Copy link
Contributor Author

@mattijn Thanks for the call out that we should document the one-shot implications.

@jonmmease
Copy link
Contributor Author

@mattijn and @joelostblom, I just pushed a change to how this works that I think is a lot nicer.

First, I added a "browser" renderer (name up for discussion, this is what we called it in plotly). This renderer exports the chart to HTML and opens it in a browser tab as a side effect. The using, port, and inline arguments can be passed during renderer initialization to configure things, like a regular renderer:

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 show method to trigger the active renderer as a side effect. I think this is useful in general, because for renderers that display charts inline, it makes it easy to display multiple charts in a cell (without concatenation). For example, with the default renderer enabled:

Screenshot 2024-03-24 at 3 12 46 PM

When the browser renderer is enabled, calling chart.show() will open the chart in the browser, even if it's not the final value in the cell/command. Additionally, calling chart.show() with the browser renderer works in a standard (non-IPython) python script or REPL. So you can have a Python scrip with several calls to chart.show() in it, run it from the command line with python my_script.py, and each chart will open in the browser.

@jonmmease jonmmease marked this pull request as draft March 24, 2024 19:23
@jonmmease jonmmease changed the title wip: Update chart.show() to open charts in new browser tab without depending on altair_viewer feature: Update chart.show() to open charts in new browser tab without depending on altair_viewer Mar 24, 2024
@jonmmease jonmmease changed the title feature: Update chart.show() to open charts in new browser tab without depending on altair_viewer feature: Add browser renderer to open charts in external browser and update chart.show() to display chart Mar 24, 2024
@jonmmease jonmmease marked this pull request as ready for review March 26, 2024 15:49
@jonmmease
Copy link
Contributor Author

I updated the docs and marked this as ready for review. @mattijn, the change since you approved was to add the "browser" renderer and have chart.show() invoke the active renderer.

@mattijn
Copy link
Contributor

mattijn commented Mar 26, 2024

I like the approach! I noticed that in practice there are still several people liking to explicitly call the .show() command to visualize the chart.

doc/releases/changes.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@binste binste left a 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.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

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.
Copy link
Contributor

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?

Suggested change
- 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/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.. _IPython: https://ipython.org/
.. _Binder: https://mybinder.org/
.. _Colab: https://colab.google/
.. _IPython: https://ipython.org/

@joelostblom joelostblom self-requested a review March 29, 2024 15:47
Copy link
Contributor

@joelostblom joelostblom left a 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:

image

The difference between using show and not become clear when I also test in the in the regular python console:

image

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)?

@jonmmease
Copy link
Contributor Author

Thanks for reviews everyone. merging for 5.3.0!

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

Successfully merging this pull request may close these issues.

None yet

4 participants