Error with 8.0.5 when selecting ipytree node

[dg-data]

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': # 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.

Troubleshoot Output

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

Command Line Output

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

Browser Output

Paste the output from your browser Javascript console here.

If using JupyterLab

• JupyterLab version:

Installed Labextensions

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)

[maartenbreddels]

@manzt didn't expect to see this!

Which browser is this?

[maartenbreddels]

cc @martinRenou

[manzt]

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.

[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:

ipywidgets/packages/base/src/widget.ts

Lines 526 to 548 in 93a848b

	serialize(state: Dict<any>): JSONObject {
	const deepcopy =
	globalThis.structuredClone || ((x: any) => JSON.parse(JSON.stringify(x)));
	const serializers =
	(this.constructor as typeof WidgetModel).serializers || {};
	for (const k of Object.keys(state)) {
	try {
	if (serializers[k] && serializers[k].serialize) {
	state[k] = serializers[k].serialize!(state[k], this);
	} else {
	// the default serializer just deep-copies the object
	state[k] = deepcopy(state[k]);
	}
	if (state[k] && state[k].toJSON) {
	state[k] = state[k].toJSON();
	}
	} catch (e) {
	console.error('Error serializing widget state attribute: ', k);
	throw e;
	}
	}
	return state;
	}

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}`);
    }
  }
};

[martinRenou]

Thanks for looking into it @manzt !

For the record, the PR that changed the default serialization behavior is #3689

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

[manzt]

@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 #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.

[bollwyvl]

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).

[manzt]

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.

[bollwyvl]

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"))

[martinRenou]

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

[bollwyvl]

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.

[bollwyvl]

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

[manzt]

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)).

[bollwyvl]

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.

[manzt]

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.

[vidartf]

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?

[martinRenou]

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 #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

[mwcraig]

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.

[vidartf]

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

[manzt]

Yes, they should have.