Fetch the full widget state via a control Comm #3021

maartenbreddels · 2020-11-19T11:47:27Z

Related issues

martinRenou

LGTM! This works very nicely with voila-dashboards/voila#766 and brings a very good speedup to Voila when there are many widgets.

It's also a simple addition to ipywidgets and doesn't change the normal behavior.

SylvainCorlay · 2021-09-28T17:25:42Z

This was discussed today at the Jupyter widgets call.

For this to get merged, we need to add documentation for this in the protocol documentation.

vidartf · 2021-09-28T17:31:18Z

ipywidgets/widgets/widget.py

+        # the message is also send as buffer, so it does not get handled by jupyter_server
+        msg = jsondumps([full_state, buffer_paths]).encode('utf8')
+        buffers.insert(0, msg)
+        comm.send(buffers=buffers)


What is the reasoning behind this sending the entire state as a "buffer" instead of sending as regular JSON? Is it to sidestep the decoding that happens in the server?

Yes. It has a big performance impact. Although I would agree we don't want to do that and we would want to improve the server instead.

I'm not sure that is the right solution either, I think the fact that the jupyter protocol speaks JSON and the messages we send (the widget protocol) are JSON are a mere coincidence. We should not embed our widget protocol message inside the jupyter protocol as JSON, the JSON from the widget protocol should just be a string or binary buffer in the jupyter message. That way the server would not parse the JSON (it would just be a string). In this case I've chosen a buffer for performance reasons, but a single string would also suffice (i.e. json.dumps(..)). But I don't want to go too much off topic here.

vidartf · 2021-09-28T17:33:20Z

ipywidgets/widgets/tests/test_send_state.py

+    Widget.close_all()
+    w = SimpleWidget()
+    Widget.handle_comm_opened_control(comm, {})
+    assert comm.messages


Ideally the tests would be inspecting the messages a little closer, e.g. check the buffer decoding etc.

jasongrout · 2021-09-28T17:40:01Z

Can we add a version metadata field for the comm_open message, like we do for the jupyter.widget channel? See https://github.com/jupyter-widgets/ipywidgets/blob/master/packages/schema/messages.md#instantiating-a-widget-object-1. This will let us experiment more freely and communicate at the start what the protocol version will be.

jasongrout · 2021-09-28T17:41:53Z

This will let us experiment more freely and communicate at the start what the protocol version will be.

For example, the working around the server performance problems by packaging the JSON in a binary buffer is probably something that can go away in a future iteration.

blois · 2021-10-20T15:19:15Z

Would it make sense to have the control channel message be a request to have the widget state be broadcast on the iopub channel? Since the data can be changing while the messages are in flight there can be a race between the response here and comm messages on iopub. Colab has had similar issues between shell and iopub- even though they are one WebSocket to the client they are still separate ZMQ sockets from the kernel and the messages can get reordered.

jasongrout · 2021-10-26T17:19:17Z

Would it make sense to have the control channel message be a request to have the widget state be broadcast on the iopub channel?

To add context, the current pattern with comm messages is that comm messages from the frontend to the kernel go over the shell channel, and comm messages from the kernel to the frontend go over the iopub channel.

Note that this is not a message on the control zmq channel. It's a new comm target on the existing channels (i.e., it will be coming back on iopub).

vidartf · 2021-11-02T17:28:42Z

ipywidgets/widgets/widget.py

@@ -317,6 +318,27 @@ def _call_widget_constructed(widget):
        if Widget._widget_construction_callback is not None and callable(Widget._widget_construction_callback):
            Widget._widget_construction_callback(widget)

+    @classmethod
+    def handle_comm_opened_control(cls, comm, msg):


I think the general consensus from the dev meeting is that this PR makes sense, but that the logic should not happen on comm open. Rather, the new comm channel can be considered a channel for the widget manager, where "get all the current widget state" would be one type of message that could be sent on it. That allows us to also use this channel for other purposes in the future without always having to transmit all the state.

Done :) Thanks for the review

Totally agree with that, doing it on comm open was just a simple approach, and what @vidartf describes is much better! And thanks for implementing that @martinRenou !

martinRenou

Needs docs for the models message format

ipywidgets/widgets/widget.py

martinRenou · 2021-11-10T08:20:04Z

Added docs + made the messaging format/naming closer to the other channel

vidartf

Logic seems good to me, but I have some minor comments on code/conventions.

I also note that it might be good to have a method on the JS base manager to request the state and handle and/or parse the reply (e.g. put the buffers back in, update the state correctly). Even if we don't use it, it might be useful to have a "reference" implementation, especially if it is written such that it could be used directly by other projects (i.e. voila). It would probably make sense to add this in a follow-up PR though, so no need to add it now.

ipywidgets/widgets/widget.py

martinRenou · 2021-11-10T12:35:03Z

Makes sense! Thanks for the review, I took your comments into account

martinRenou · 2021-11-16T18:01:46Z

Thanks!

martinRenou · 2021-11-16T18:06:14Z

@meeseeksdev please backport to master

lumberbot-app · 2021-11-16T18:06:20Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout master
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -m1 16ffbc9471b35f40585171d8491239ee96b9c654

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #3021: Fetch the full widget state via a control Comm'

Push to a named branch:

git push YOURFORK master:auto-backport-of-pr-3021-on-master

Create a PR against branch master, I would have named this PR:

"Backport PR #3021 on branch master (Fetch the full widget state via a control Comm)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

maartenbreddels mentioned this pull request Nov 19, 2020

Performance: voila takes double the time to create widgets compared to Notebook voila-dashboards/voila#764

Closed

martinRenou self-requested a review September 22, 2021 14:21

martinRenou changed the title ~~WIP refactor: fetch the full widget state via a control Comm~~ Fetch the full widget state via a control Comm Sep 23, 2021

martinRenou marked this pull request as ready for review September 23, 2021 15:15

martinRenou reviewed Sep 23, 2021

View reviewed changes

martinRenou approved these changes Sep 23, 2021

View reviewed changes

vidartf reviewed Sep 28, 2021

View reviewed changes

jtpio mentioned this pull request Sep 29, 2021

2021 Jupyter Widgets Dev Meetings jupyter-widgets/team-compass#1

Closed

martinRenou force-pushed the refactor_widget_fetching branch 4 times, most recently from d2c9083 to 16d0f57 Compare October 13, 2021 09:28

jasongrout self-requested a review October 19, 2021 17:22

vidartf reviewed Nov 2, 2021

View reviewed changes

martinRenou force-pushed the refactor_widget_fetching branch 2 times, most recently from 28ddc59 to d7e9b99 Compare November 9, 2021 10:47

martinRenou reviewed Nov 9, 2021

View reviewed changes

ipywidgets/widgets/widget.py Show resolved Hide resolved

martinRenou force-pushed the refactor_widget_fetching branch from a1010a6 to 341f215 Compare November 10, 2021 10:50

martinRenou mentioned this pull request Nov 10, 2021

Feature: fetch all widgets in one single comm message using the control channel voila-dashboards/voila#766

Merged

vidartf reviewed Nov 10, 2021

View reviewed changes

ipywidgets/widgets/widget.py Show resolved Hide resolved

ipywidgets/widgets/widget.py Outdated Show resolved Hide resolved

ipywidgets/widgets/widget.py Outdated Show resolved Hide resolved

Implement jupyter.widget.control comm channel

430013e

martinRenou force-pushed the refactor_widget_fetching branch from 341f215 to 430013e Compare November 10, 2021 12:34

vidartf merged commit 16ffbc9 into jupyter-widgets:7.x Nov 16, 2021

lumberbot-app bot added the Still Needs Manual Backport label Nov 16, 2021

jtpio added this to the 8.0 milestone Nov 16, 2021

martinRenou mentioned this pull request Nov 16, 2021

Implement jupyter.widget.control comm channel #3313

Merged

jtpio removed this from the 8.0 milestone Nov 16, 2021

jasongrout added this to the 7.7 milestone Feb 8, 2022

jtpio mentioned this pull request Mar 21, 2022

Update shim packaging and metadata jupyterlite/jupyterlite#566

Merged

9 tasks

jasongrout removed the Still Needs Manual Backport label Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch the full widget state via a control Comm #3021

Fetch the full widget state via a control Comm #3021

maartenbreddels commented Nov 19, 2020

martinRenou left a comment

SylvainCorlay commented Sep 28, 2021

vidartf Sep 28, 2021

martinRenou Sep 28, 2021

maartenbreddels Nov 1, 2021

vidartf Sep 28, 2021

jasongrout commented Sep 28, 2021 •

edited

Loading

jasongrout commented Sep 28, 2021

blois commented Oct 20, 2021

jasongrout commented Oct 26, 2021 •

edited

Loading

vidartf Nov 2, 2021 •

edited

Loading

martinRenou Nov 9, 2021

maartenbreddels Nov 9, 2021

martinRenou left a comment

martinRenou commented Nov 10, 2021

vidartf left a comment

martinRenou commented Nov 10, 2021

martinRenou commented Nov 16, 2021

martinRenou commented Nov 16, 2021

lumberbot-app bot commented Nov 16, 2021

Fetch the full widget state via a control Comm #3021

Fetch the full widget state via a control Comm #3021

Conversation

maartenbreddels commented Nov 19, 2020

martinRenou left a comment

Choose a reason for hiding this comment

SylvainCorlay commented Sep 28, 2021

vidartf Sep 28, 2021

Choose a reason for hiding this comment

martinRenou Sep 28, 2021

Choose a reason for hiding this comment

maartenbreddels Nov 1, 2021

Choose a reason for hiding this comment

vidartf Sep 28, 2021

Choose a reason for hiding this comment

jasongrout commented Sep 28, 2021 • edited Loading

jasongrout commented Sep 28, 2021

blois commented Oct 20, 2021

jasongrout commented Oct 26, 2021 • edited Loading

vidartf Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

martinRenou Nov 9, 2021

Choose a reason for hiding this comment

maartenbreddels Nov 9, 2021

Choose a reason for hiding this comment

martinRenou left a comment

Choose a reason for hiding this comment

martinRenou commented Nov 10, 2021

vidartf left a comment

Choose a reason for hiding this comment

martinRenou commented Nov 10, 2021

martinRenou commented Nov 16, 2021

martinRenou commented Nov 16, 2021

lumberbot-app bot commented Nov 16, 2021

jasongrout commented Sep 28, 2021 •

edited

Loading

jasongrout commented Oct 26, 2021 •

edited

Loading

vidartf Nov 2, 2021 •

edited

Loading