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

Backward compat for ipywidgets 7 #1487

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Aug 19, 2024

Revive #1479

Looking into fixing #1485

Copy link
Contributor

Binder 👈 Launch a Binder on branch martinRenou/voila/ipywidgets7_support

@@ -95,9 +109,7 @@ module.exports = [
name: ['_JUPYTERLAB', 'CORE_LIBRARY_FEDERATION']
},
name: 'CORE_FEDERATION',
shared: {
Copy link
Member Author

@martinRenou martinRenou Aug 19, 2024

Choose a reason for hiding this comment

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

I'm having issues with the sharing logic which I'm not sure how to tackle.

We would need to have @jupyter-widgets/base be a shared package, but the version being loaded in the page would depend on the current ipywidgets version (model version requested by the kernel).
In the current state of the PR, @jupyter-widgets/[email protected] is loaded.

Would love to chat with you @jtpio or @trungleduc on this.

I will this PR as draft for now

Copy link
Member

Choose a reason for hiding this comment

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

what is the use case we want to support? ipywidgets 7 and voila are installed in the same kernel env or ipywidgets 8 + voila in the base env and kernel env is using ipywidgets 7?

Copy link
Member Author

@martinRenou martinRenou Aug 19, 2024

Choose a reason for hiding this comment

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

Voila in the base env and either ipywidgets 7 or 8 in the kernel env basically. Resulting in having Voila support both ipywidgets 7 or 8 if the kernel env is the voila base env.

For Voila 0.4.x that was easy using lazy loading of packages, and for some reason the package sharing logic did not come in my way when doing #1482. Maybe because the build setup did not use webpack federated packages?

Now in this PR, for some reason the webpack sharing package logic makes it that @jupyter-widgets/[email protected] is in the page and shared, which incompatible with @jupyter-widgets/controls for ipywidgets 7 so you can't create a VBox widget for example using ipywidgets 7 in the kernel. And we can't decide at build time which version of @jupyter-widgets/base should be loaded and shared, I'd like to do that at runtime. Maybe overwriting __webpack_share_scopes__ at runtime in some way during the lazy load of the widget front-end?

Copy link
Member

@trungleduc trungleduc Aug 19, 2024

Choose a reason for hiding this comment

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

I tested your PR with these configurations:

  • Base = voila + ipywidgets7 + ipyleaflet 0.16.0, Kernel = Base -> work
  • Base = voila + ipywidgets 8 + ipyleaflet 0.19.2, Kernel = Base -> work
  • Base = voila + ipywidgets 8 + ipyleaflet 0.19.2, Kernel = ipywidgets 7 + ipyleaflet 0.16.0 -> ipywidgets works, ipyleaflet does not, which is understandable.

vl7

  • Base = voila + ipywidgets 7, Kernel = ipywidgets 8 -> skipped, who would use is non-sense combination 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your testing! Yeah hopefully most cases would work, the one that does not work is:

Base = voila + ipywidgets7, Kernel = Base -> VBox does not work.

This is because of the issue I'm trying to explain up there in this thread with mismatching of versions between @jupyter-widgets/base and controls. We have base for 8 and controls for 7 in the page, thankfully most widgets do work in this case, but I'd really prefer to have base for 7 in the page in that case. And that's what I'm failing to figure out.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I see. I think we should do the minimum for the ipywidgets backward compatible in Voila. The long-term solution is to adopt the voici/jupyterlab execution model. So that we only need to fix the backward issue once.

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned by @maartenbreddels in jupyter-widgets/ipywidgets#3932

In solara (because we run in the same process), we know beforehand at the server what the version of ipywidgets is, which allows for a different strategy.
We also build the final bundle twice using the webpack alias trick:
https://github.com/widgetti/solara/blob/20b2e8012e7683eb38ff62a74a371eb1a052bf41/packages/solara-vuetify-app/webpack.config.js#L95

I wonder if the two webpack bundles approach could suit us, one bundle with @jupyter-widgets/base for 7 being shared, another bundle with 8. And we load the bundle upon widget request depending on the widget version.

So basically very similar to your suggestion @trungleduc

I mean bundling base7 and controls7 into a standalone file then you import base7 and controls7 from this file in your ipywidgets7 plugins to register the classes

While making sure that the shared package is different for each bundle

Copy link
Member

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 work if we check for the widget version in the backend and write it to the page template

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah no I don't think checking in the back-end is a viable solution, ipywidgets may not be installed and if it is we don't know if it's the same version as in the kernel.

We'd still need to rely on the lazy loading logic from the widgets register, and lazy load one bundle or the other depending on what the kernel requests.

Copy link
Member

Choose a reason for hiding this comment

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

theoretically, we can peek at the kernel before generating the template, it can be the last resort if the lazy loading logic does not work.

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

Successfully merging this pull request may close these issues.

2 participants