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

Refactor JupyterLab JS extensions and package as federated extension #3142

Merged
merged 3 commits into from
May 5, 2021

Conversation

fcollonval
Copy link
Contributor

@fcollonval fcollonval commented Apr 9, 2021

This PR:


  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • For a new feature, I have added documentation examples in an existing or
    new tutorial notebook (please see the doc checklist as well).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

@fcollonval
Copy link
Contributor Author

I pushed this PR as draft to get feedback early.

@nicolaskruchten
Copy link
Contributor

Thanks for this PR, it seems very exciting! I'll try to review it and give you some feedback next week :)

@fcollonval
Copy link
Contributor Author

fcollonval commented Apr 9, 2021

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

@fcollonval
Copy link
Contributor Author

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:

https://github.com/jupyterlab/jupyterlab/blob/3cf91bbf8ea3e2ad56f92efbb76cceb6a0cfa993/jupyterlab/federated_labextensions.py#L160

@fcollonval fcollonval marked this pull request as ready for review April 9, 2021 14:08
@nicolaskruchten
Copy link
Contributor

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

@nicolaskruchten
Copy link
Contributor

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

@fcollonval
Copy link
Contributor Author

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 😃

@nicolaskruchten
Copy link
Contributor

Thinking a bit more about it; actually the CI is about testing python and not the JS

yes exactly.

@jonmmease
Copy link
Contributor

@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 to jupyterlab-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?

@nicolaskruchten
Copy link
Contributor

Also: is this still compatible with how classic non-lab notebook gets access to the widget plugin/extension?

@fcollonval
Copy link
Contributor Author

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 in webpack.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 in webpack.config.js)
  • For JupyterLab, the packaging tool is considered an implementation detail. So developer should use the helper jupyter labextension build and package @jupyterlab/builder to produce the proper asset. The helper will look at the jupyterlab entry in the package.json to discover the entrypoint (here: jupyterlab.extension = lib/jupyterlab-plugin - so the entrypoint is defined in plotlywidget/src/jupyterlab-plugin). The webpack configuration comes from JupyterLab core; hence nothing appears in webpack.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 to jupyterlab-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.

@nicolaskruchten
Copy link
Contributor

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 ?

@fcollonval
Copy link
Contributor Author

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.

@nicolaskruchten
Copy link
Contributor

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?

@heuripedes
Copy link

heuripedes commented Apr 15, 2021

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.

@fcollonval
Copy link
Contributor Author

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?

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",

@nicolaskruchten
Copy link
Contributor

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?

@fcollonval
Copy link
Contributor Author

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

koenlek commented Apr 21, 2021

This is very exciting! Hopefully this lands soon :) Thanks for working on this!

@nicolaskruchten
Copy link
Contributor

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?

@nicolaskruchten
Copy link
Contributor

Also note: once #3160 lands you'll be able to merge master into this branch and have the CI go green again :)

@jonmmease
Copy link
Contributor

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

@jonmmease
Copy link
Contributor

@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!

@nicolaskruchten
Copy link
Contributor

And it renders without error? Hmm. Ok I'll try to see what else I can do here to get things to replicate.

@fcollonval
Copy link
Contributor Author

Something cross my mind, I did not update the version as you did. Could it be you miss a file when updating it (the trickiest one being in packages/python/plotly/plotly/_widget_version.py)?

@fcollonval
Copy link
Contributor Author

Or that the jupyterlab-plotly package.json file included in the webpacked JS files in jupyterlab_plotly/labextension/static is not the one with the latest version number (for me, the file in which the package.json file is inlined is 657.56735d454bda37d90698.js - not sure the name is reproductible).

@nicolaskruchten
Copy link
Contributor

I did indeed bump all versions to 5.0.0 ... Here is my wheel: plotly-5.0.0-py2.py3-none-any.whl.zip ... maybe I missed something though :)

@fcollonval
Copy link
Contributor Author

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

@nicolaskruchten
Copy link
Contributor

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.

@nicolaskruchten
Copy link
Contributor

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.

@nicolaskruchten
Copy link
Contributor

(sorry, unintended close!)

@nicolaskruchten
Copy link
Contributor

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

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?

@nicolaskruchten
Copy link
Contributor

I'm aiming for the end of the month, give or take.

@fcollonval fcollonval deleted the fcollonval/issue3036 branch May 6, 2021 07:00
@nicolaskruchten
Copy link
Contributor

I've just pushed 5.0.0rc1 to PyPI if someone wants to give this a spin :)

@jabbera
Copy link

jabbera commented Jun 1, 2021

working for me!

@koenlek
Copy link

koenlek commented Jun 2, 2021

Works like a charm for me too, thanks!

@aiqc
Copy link

aiqc commented Jun 5, 2021

  • It just took 6min to install 5.0.0rc1. Should this be an optional dependency e.g. PyPI plotly[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

@nicolaskruchten
Copy link
Contributor

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

@fcollonval
Copy link
Contributor Author

@nicolaskruchten I confirm the issue with the tarball. Neither the jupyterlab js assets, nor the classical notebook ones are copied.

@fcollonval
Copy link
Contributor Author

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.

@fcollonval
Copy link
Contributor Author

Xref: #3231

@nicolaskruchten
Copy link
Contributor

This is now released as part of v5.0. Thanks again @fcollonval :)

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

Successfully merging this pull request may close these issues.

JupyterLab prebuilt extensions
8 participants