ipywidgets

ipywidgets copied to clipboard

Description

I have an ipytree widget in Jupyterlab, and on clicking a node of it getting errors:

Error serializing widget state attribute: selected_nodes serialize @ 595.7f2125f3d2c7376588c2.js?v=7f2125f3d2c7376588c2:1 sync @ 595.7f2125f3d2c7376588c2.js?v=7f2125f3d2c7376588c2:1 save @ 644.7d1bff49f8e38fac4070.js?v=7d1bff49f8e38fac4070:1 save_changes @ 595.7f2125f3d2c7376588c2.js?v=7f2125f3d2c7376588c2:1 (anonymous) @ 568.c44c0ae4f70f7df0fe86.js?v=c44c0ae4f70f7df0fe86:1 dispatch @ 755.71bcc770291b01d6ebaa.js?v=71bcc770291b01d6ebaa:2 v.handle @ 755.71bcc770291b01d6ebaa.js?v=71bcc770291b01d6ebaa:2 trigger @ 755.71bcc770291b01d6ebaa.js?v=71bcc770291b01d6ebaa:2 triggerHandler @ 755.71bcc770291b01d6ebaa.js?v=71bcc770291b01d6ebaa:2 trigger @ 287.fd063a1253f3266560f7.js?v=fd063a1253f3266560f7:2 select_node @ 287.fd063a1253f3266560f7.js?v=fd063a1253f3266560f7:2 activate_node @ 287.fd063a1253f3266560f7.js?v=fd063a1253f3266560f7:2 (anonymous) @ 287.fd063a1253f3266560f7.js?v=fd063a1253f3266560f7:2 dispatch @ 755.71bcc770291b01d6ebaa.js?v=71bcc770291b01d6ebaa:2 v.handle @ 755.71bcc770291b01d6ebaa.js?v=71bcc770291b01d6ebaa:2 Uncaught DOMException: Failed to execute 'structuredClone' on 'Window': #<Promise> could not be cloned.

Reproduce

In Jupyterlab (3.4.5):

from ipytree import Node, Tree

tree = Tree() subtree = Node() tree.add_node(subtree) subtree.add_node(Node()) tree

Expected behavior

Context

When downgrading to ipywidgets==8.0.4 and jupyterlab_widgets==3.0.5 versions the widget works as expected without errors.

Paste the output from running `jupyter troubleshoot` from the command line here.
You may want to sanitize the paths in the output.

Paste the output from your command line running `jupyter lab` (or `jupyter notebook` if you use notebook) here, use `--debug` if possible.

Paste the output from your browser Javascript console here.

If using JupyterLab

• JupyterLab version:

JupyterLab v3.4.5
/opt/conda/share/jupyter/labextensions
        jupyterlab_pygments v0.2.2 enabled OK (python, jupyterlab_pygments)
        jupyter-offlinenotebook v0.2.2 enabled OK
        ipytree v0.2.2 enabled OK
        ipylab v0.6.0 enabled OK (python, ipylab)
        @jupyterlab/plugin-playground v0.3.0 enabled OK (python, jupyterlab-plugin-playground)
        @jupyter-widgets/jupyterlab-manager v5.0.6 enabled OK (python, jupyterlab_widgets)

Mar 22 '23 19:03 dg-data

@manzt didn't expect to see this!

Which browser is this?

Mar 22 '23 20:03 maartenbreddels

cc @martinRenou

Mar 22 '23 20:03 maartenbreddels

Interesting. This is certainly related to replacing JSON.parse(JSON.stringify(obj)) with structuredClone(obj). However, I'm almost certain that previously whatever is causing this issue would just be silently deleted from the cloned object.

EDIT: Sure enough, it's trying to serialize a Promise somewhere:

DOMException: Failed to execute 'structuredClone' on 'Window': #<Promise> could not be cloned.

To illustrate:

let p = Promise.resolve("foo");
JSON.parse(JSON.stringify([p, 10])); // [{}, 10]
structuredClone([p, 10]); // Uncaught DOMException: Failed to execute 'structuredClone' on 'Window': #<Promise> could not be cloned.

~EDIT: What version of ipytree? I can't find the activate_node method anywhere.~

Mar 22 '23 21:03 manzt

I think you should be able to fix this in ipytree by implementing a serialize to complement the deserialize for selected_nodes trait, overriding the automatic deepcopy serialization: https://github.com/jupyter-widgets/ipywidgets/blob/93a848b5ad541db08f464a22b15d05abcfe03a63/packages/base/src/widget.ts#L526-L548

TreeModel.serializers = {
  ...widgets.DOMWidgetModel.serializers,
  nodes: { deserialize: widgets.unpack_models },
  selected_nodes: { deserialize: widgets.unpack_models }
};

I think this might actually be a bug with the implementation of TreeModel? If the widget sets model state for a trait which it implements deserialize for, then it should (must?) implement serialize to reverse that process. It's fine to only implement deserialize if you are never setting state on the model.

I believe the correct serializer implementation would be:

TreeModel.serializers = {
  ...widgets.DOMWidgetModel.serializers,
  nodes: { deserialize: widgets.unpack_models },
  selected_nodes: {
    deserialize: widgets.unpack_models,
    serialize(nodes) {
      return nodes.map(model => `IPY_MODEL_${model.model_id}`);
    }
  }
};

Mar 22 '23 21:03 manzt

Thanks for looking into it @manzt !

For the record, the PR that changed the default serialization behavior is https://github.com/jupyter-widgets/ipywidgets/pull/3689

@manzt would you like to open a PR in ipytree to get credits for your fix?

Mar 23 '23 08:03 martinRenou

@manzt would you like to open a PR in ipytree to get credits for your fix?

Done

For the record, the PR that changed the default serialization behavior is https://github.com/jupyter-widgets/ipywidgets/pull/3689

Yup, my (long-winded) explanation above was just pointing to the fact that folks might be relying on unexpected behavior in the prior default serializer (JSON.parse(JSON.stringify(obj))), which has many sharp edges that often fail silently.

Mar 23 '23 13:03 manzt

Making every downstream package update on a point release because "our old implementation was suspect," seems like... an unfriendly move.

Can we restore the appearance of the old behavior, and get the new benefits? Basically, if deserialize === unpack_models is defined, and serialize == null, then use something like the above (except, perhaps, in a dump for loop that doesn't create excessive anonymous functions).

Mar 23 '23 17:03 bollwyvl

I think it is extremely unlikely that every downstream package will need an update due to this change (elaborated below), but I certainly see your point. Perhaps just make the new default cloning re-coverable, wrapping in a try/catch and falling back to JSON.parse(JSON.stringify(obj)) if the structuredClone throws?

To be clear, it's completely fine to only implement deserialize if the JS never sets the value on the model (as is the most common pattern for widgets which wrap other widgets, e.g., HBox, VBox, etc). In that case, serialization is only happening on the Python side, so the client only needs to deserialize. The issue arises if the front-end tries to set a value on the model for which they've only implemented deserialize, which should necessitate the implementation of serialize to reverse deserialize.

Mar 23 '23 18:03 manzt

JS never sets the value on the model

Counterpoint: any library (or user) use of jslink or jsdlink, frequently used to improve responsiveness. This fails with the same error:

from ipywidgets import *
slider = FloatSlider()
box1 = HBox([slider])
box2 = VBox([])
jslink((box1, "children"), (box2, "children"))

Mar 24 '23 14:03 bollwyvl

A quick fix may be to set proper serializers on the the boxes children?

Mar 24 '23 15:03 martinRenou

quick fix

quick fixes are what got us here, let's do it right, and test a bunch of downstreams with the behavior in e.g. binder.

set proper serializers on the the boxes children?

If it's good enough for the goose... again, i think if deserialize === unpack_models, and no serialize is set, it's almost certainly the right move to use the nickle fix. Since unpack_models is part of a singleton package, it should pretty much always work, as can't always be said.

Mar 24 '23 15:03 bollwyvl

Here's a strawman PR: might not be able to bring it home, but do intend to check on binder/lite...

Mar 24 '23 15:03 bollwyvl

Thanks for the PR! Looking into this deeper, I think pack_models was implicit with JSON.stringify(JSON.parse()) because each model instance implements toJSON():

JSON.stringify([ { toJSON: () => "FOO" }, { bar: { toJSON: () => "BAR" },  answer: 42 }])
// '["FOO",{"bar":"BAR","answer":42}]'

so,

JSON.stringify([model1, model2])
// '[ "IPY_MODEL_<model_id>", "IPY_MODEL_<model_id>"]'

I was not aware of this behavior previously, and think your check for deserialize === unpack_models makes a lot of sense, I just wonder if we could have const pack_models = (obj) => JSON.stringify(JSON.parse(obj)).

Mar 24 '23 16:03 manzt

The strawman is looking fine, for something i wrote through the github code editing ui:

I don't think it's safe to assume that toJSON will work for all the cases, hence the complexity of mirroring the structure of unpack_models.

Mar 24 '23 16:03 bollwyvl

The assumption of toJSON why was it worked previously to forgo a custom serializer with objects containing WidgetModels, but I understand the want to mirror unpack_models and make it more explicit.

Mar 24 '23 17:03 manzt

Note that this also broke pythreejs, as it uses its own deserializer like this:

https://github.com/jupyter-widgets/pythreejs/blob/584fe393ff593319ce33cd53dd94c8625aa01c88/js/src/_base/serializers.js#L6-L59

Since the fix in #3738 checks equality directly against unpack_models, that fix doesn't work for pythreejs, meaning that many calls to serialize it fails, since it was also relying on the toJSON behavior of widgets. I'm also not confident in the ability of pack_models to work reliably, since it is non-trivial to traverse contained references in its serialized state. Can we please revert out of using structuredClone? Then we can subsequently look at different alternatives for adding support for dataviews in a backwards compatible manner?

Jun 14 '23 12:06 vidartf

Can we please revert out of using structuredClone? Then we can subsequently look at different alternatives for adding support for dataviews in a backwards compatible manner?

The following is broken since https://github.com/jupyter-widgets/ipywidgets/pull/3689:

• box widget in core ipywidgets
• draw control in ipyleaflet
• the renderer of pythreejs
• ipytree

As a result, I also would be in favor of reverting the PR in a patch release of ipywidgets.

Does that sound good to everyone? Also pinging @ibdafna @mwcraig for opinions

Aug 30 '23 19:08 martinRenou

With the caveat that my understanding of the underlying issue is limited to what I've read here and in the PR #3689, yes, I'm in favor of reverting the change until we can provide a graceful fallback of some sort.

Sep 06 '23 13:09 mwcraig

(the reverts seems to have fixed this for pythreejs, I assume for ipytree as well?)

Nov 10 '23 12:11 vidartf

Yes, they should have.

Nov 10 '23 14:11 manzt