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.

Sign up for GitHub

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

Jump to bottom

Backward compat for ipywidgets 7 #1487

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

Backward compat for ipywidgets 7 #1487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions .github/workflows/ui-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python-version: [3.8]
node-version: [18.x]
ipywidgets: [7, 8]
fail-fast: false

steps:
Expand All @@ -18,10 +17,21 @@ jobs:
uses: jupyterlab/maintainer-tools/.github/actions/base-setup@v1

- name: Install dependencies
if: ${{ matrix.ipywidgets == 7 }}
run: |
python -m pip install -r requirements-visual-test.txt
python -m pip install jupyterlab_miami_nights --no-deps
python -m pip install ".[test7]"

- name: Install dependencies
if: ${{ matrix.ipywidgets == 8 }}
run: |
python -m pip install -r requirements-visual-test.txt
python -m pip install jupyterlab_miami_nights --no-deps
python -m pip install ".[test]"

- name: Build voila
run: |
jlpm
jlpm build
jupyter labextension develop . --overwrite
Expand Down
3 changes: 3 additions & 0 deletions packages/voila/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,15 @@
"browserslist": ">0.8%, not ie 11, not op_mini all, not dead",
"dependencies": {
"@jupyter-widgets/base": "^6.0.6",
"@jupyter-widgets/base7": "npm:@jupyter-widgets/[email protected]",
"@jupyter-widgets/controls7": "npm:@jupyter-widgets/[email protected]",
"@jupyter-widgets/jupyterlab-manager": "^5.0.9",
"@jupyterlab/application": "^4.0.0",
"@jupyterlab/apputils": "^4.0.0",
"@jupyterlab/apputils-extension": "^4.0.0",
"@jupyterlab/codemirror": "^4.0.3",
"@jupyterlab/codemirror-extension": "^4.0.0",
"@jupyterlab/console": "^4.0.0",
"@jupyterlab/coreutils": "^6.0.0",
"@jupyterlab/docregistry": "^4.0.0",
"@jupyterlab/javascript-extension": "^4.0.0",
Expand Down
61 changes: 61 additions & 0 deletions packages/voila/src/ipywidgets7.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/***************************************************************************
* Copyright (c) 2024, Voilà contributors *
* Copyright (c) 2024, QuantStack *
* *
* Distributed under the terms of the BSD 3-Clause License. *
* *
* The full license is in the file LICENSE, distributed with this software. *
****************************************************************************/

import * as base from '@jupyter-widgets/base';
import {
JupyterFrontEnd,
JupyterFrontEndPlugin
} from '@jupyterlab/application';

const JUPYTER_WIDGETS_VERSION = '1.2.0';
const JUPYTER_CONTROLS_VERSION = '1.5.0';

/**
* The base widgets.
*/
export const baseWidgets7Plugin: JupyterFrontEndPlugin<void> = {
id: `@jupyter-widgets/jupyterlab-manager:base-${JUPYTER_WIDGETS_VERSION}`,
requires: [base.IJupyterWidgetRegistry],
autoStart: true,
activate: (
app: JupyterFrontEnd,
registry: base.IJupyterWidgetRegistry
): void => {
registry.registerWidget({
name: '@jupyter-widgets/base',
version: JUPYTER_WIDGETS_VERSION,
exports: () => {
return require('@jupyter-widgets/base7') as any;
}
});
}
};

/**
* The control widgets.
*/
export const controlWidgets7Plugin: JupyterFrontEndPlugin<void> = {
id: `@jupyter-widgets/jupyterlab-manager:controls-${JUPYTER_CONTROLS_VERSION}`,
requires: [base.IJupyterWidgetRegistry],
autoStart: true,
activate: (
app: JupyterFrontEnd,
registry: base.IJupyterWidgetRegistry
): void => {
registry.registerWidget({
name: '@jupyter-widgets/controls',
version: JUPYTER_CONTROLS_VERSION,
exports: () => {
const controlsWidgets = require('@jupyter-widgets/controls7') as any;
require('@jupyter-widgets/controls7/css/widgets-base.css');
return controlsWidgets;
}
});
}
};
7 changes: 6 additions & 1 deletion packages/voila/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { PageConfig, URLExt } from '@jupyterlab/coreutils';

import { VoilaApp } from './app';
import plugins from './voilaplugins';
import { baseWidgets7Plugin, controlWidgets7Plugin } from './ipywidgets7';
import { VoilaServiceManager } from './services/servicemanager';
import { VoilaShell } from './shell';
import {
Expand Down Expand Up @@ -42,7 +43,10 @@ async function main() {
require('@jupyter-widgets/jupyterlab-manager/lib/plugin').default.filter(
(p: any) => p.id !== '@jupyter-widgets/jupyterlab-manager:plugin'
),
plugins
plugins,
// For backward compat with ipywidgets 7
baseWidgets7Plugin,
controlWidgets7Plugin
];

if (shouldUseMathJax2()) {
Expand Down Expand Up @@ -85,6 +89,7 @@ async function main() {
}

const data = p.value;

if (data.extension) {
federatedExtensionPromises.push(createModule(data.name, data.extension));
}
Expand Down
4 changes: 4 additions & 0 deletions packages/voila/src/sharedscope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// Ideally we would remove this but the ipywidgets labextension requires it
import '@jupyterlab/notebook';
import '@jupyterlab/console';

import '@lumino/algorithm';
import '@lumino/application';
import '@lumino/coreutils';
Expand Down
18 changes: 15 additions & 3 deletions packages/voila/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,20 @@ const distRoot = path.resolve(
'static'
);

const shared = {};
for (const dependency of Object.keys(data.dependencies)) {
// TODO Why can we not share those?
if (
['@jupyter-widgets/base7', '@jupyter-widgets/controls7'].includes(
dependency
)
) {
continue;
}

shared[dependency] = data.dependencies[dependency];
}

module.exports = [
merge(baseConfig, {
mode: 'development',
Expand All @@ -95,9 +109,7 @@ module.exports = [
name: ['_JUPYTERLAB', 'CORE_LIBRARY_FEDERATION']
},
name: 'CORE_FEDERATION',
shared: {
Copy link
Member Author

@martinRenou martinRenou Aug 19, 2024 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

...data.dependencies
}
shared
})
],
resolve: {
Expand Down
10 changes: 10 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,16 @@ test = [
"pytest",
"pytest-tornasync",
]
test7 = [
"ipykernel",
"ipywidgets==7.8.2",
"matplotlib",
"numpy",
"pandas",
"papermill",
"pytest",
"pytest-tornasync",
]
docs = [
"myst-parser",
"pydata-sphinx-theme",
Expand Down
Loading
Loading