-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Refactor JupyterLab JS extensions and package as federated extension #3142
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
Refactor JupyterLab JS extensions and package as federated extension #3142
Conversation
I pushed this PR as draft to get feedback early.
Thanks for this PR, it seems very exciting! I'll try to review it and give you some feedback next week :)
This is working locally (削除) but I have trouble with out of memory kill signal on CircleCI. Help will be welcomed to fix that. (削除ここまで)
Reference: Exit code 137 => out of memory - https://discuss.circleci.com/t/every-build-fails-with-received-killed-signal-during-apk-build/25627/6
Ok I got a first version - but the remaining errors raise two questions:
- python-3.7-core error are related to pandas - what would be your recommendation to fix the test:
plotly/tests/test_core/test_px/test_px_hover.py:163
if not isinstance(dtype, (np.dtype, type(np.dtype))):
> dtype = dtype.dtype
E AttributeError: type object 'object' has no attribute 'dtype'
.tox/py37-core/lib/python3.7/site-packages/pandas/core/dtypes/cast.py:1175: AttributeError
- a more important problem: JupyterLab 3 provides a helper to create the federated package
jupyter labextension build. But JupyterLab 3 requires Python >=3.6. Hence the error for python 2.7 and 3.5 in the CI. So should we shortcut the helper to call the final node command:
Thanks again! I think we might be dropping 2.7/3.5 support very soon (i.e. in the next version, which will be 5.0) so this problem might just go away :)
And even if we choose to keep 2.7/3.5 support in 5.0, we can probably just not run these bits in those CI jobs :)
Thinking a bit more about it; actually the CI is about testing python and not the JS. So we could implement a option like --skip-npm in jupyter-packaging. This will have two benefits: remove that dependency problem with JupyterLab 3 for Python < 3.6 and speed-up the CI 😃
Thinking a bit more about it; actually the CI is about testing python and not the JS
yes exactly.
@fcollonval, thanks so much for working on this! A couple of general thoughts / questions:
- Do I understand correctly that the combined extension would be named
plotlywidget?. Is there a reason to prefer that name tojupyterlab-plotly? I think I'd prefer to keep the latter if possible. - Possibly related to the above, will this package structure still be compatible with how Voila and nbviewer look up third party widgets?
Also: is this still compatible with how classic non-lab notebook gets access to the widget plugin/extension?
Thanks for looking into this.
I'll try to describe as clearly as possible the logic behind a typical Jupyter widget repository. Then your questions should be easy to answer.
Most Jupyter widget package are wrappers of existing JS library inside a Backbone model and views to allow communication with the Python model. This package is defining that wrapper in packages/javascript/plotlywidget; more precisely in the Figure file. Loading that widget asset in a notebook frontend will depend of the frontend because the logic to load external assets differs from one tool to another. So to deal with the various scenarii while sharing as much code as possible for the various frontends, webpack is used to produce various artifacts with various entrypoints:
- For the classical notebook, the entry point is defined by
plotlywidget/src/extensions(first export entry inwebpack.config.js) - For voila/nbviewer, their code will load the needed package on the CDN unpkg - using require.js. The entry point is
plotlywidget/src/index(second export entry inwebpack.config.js) - For JupyterLab, the packaging tool is considered an implementation detail. So developer should use the helper
jupyter labextension buildand package@jupyterlab/builderto produce the proper asset. The helper will look at thejupyterlabentry in thepackage.jsonto discover the entrypoint (here:jupyterlab.extension=lib/jupyterlab-plugin- so the entrypoint is defined inplotlywidget/src/jupyterlab-plugin). The webpack configuration comes from JupyterLab core; hence nothing appears inwebpack.config.js- although the available webpack should support federated module (i.e. v5 or more).
So the code proposed in this PR will generate the entrypoints for those 3 frontends (once the python and npm packages are published) - I checked it worked in the classical notebook and JupyterLab.
About the render mime extension, JupyterLab extension can provide multiple plugins that can be packaged within the same node package. For the render mime extension, the situation is singular as it needs its own entrypoint - jupyterlab.mimeExtension defined in package.json. So the only action of this PR was to merge the two packages as it will clarify the code and avoid to get plotly js installed twice in JupyterLab.
Do I understand correctly that the combined extension would be named
plotlywidget?. Is there a reason to prefer that name tojupyterlab-plotly? I think I'd prefer to keep the latter if possible.
As the widget code is not tied to a specific frontend, I kept plotlywidget rather than jupyterlab-plotly.
Possibly related to the above, will this package structure still be compatible with how Voila and nbviewer look up third party widgets?
Yes
Also: is this still compatible with how classic non-lab notebook gets access to the widget plugin/extension?
Yes
A final comment - webpack has the nice feature to drop code unneeded depending on the entrypoint. This means in particular that the JupyterLab code is not part of the classical notebook extension bundle.
Very cool, thanks for the explanation! I don't think plotlywidget is the right name for this extension, though, because you'll still need to install it if you want to use JupyterLab without any widgets i.e. just with fig.show() right? I think I'd favour keeping the jupyterlab-plotly name or coming up with something else generic that's not already taken... I think jupyter-plotly is some other older thing right @jonmmease ?
you'll still need to install it if you want to use JupyterLab without any widgets i.e. just with
fig.show()right?
Indeed you do
I let you settle on the new name. Than I'm happy to update the PR with it.
Thanks!
Assuming we come up with a new name that's not jupyterlab-plotly or plotlywidget, I assume that the instructions to users will need to include "uninstall any pre-existing jupyterlab-plotly and plotlywidget extensions", correct?
Also, for JupyterLab 1 and 2, the old-style installation is meant to still work, right? We'll push the new extension to NPM under its new name, and jupyter labextension install <newname> will work?
Since yesterday, we're currently testing this for a graduate level class of 20 students. Everything has been smooth so far. We're using Anaconda's Python 3.8.8 with Jupyter Lab 3 and Dash.
Assuming we come up with a new name that's not
jupyterlab-plotlyorplotlywidget, I assume that the instructions to users will need to include "uninstall any pre-existingjupyterlab-plotlyandplotlywidgetextensions", correct?
That will be safer indeed.
Also, for JupyterLab 1 and 2, the old-style installation is meant to still work, right? We'll push the new extension to NPM under its new name, and
jupyter labextension install <newname>will work?
Yes the old installation mechanism is still supported in JupyterLab 3. So the new package will be compatible using jupyter labextension install <newname> as the JS dependencies are covering 1, 2 and 3:
"@jupyter-widgets/base": "^2.0.0 || ^3.0.0 || ^4.0.0",
"@jupyterlab/rendermime-interfaces": "^1.3.0 || ^2.0.0 || ^3.0.0",
That will be safer indeed.
OK, thanks for the confirmation. Do you have a sense of what the behaviour would be if you had both the old extensions and the new one installed? Is there anything we can do to the new one to have it have priority or otherwise complain if the older ones are installed as well?
Do you have a sense of what the behaviour would be if you had both the old extensions and the new one installed?
For the widget, we should be fine as it checks the JS module and version constrains from the Python package. So end users will need to execute old notebook to see the plotly graphs; like for any update.
For the renderer, their is a rank that can be used to create a hierarchy of the available renderer. Unfortunately the default value 0 (use here too) means highest priority. So it requires testing to see what will happen in that case.
Is there anything we can do to the new one to have it have priority or otherwise complain if the older ones are installed as well?
I never look into the plugin code in details - but it may be possible to use hasPlugin(id) method of JupyterLab to emit a warning regarding the renderer.
koenlek
commented
Apr 21, 2021
This is very exciting! Hopefully this lands soon :) Thanks for working on this!
OK so I think we'd like to call the unified extension jupyterlab-plotly in the end. My understanding was that if JLab sees a JS extension and a Python extension of the same name, the Python one will be used, right? And then for the widget side of things, the version constrains should make it such that the Python extension will win also?
Also note: once #3160 lands you'll be able to merge master into this branch and have the CI go green again :)
@fcollonval, we've done a bunch of CI cleanup that's causing conflicts here. I'm going to work on resolving those and to see if I can get the tests passing.
I'll also port #2771 into the updated version of FigureWidget. Is it alright if I push to this branch and let you know when I'm done?
@fcollonval, I got the tests fixed up in #3169.
When you have a chance, could you take a look at that branch and the squash merge it into this one? I think that, along with the name change back to jupyterlab-plotly, is about all that's left here. Thanks!
191a548 to
a6c6d99
Compare
I'm lost. I created a fresh environment:
name: dplotly
channels:
- conda-forge
- defaults
dependencies:
- pandas
- jupyterlab
- ipywidgets
Then install your wheel => the plotly code worked
Then install hvplot with pip install hvplot => the test code worked
Then install hvplot with pip install hvplot => the test code worked
And after installing hvplot, just trying to display a simple go.FigureWidget() also works? That's what fails for me.
OK, so in a totally clean environment, I can't replicate this either. I'm trying to figure out what's different about my big/complex environment vs a totally clean one.
(sorry, unintended close!)
OK, I think this is good to merge! I'll keep trying to get to the bottom of why I'm seeing this pyviz_comms thing in one of my environments, but that shouldn't block the merging of this excellent PR :)
I've checked things out in both JupyterLab 2 and 3, with and without the older version of the plotlywidget extension installed and everything checks out.
dhirschfeld
commented
May 5, 2021
Hi 👋
I'm having problems with the current non-federated plotly extension so I'm very keen to try this out. Is there any rough idea when this functionality might make it into a release?
I'm aiming for the end of the month, give or take.
I've just pushed 5.0.0rc1 to PyPI if someone wants to give this a spin :)
jabbera
commented
Jun 1, 2021
working for me!
koenlek
commented
Jun 2, 2021
Works like a charm for me too, thanks!
-
It just took 6min to install
5.0.0rc1. Should this be an optional dependency e.g. PyPIplotly[jupyterlab]so that everyday plotly users don't revolt? -
Noticed that when uninstalling
plotly==5.0.0rc1, it does not uninstall the lab extension. I assume the existing extension would be overwritten when upgrading the plotly package?
^ @nicolaskruchten feedback
@aiqc this package is known to sometimes take a long time to install but not mostly because of this extension as far as I know (it's because it has so many files I think, and is usually reported on Windows).
@fcollonval question: I tried to pip install <tarball> and the labextension wasn't automatically installed. Is this by design/a known limitation?
@nicolaskruchten I confirm the issue with the tarball. Neither the jupyterlab js assets, nor the classical notebook ones are copied.
This looks like the update_package_data function in setup.py is not called when using the tarball (probably because npm is not available). So pip does not see the JS assets.
Xref: #3231
This is now released as part of v5.0. Thanks again @fcollonval :)
Uh oh!
There was an error while loading. Please reload this page.
This PR:
plotly.graph_objects, my modifications concern thecodegenfiles and not generated files.modified existing tests.
new tutorial notebook (please see the doc checklist as well).