ipywidgets 8.0 support

[jasongrout]

I bumped the version of ipywidgets packages to the 8.0rc0 releases, and things seem to just work.

I also dropped python 3.6 support.

[martinRenou]

@jasongrout I'm going to rebase this. I didn't mean to make a merge commit.

[martinRenou]

UI tests are failing, I can also reproduce locally. But the weird thing is that when trying ipyleaflet outside of playwright's test it works just fine.

[jasongrout]

UI tests are failing, I can also reproduce locally.

Here is the error: Sizes differ; expected image 1480px X 400px, but got 1484px X 400px.

Can we not just update the screenshots?

[martinRenou]

Here is the error: Sizes differ; expected image 1480px X 400px, but got 1484px X 400px.

Actually the actual output doesn't look right. It's missing the Zoom control and the layer "attribution" at the bottom right.

If you look at the videos recorded by playwright show that the Zoom control is visible ~0.5 seconds then disappear.

What's really weird is that @davidbrochart also sees this with xarray-leaflet, but it's using ipywidgets 7. So I'm not sure it's really ipywidgets 8 related.

[martinRenou]

It's probably a race-condition that gets triggered more easily with ipywidgets 8 than 7

[davidbrochart]

Note that I get this error in the console:

Error: Module @jupyter-widgets/base, version ^1.2.0 is not registered, however,         2.0.0 is

[jasongrout]

Note that I get this error in the console:

Error: Module @jupyter-widgets/base, version ^1.2.0 is not registered, however,         2.0.0 is

What version of ipywidgets and ipyleaflet do you have installed? base version 1.2.0 is pretty old, but so is 2.0, so this is surprising.

[davidbrochart]

ipywidgets==7.7.1
ipyleaflet==0.17.0

[jasongrout]

ipywidgets==7.7.1

This should be providing @jupyter-widgets/base version 4, but your error is indicating version 2 is there? (your error message is cut off, so I'm not sure).

ipyleaflet==0.17.0

This should be good with @jupyter-widgets/base 2, 3, or 4 (https://github.com/jupyter-widgets/ipyleaflet/blob/0.17.0/js/package.json#L28)

My guess is that the message is coming from some other widget?

[jasongrout]

Actually the actual output doesn't look right. It's missing the Zoom control and the layer "attribution" at the bottom right.

If you look at the videos recorded by playwright show that the Zoom control is visible ~0.5 seconds then disappear.

Ah, I see in the video what you are saying.

[martinRenou]

Ah, I see in the video what you are saying

I've been trying to understand the cause of this, I found that there is an error occurring in the JavaScript console:
Error setting state: Cannot read properties of undefined (reading 'then')

But I'm not able to get the full traceback (I'm a Playwright newbie)

[jasongrout]

I was able to inspect the chrome debug log with these changes:

diff --git a/ui-tests/package.json b/ui-tests/package.json
index bf8aa8d..01d8ca3 100644
--- a/ui-tests/package.json
+++ b/ui-tests/package.json
@@ -8,7 +8,7 @@
     "start:detached": "yarn run start&",
     "clean": "rimraf tests/notebooks/.ipynb_checkpoints && rimraf test-output",
     "test": "yarn run clean && playwright test",
-    "test:debug": "yarn run clean && PWDEBUG=1  playwright test",
+    "test:debug": "yarn run clean && PWDEBUG=console  playwright test",
     "test:update": "playwright test --update-snapshots"
   },
   "author": "ipyleaflet",
diff --git a/ui-tests/tests/ipyleaflet.test.ts b/ui-tests/tests/ipyleaflet.test.ts
index 47c706b..0a9b465 100644
--- a/ui-tests/tests/ipyleaflet.test.ts
+++ b/ui-tests/tests/ipyleaflet.test.ts
@@ -9,6 +9,7 @@ async function renderMap(fileName: string, page: IJupyterLabPageFixture) {
   await page.notebook.waitForRun();
   const maps = await page.$("div.leaflet-container");
   await new Promise((_) => setTimeout(_, 1000));
+  await page.pause();
   expect(await maps.screenshot()).toMatchSnapshot({
     name: `${fileName}.png`,
   });

I got a short traceback for one of those errors, that points to this line where it seems that state_change is undefined:

https://github.com/jupyter-widgets/ipywidgets/blob/3e6e99d1a92928fe5fc8692666b2cbe5ce7a2c36/packages/base-manager/src/manager-base.ts#L154

Looking at where state_change is defined, I see it is defined in the initialize method: https://github.com/jupyter-widgets/ipywidgets/blob/3e6e99d1a92928fe5fc8692666b2cbe5ce7a2c36/packages/base/src/widget.ts#L129

Perhaps we should set the default value of state_change inline (so it is set in the constructor) rather than waiting around until the initialize method is called. On the other hand, I'm not sure why this model is being used before its initialize method is being called. It seems to be called eventually, in that stepping through the test by hand works great.

[jasongrout]

By the way, it looks like leaflet-heat.js and leaflet-magnifyingglass.js both use the outdated backbone extend method rather than es6 classes.

[jasongrout]

Is there a way to use slowMo mode to see if that resolves any race condition? https://playwright.dev/docs/debug#headed-mode

[jasongrout]

Perhaps we should set the default value of state_change inline (so it is set in the constructor) rather than waiting around until the initialize method is called.

jupyter-widgets/ipywidgets#1498 changed things to set the values in initialize rather than inline (i.e., in the constructor). This idea would be reverting that change, so if we do it, we should make sure it doesn't break something.

[jasongrout]

Debugging tip: I got a good live stack trace for the error by doing jlpm run test:debug in the ui-testing directory. When the window pops up, it pauses. I opened the Chrome debugger and set it to break on errors, then I ran the test. It paused at https://github.com/jupyter-widgets/ipywidgets/blob/3e6e99d1a92928fe5fc8692666b2cbe5ce7a2c36/packages/base-manager/src/manager-base.ts#L154, where it claims that model is 'IPY_MODEL_705193d140764e1fb66f3276129c65d6', i.e., the model has not been instantiated.

[jasongrout]

where it claims that model is 'IPY_MODEL_705193d140764e1fb66f3276129c65d6', i.e., the model has not been instantiated.

jupyter-widgets/ipywidgets#3549 should fix the instance of this bug that I saw.

[jasongrout]

Based on jupyter-widgets/ipywidgets#3549, I think this is a race condition. Here's what I think is happening:

1. Lab widget manager requests state from the kernel
2. Quickly, the code is executed in lab, which creates several widgets
3. The state request is answered from the kernel, which includes these widgets that have already been created in step 2, so the codepath in Deserialize state before setting it, as is done elsewhere set_state is used ipywidgets#3549 is followed, setting the widget state to a wrong value (i.e., child widgets are reset to their widget id because the state wasn't deserialized)
4. Step 3 is interspersed with the promises in the widget initialization, so some widget usually errors out when it tries to, for example, use its child style widget, or its child icon widget, etc.

We don't see this race condition when we are interacting at human speeds because the kernel state request is answered before code is run and widgets are created (i.e., at human speeds, steps 2 and 3 are reversed, so we don't go down the problematic code path)

@martinRenou - can you review jupyter-widgets/ipywidgets#3549?

[martinRenou]

Thanks a lot for investigating this!

[davidbrochart]

Note that I get this error in the console:

Error: Module @jupyter-widgets/base, version ^1.2.0 is not registered, however,         2.0.0 is

I created a widget from widget-ts-cookiecutter with ipywidgets==7.7.1, and even the example widget has this error message, although the widget works fine. Maybe it's because it's a simple widget, but I can't get ipyspin to work anymore.

[davidbrochart]

With ipyspin I also have this error:

Failed to load model class 'HBoxModel' from module '@jupyter-widgets/controls'
Error: Module @jupyter-widgets/controls, version ^1.5.0 is not registered, however,         2.0.0 is
    at f.loadClass (http://localhost:8888/lab/extensions/@jupyter-widgets/jupyterlab-manager/static/134.c8db098d38011369cafa.js?v=c8db098d38011369cafa:1:74977)
    at f.loadModelClass (http://localhost:8888/lab/extensions/@jupyter-widgets/jupyterlab-manager/static/150.a4d4bb4ed1bc1a3626c7.js?v=a4d4bb4ed1bc1a3626c7:1:10655)
    at f._make_model (http://localhost:8888/lab/extensions/@jupyter-widgets/jupyterlab-manager/static/150.a4d4bb4ed1bc1a3626c7.js?v=a4d4bb4ed1bc1a3626c7:1:7451)
    at f.new_model (http://localhost:8888/lab/extensions/@jupyter-widgets/jupyterlab-manager/static/150.a4d4bb4ed1bc1a3626c7.js?v=a4d4bb4ed1bc1a3626c7:1:5137)
    at f.handle_comm_open (http://localhost:8888/lab/extensions/@jupyter-widgets/jupyterlab-manager/static/150.a4d4bb4ed1bc1a3626c7.js?v=a4d4bb4ed1bc1a3626c7:1:3894)
    at _handleCommOpen (http://localhost:8888/lab/extensions/@jupyter-widgets/jupyterlab-manager/static/134.c8db098d38011369cafa.js?v=c8db098d38011369cafa:1:73393)
    at b._handleCommOpen (http://localhost:8888/static/lab/jlab_core.9193dfb13484acaca919.js?v=9193dfb13484acaca919:2:994423)
    at async b._handleMessage (http://localhost:8888/static/lab/jlab_core.9193dfb13484acaca919.js?v=9193dfb13484acaca919:2:996413)

[davidbrochart]

Actually, even an IntSlider is broken on my side with this environment:

ipywidgets==7.1.1
jupyterlab==3.4.5

[davidbrochart]

Pinning jupyterlab-widgets>=1.0.0,<2 solved the issue.

[jasongrout]

Pinning jupyterlab-widgets>=1.0.0,<2 solved the issue.

I think jupyter-widgets/ipywidgets#3551 then fixes this.

[jasongrout]

@martinRenou - can you review jupyter-widgets/ipywidgets#3549?

I've released jupyterlab_widgets 3.0.2 with this fix.

[jasongrout]

Testing against the ipywidgets 8 bugfix, most of the screenshots now look okay (the main change being that the attribution font seems to be larger now) except for:

• Render MagnifyingGlass - no map shows in actual, but does show in expected
• Render TileLayer - no map shows in actual, but does show in expected

I'm not sure if there are errors in those cases, but can I turn the investigation back over to you, @martinRenou?

[jasongrout]

@davidbrochart - I just released ipywidgets 7.7.2 that contains jupyter-widgets/ipywidgets#3551 - can you see if that fixes things for you? Basically, it pulls in jupyterlab_widgets 1 instead of jupyterlab_widgets 3.

I saw that if you were using conda-forge, it was already pulling in the correct dependencies, so we were really only fixing the pypi package with the change above. Also, if you pinned the version of jupyterlab_widgets explicitly, it hopefully would have fixed things for you.

[davidbrochart]

I just released ipywidgets 7.7.2 that contains jupyter-widgets/ipywidgets#3551 - can you see if that fixes things for you?

Yes, I checked that it works in a fresh environment by removing jupyterlab_widgets>=1.0.0,<2 and adding ipywidgets>=7.7.2,<8. I use a conda-forge environment only to get python and pip, but then I install everything with pip for a development install.
Thanks!

[jasongrout]

Yes, I checked that it works in a fresh environment

Thanks for checking our fix!

[jasongrout]

• Render MagnifyingGlass - no map shows in actual, but does show in expected

• Render TileLayer - no map shows in actual, but does show in expected

Looks like in the magnifying case, the zoom level should have made the map blank, but for some reason the expected image actually showed a zoom level as if it was set to something like 1. I've reset the zoom level in the source to see if the new image made more sense. Also, we show the magnifying glass in the expected, but not actual - perhaps the mouse pointer is moved somewhere else in actual?

For the tile layer, for some reason again the actual is a blank map (which I think is correct from doing it manually, it seems to be zoomed into blank ocean), while the expected shows a zoomed in plot I think of England. I've also reset the zoom in this case to show more of the world.

[martinRenou]

I've set the zoom level to be quite high (in the sea) because the image providers tend to change slightly from time to time: moving street names around, changing font size slightly etc. Making our tests very flaky. The sea color not changing much it was more reliable to test against it.

[martinRenou]

but can I turn the investigation back over to you, @martinRenou?

Sure! Thanks a lot for investigating the issues. I'll make sure to make the UI-tests pass and release ipyleaflet.

[martinRenou]

Please update galata references

[jasongrout]

I've set the zoom level to be quite high (in the sea) because the image providers tend to change slightly from time to time: moving street names around, changing font size slightly etc. Making our tests very flaky. The sea color not changing much it was more reliable to test against it.

Ah, that makes a lot of sense. Feel free to change the zoom level back if you like. When I see a blank map, I'm never sure if it is just zoomed in to sea, or if the tile layer didn't load or something.

[jasongrout]

The only test update that still looks odd is ui-tests/tests/ipyleaflet.test.ts-snapshots/MagnifyingGlass-linux.png, where the magnifying glass used to be visible, but now it is not. I tried using playwright to move the mouse onto the map to see if it made the glass visible, but it didn't seem to work. It does seem that the magnifying glass is visible when trying an example by hand, though.

[martinRenou]

Thanks! We should probably remove the magnifying glass test then... Probably something changed upstream that makes it not appear anymore. If we can't move the mouse programmatically let's not fight too much.

[jasongrout]

FYI, this is the change I tried. I didn't investigate too much past making this change and looking at the test result, though, so it's possible the change is wrong and it actually isn't moving the mouse like I intended:

diff --git a/ui-tests/tests/ipyleaflet.test.ts b/ui-tests/tests/ipyleaflet.test.ts
index 47c706b..3ac2b7c 100644
--- a/ui-tests/tests/ipyleaflet.test.ts
+++ b/ui-tests/tests/ipyleaflet.test.ts
@@ -9,7 +9,9 @@ async function renderMap(fileName: string, page: IJupyterLabPageFixture) {
   await page.notebook.waitForRun();
   const maps = await page.$("div.leaflet-container");
   await new Promise((_) => setTimeout(_, 1000));
-  expect(await maps.screenshot()).toMatchSnapshot({
+  const bb = (maps as any).getClientBoundingBox();
+  await page.mouse.move(bb.top + bb.height / 2, bb.left + bb.width / 2);
+  expect(await maps!.screenshot()).toMatchSnapshot({
     name: `${fileName}.png`,
   });
 }

[martinRenou]

Thanks! It looks like you switched the x and y coordinates? I will try with:

const bb = (maps as any).getClientBoundingBox();
await page.mouse.move(bb.left + bb.width / 2, bb.top + bb.height / 2);
expect(await maps!.screenshot()).toMatchSnapshot({
...

[martinRenou]

Please update galata references

[jasongrout]

It looks like you switched the x and y coordinates

Oops, I think you're right. I also wasn't for sure if I was using the right origin/orientation for the coordinates.

[martinRenou]

Please update galata references

[martinRenou]

Let's gooo! Thank you @jasongrout !