-
Notifications
You must be signed in to change notification settings - Fork 505
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
Backward compat for ipywidgets 7 #1487
Conversation
@@ -95,9 +109,7 @@ module.exports = [ | |||
name: ['_JUPYTERLAB', 'CORE_LIBRARY_FEDERATION'] | |||
}, | |||
name: 'CORE_FEDERATION', | |||
shared: { |
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.
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
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.
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?
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.
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?
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.
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.
- Base = voila + ipywidgets 7, Kernel = ipywidgets 8 -> skipped, who would use is non-sense combination 😄
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.
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.
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.
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.
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.
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
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 work if we check for the widget version in the backend and write it to the page template
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.
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.
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.
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.
6e675be
to
be5b3ed
Compare
Closing in favor of #1501 |
Revive #1479
Looking into fixing #1485