-
Notifications
You must be signed in to change notification settings - Fork 795
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 offline ChartWidget based on AnyWidget #3108
Conversation
altair/widget/__init__.py
Outdated
|
||
class ChartWidget(anywidget.AnyWidget): | ||
_esm = _here / "static" / "index.js" | ||
spec = traitlets.Unicode().tag(sync=True) |
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.
Why isn't spec also a dict?
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.
Yeah, I think it could be. This was just copied from the blog post.
At this point I'm only really looking for feedback on the the project layout. The Python ChartWidget
and JavaScript render
functions are going to be rewritten.
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.
Makes sense. I still feel this could live in ipyvega since there is nothing that is strictly Altair specific in the renderer but I don’t feel strong either way.
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.
My hope is, but please correct me if I'm wrong here, that by incorporating anywidget
it is possible to add a new template or upgrade the current existing html/js-templates in https://github.com/altair-viz/altair/blob/master/altair/utils/html.py.
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.
Would it make sense to deprecate ipyvega when this is ready then? I'd think 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.
@manzt, what does it take to write an anywidget to a self-contained html file? I may be mis-remembering, but I thought that was something that you said was possible.
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.
Would it make sense to deprecate ipyvega when this is ready then? I'd think so.
Yeah, I think this could cover the same use-cases as the VegaWidget
in ipyvega. I'm not sure if the mimerenderer portion of ipyvega is covered by Altair right now.
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.
You should be able to use use the ipywidgets.embed.embed_minimal_html
:
chart_widget = ChartWidget(...)
from ipywidgets.embed import embed_minimal_html
embed_minimal_html("chart.html", views=[chart_widget], title="my chart")
But this doesn't seem to be working for any third-party widgets i've tested at the moment... I'll try to have a look and figure out what's going on.
Yup, exactly. You should also be able to run npm run build -- --watch during developmen to enable esbuild's watch-mode and get live-reloading from anywidget. |
Another 2 questions @jonmmease, I became a bit scared of this package-lock.json and thought we will have to do the dependency management and version control of the correct (~125) node modules. But in my current understanding these node modules are indirectly defined through this line: "dependencies": {
"vega-embed": "^6.22.1"
}, When doing Since you also didn't commit the node_modules folder, would it make sense to place the Currently the used VEGALITE_VERSION: Final = SCHEMA_VERSION.lstrip("v")
VEGA_VERSION: Final = "5"
VEGAEMBED_VERSION: Final = "6" Would it make sense to somehow make sure these versions are in-sync with each other? |
Yes,
Yes, we should come up with some kind of approach to either sync these automatically, or test that the versions match. We would also want to do the same for the versions of Vega-Lite and Vega that Altair is using. |
Thanks for explaining, in my understanding we always want the exact same versions of the sub-dependencies that were used in the releases of the corresponding Vega-Embed/Vega and Vega-Lite packages. Do you think this package-lock.json can provide this? Or this wouldn't matter? |
To match what Altair is using, I'll add For transitive dependencies, we may not get the exact same versions that are used in the vega/vega-lite bundles that we pull from the CDN. This shouldn't be a problem, but if it ever comes up, we can pin the versions of these transitive dependencies by adding them to
What do you mean by "the build-versions"? Right now, I don't think we bundle Vega/Vega-Lite with Altair. This is why altair_viewer is a dependency for offline html export. At this point, I don't think there's a way around running |
I was wondering indeed if we could make use of these here. Thanks for the clarifications! |
Yeah, that's correct. The assets must be sent from the jupyter kernel for things to work offline. |
@jonmmease if you want to avoid checking in the built artifact ( [tool.hatch.build]
artifact = "altair/static/index.js"
[tool.hatch.build.hooks.jupyter-builder]
build-function = "hatch_jupyter_builder.npm_builder"
ensured-targets = ["altair/static/index.js"]
skip-if-exists = ["altair/static/index.js"]
dependencies = ["hatch-jupyter-builder>=0.5.0"]
[tool.hatch.build.hooks.jupyter-builder.build-kwargs]
npm = "npm"
build_cmd = "build"
path = "widget" to you This is how |
@@ -0,0 +1,104 @@ | |||
import anywidget # type: ignore |
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.
oh interesting, are anywidget's types not working with mypy? maybe I forget a py.typed
?
widget/package.json
Outdated
"main": "index.js", | ||
"scripts": { | ||
"build": "esbuild src/index.js --bundle --format=esm --outfile=../altair/widget/static/index.js --minify", | ||
"watch": "esbuild src/index.js --bundle --format=esm --watch --outfile=../altair/widget/static/index.js --minify" |
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.
If you want, you can reuse the prior build command and supply extra flags like:
"watch": "esbuild src/index.js --bundle --format=esm --watch --outfile=../altair/widget/static/index.js --minify" | |
"watch": "npm run build -- --sourcemap=inline --watch" |
The inline sourcemaps are nice during dev so that you get nice tracebacks in the console for any errors.
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.
Good idea!
Thanks for the example. The hatch approach is really nice! @mattijn @binste @joelostblom , do you all have a preference between these two options?:
|
Its great to see these I'm leaning toward (2) at the moment. I've I tried it a bit and naively thought that the class ChangePar:
def __init__(self, new_params):
self.new = new_params
par = chart_widget.params['cornerRadius']
par.value = 40
change_par = ChangePar(new_params={par.name: {"value": par.value}})
chart_widget.change_params(change_par)
chart_widget.params['cornerRadius']
# no change when rendering chart_widget Is the Can this also be an attempt to enhance the capabilities of |
Yeah, this should be a private function named something more like I'm planning to add support for updating params in the future (VegaFusion will need this). Maybe a method like
FWIW, that's my preference as well. One thing to clarify though is that npm is not installable with |
I don't have good experiences with mixing hatch and conda in workflows at Github Action. |
Wow so much progress being made quickly here, thank you @jonmmease ! Regarding your question, I would also lean towards 2; if |
Just to be clear, this build does not require the
The FWIW, I think I would also prefer option 2. |
Thanks for the input all. I'll work on incorporating the hatchling npm plugin |
pyproject.toml
Outdated
@@ -94,6 +94,7 @@ allow-direct-references = true | |||
|
|||
[tool.hatch.build] | |||
include = ["/altair"] | |||
artifact = "altair/widget/static/index.js" |
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.
should be “artifacts”, sorry for the typo in my suggestion
This is awesome! 🔥 Really cool feature and great to see so many people providing inputs. A question regarding putting this into Altair vs. ipyvega. In past discussions such as #2807 it was noted that the Vega-Lite and Vega JS code was kept separate in altair_viewer to keep Altair small. I think there were other discussions but I can't find them right now. An Altair wheel file is currently 470KB which is a great advantage in the WASM world and I'm wondering if it's worth keeping it this way. Ipyvega could be installed, together with other optional dependencies such as vl-convert, using Apart from speed, another upside would be that we don't need npm for development. I don't mind it personally but it's already now not straightforward to start contributing to Altair although it got a lot easier since we no longer need altair_saver. We would again loose part of that advantage as it's no longer a simple I'm also on board with including it in Altair and want to reiterate that I find this an awesome feature. Just want to hear your thoughts on these points. Maybe they were already discussed somewhere and I missed it. |
Good questions @binste! I think we all like the provision of improved access to the View API of Vega within Altair. Regarding added LOC, I think this PR is small for the functionality it returns. In my understanding this PR would enable (eventually) resolving the three most upvoted issues in the Altair Github repository, namely #426, #1153, #290 (and also #435, #1801). Is that true @jonmmease? Regarding WASM, I'm not sure if size is the bottleneck. I was in favor of including vl-convert as a hard dependency, see #2816 and willing to accept an increase of 20mb in size. But the inability of installing a compiled library (vl-convert) within WASM and other places with certain arm architectures was the argument to not do it. For me this extra few KBs, even if this an x2 increase of total size, is acceptable. Not sure if we need this few extra KB at this moment though, see below. If we follow this route through inclusion of a bundled version for offline rendering will this release the dependency on altair_viewer? The current released version of altair_viewer provides no support for Altair version 5 and higher and I'm not certain if there will come a new release. Maintenance burden is a good point. Will And while I like the hatch-bundling approach, it is currently not yet working for me. This is the current result if I do: error: subprocess-exited-with-error
Preparing editable metadata (pyproject.toml) did not run successfully.
exit code: 1
[138 lines of output]
INFO:hatch_jupyter_builder.utils:Running jupyter-builder
INFO:hatch_jupyter_builder.utils:Building with hatch_jupyter_builder.npm_builder
INFO:hatch_jupyter_builder.utils:With kwargs: {'npm': 'npm', 'build_cmd': 'build', 'path': 'widget'}
INFO:hatch_jupyter_builder.utils:Installing build dependencies with npm. This may take a while...
INFO:hatch_jupyter_builder.utils:> D:\Software\mambaforge\envs\stable\npm.CMD install
npm WARN vega-embed@6.22.1 requires a peer of vega@^5.21.0 but none is installed. You must install peer dependencies yourself.
npm WARN vega-embed@6.22.1 requires a peer of vega-lite@* but none is installed. You must install peer dependencies yourself.
npm WARN vega-themes@2.14.0 requires a peer of vega@* but none is installed. You must install peer dependencies yourself.
npm WARN vega-themes@2.14.0 requires a peer of vega-lite@* but none is installed. You must install peer dependencies yourself.
npm WARN altair_widget@0.1.0 No repository field.
... |
It shouldn’t be. In this case you’d remove widget/ and check-in altair/static/index.js that imports dependencies from a CDN. |
Thanks for the thoughtful comments @binste and @mattijn. Added functionality
Yes, I think there's a clear path to covering all of these. The only one I didn't already have in mind was #435 (Having the ability to stream data to the client). But ipyvega already has the ability, so it's certainly something we can get working here. Bundle sizeThe bundle size is a valid concern. Altair's current wheel size is dramatically smaller than alternative Python visualization libraries. Altair 5.0.1 wheel: 456k At the moment, this PR would increase the Altair wheel size to 765k. This is a ~70% size increase. But this would go up further when adding the apache-arrow JavaScript library for binary deserialization (maybe up to around a 1MB?). On the one hand, this is still 7x smaller than the next smallest option. On the other hand, that's not necessarily justification for doubling the wheel size. Since part of the discussion here is about Altair's use in wasm environments, it's worth noting that I think the widget should be functional in JupyterLite, so there would be some benefit to some Python wasm users. Online vs OfflineYou make a good point, @mattijn, that reworking this PR to be online-only might be a good compromise as a starting point. This would eliminate the bundle size increase and remove the need for npm and package.json. An interesting consequence of how AnyWidget dynamically loads JS is that it should be possible to swap the online JS for an offline bundle without changing the Python portion of the widget. Maybe we eventually add an There are details to figure out, but I'm liking the idea of including an online version of the widget directly in the Next stepsGiven the change in direction (both of bundling and ChartWidget itself), I think I'll close this PR and then open a new online-only version where we can review the widget implementation itself. Thanks again for all of the engagement on this issue! |
This PR is a WIP example of adding a
ChartWidget
Jupyter Widget for Altair charts based on AnyWidget. See #3106 for some discussion.In this first iteration, I've used the exact widget implementation from the AnyWidget blog post. There's a lot I want to add to this implementation, but starting simple for the sake of discussion.
Usage
Define a chart
Wrap in a
ChartWidget
and wire up a selection callbackBuilding
To support offline usage, I adapted the blog example to use the esbuild bundler as recommend in AnyWidget docs.
@manzt, is this the right idea for using esbuild to bundle everything for offline support?
To build the JavaScript portion of the widget:
This writes a file to
altair/widget/static/index.js
which includes all dependencies inline (it's around 800k minified).The
ChartWidget
class inaltair/widget/__init__.py
references the bundle using the_esm
property.Notes
To get from here to the initial widget functionality discussed in #3106, it would mostly be a matter of adding functionality to the
render
JavaScript function inwidget/src/index.js
and to theChartWidget
Python class inaltair/widget/__init__.py
.I'll keep working on the functionality over the coming weeks, but how do folks feel about including this widget in the Altair repo and package?