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 "jupyter" renderer based on JupyterChart #3283

Merged
merged 8 commits into from
Dec 26, 2023

Conversation

jonmmease
Copy link
Contributor

This PR adds a new "jupyter" renderer that performs rendering using JupyterChart. Enable with:

import altair as alt
alt.renderers.enable("jupyter")
...
chart

In combination with #3281, this will make it possible to enable VegaFusion's serverside data transformation on interactive charts without needing to manually wrap charts with JupyterChart.

cc @joelostblom

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.

Thank you! This will make it really convenient to use both jupyter charts and to work with large datasets (after #3281). I think especially for working with large data, being able to switch all charts back and fourth between different options is helpful for exploring what works best in a particular scenario. I will try to test this out a bit, maybe over the weekend. For now, just one comment on the function name

altair/vegalite/v5/display.py Outdated Show resolved Hide resolved
@joelostblom joelostblom self-assigned this Dec 14, 2023
@joelostblom
Copy link
Contributor

Hmm what are the possible things that could be going wrong when I'm getting the text representation instead of the actual chart?

image

I tried installing your branch in a new environment. I was prompted to install anywidget first time I tried creating a jupyterchart, did that and restart jupyterlab after clearing the notebook just in case, but I'm still only seeing the text representation. Is there another optional dependency for jupyter charts that we are do not warn about (maybe vl-convert?)? Or is something going on with my jlab maybe (I'm on 4.0.5).

@mattijn
Copy link
Contributor

mattijn commented Dec 17, 2023

What is the result if you test a basic Jupyter Widget?

import ipywidgets

ipywidgets.IntSlider()

@jonmmease
Copy link
Contributor Author

In addition to @mattijn's question, do you see the same thing when displaying a JupyterChart directly? Could you check the browser console for errors?

@joelostblom
Copy link
Contributor

joelostblom commented Dec 17, 2023

Thanks both, I did see the text representation for regular jupyter widgets as well and also when using JupyterChart directly. There was an error in the console saying TypeError: t.currentWidget.context.model.metadata.has is not a function, but I didn't feel like putting time into understanding what was broken so I installed a new version of jupyterlab (4.0.9) in a fresh environment and things are working as expected now.


Regarding this PR, my understanding is that the renderer only changes the chart at display time, so using this renderer will not give access to all the JupyterChart functionality for each chart, which is also what I saw when testing:

image

This makes sense, since that it how all the other renderers work, but I wonder if it will be confusing in this case since it means that although someone enables the jupyter renderer, they will not be able to take advantage of anything from the JupyterChart documentation page, such as updating the style of marks dynamically:

image

I can see this leading to tickets opened along the lines of "I can't use any JupyterChart functionality even after enabling the jupyter renderer". I wonder if going in one of these two directions would make sense:

  1. I think the use of the jupyter renderer is very valuable and convenient in conjunction with Integrate VegaFusion into JupyterChart #3281 for displaying large data set. Do you think there will be a separate advantage/use case for the jupyter renderer even without that PR? If not, what do you think about renaming the jupyter renderer to indicate that it is to be used with large data and the fact that it uses JupyterChart to achieve this will be treated as a backend detail and not be part of the name? If we go this way, then I think it makes sense if the renderer also enables the vegafusion datatransformer automatically.
  2. Another option would be to convert all chart objects to JupyterCharts permanently, not just for rendering. I guess this would have to be called something else than a renderer then, maybe a transformer? Not sure how this would work in conjunction with the existing vegafusion transformer.

It's possible that the best is just to keep what you have hear and expand the documentation to be really clear on what the renderer can and can't do, but I'm curious to hear your thoughts on the two options above as well.

@jonmmease
Copy link
Contributor Author

Thanks for trying this out @joelostblom.

Yes, you're right that using the "jupyter" renderer means you don't get access to the JupyerChart object. I think this is reasonable and consistent with the other renderers, but I'll update the docs to make this more explicit.

Regarding whether the "jupyter" renderer will have uses beyond the VegaFusion integration. One additional feature I'm going to add after these two PRs merge is offline support for JupyterChart. In this case, the "jupyter" renderer could be the preferred offline rendering approach as, unlike with the mime renderer, we would still control the version of vega-lite being used. One idea would be to enabled it with alt.renderers.enable("jupyter", offline=True).

@joelostblom
Copy link
Contributor

Offline support sounds like a great additional feature for Jupyter renderer! Then I also agree it makes sense to keep it as a renderer only and clearly highlighting the differences to using JupyterChart directly in the docs like you said 👍

@jonmmease
Copy link
Contributor Author

@joelostblom, I updated the language in the docs. See if this looks good!

@mattijn @binste Do you have any objections to adding this renderer, or to the name/API?

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! I think that section of the docs looks good for what this PR provides. When the other PRs land, we can expand the docs to indicate the functionality this render brings, e.g. the integration with the vegafusion transformer and later offline access.

I tested with a few different charts from the gallery and they seem to render fine, not sure if there is a more systematic way to go about it, otherwise that seems sufficient for me.

One last question, would it make sense to add a test similar to the ones for the other renderer functions to check if the metadata is passed correctly or would that not apply here?

@jonmmease
Copy link
Contributor Author

Good call on the testing. It reminded me that we don't support embed options in JupyterChart yet. I opened #3288 to track this, and will add a test case like the above when I implement that.

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.

Ok, if that is added in a follow up PR when the functionality is available in JupyterChart, then this PR looks good to me in terms of just adding the renderer engine, but maybe let's leave open a few days to see is someone else wants to chime in before merging.

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.

Thanks @jonmmease ! Looks good to me :) Agree that this will be a very nice addition together with the VegaFusion integration and potential offline support!

doc/releases/changes.rst Outdated Show resolved Hide resolved
Co-authored-by: Stefan Binder <binder_stefan@outlook.com>
@jonmmease
Copy link
Contributor Author

Thanks @binste!

I'm going to merge this on Monday if there isn't more feedback.

@jonmmease jonmmease merged commit b081237 into main Dec 26, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Performance
Development

Successfully merging this pull request may close these issues.

4 participants