Echo state updates in a backwards-compatible way

[jasongrout]

This updates the update echo implementation to be backwards compatible with 7.x.

Fixes #3392

From the new docs in messages.md:

Starting with protocol version 2.1.0, echo_update messages from the kernel to the frontend are optional update messages for echoing state in messages from a frontend to the kernel back out to all the frontends.

{
  'comm_id' : 'u-u-i-d',
  'data' : {
    'method': 'echo_update',
    'state': { <dictionary of widget state> },
    'buffer_paths': [ <list with paths corresponding to the binary buffers> ]
  }
}

The Jupyter comm protocol is asymmetric in how messages flow: messages flow from a single frontend to a single kernel, but messages are broadcast from the kernel to all frontends. In the widget protocol, if a frontend updates the value of a widget, the frontend does not have a way to directly notify other frontends about the state update. The echo_update optional messages enable a kernel to broadcast out frontend updates to all frontends. This can also help resolve the race condition where the kernel and a frontend simultaneously send updates to each other since the frontend now knows the order of kernel updates.

The echo_update messages enable a frontend to optimistically update its widget views to reflect its own changes that it knows the kernel will yet process. These messages are intended to be used as follows:

1. A frontend model attribute is updated, and the frontend views are optimistically updated to reflect the attribute.
2. The frontend queues an update message to the kernel and records the message id for the attribute.
3. The frontend ignores updates to the attribute from the kernel contained in echo_update messages until it gets an echo_update message corresponding to its own update of the attribute (i.e., the parent_header id matches the stored message id for the attribute). It also ignores echo_update updates if it has a pending attribute update to send to the kernel. Once the frontend receives its own echo_update and does not have any more pending attribute updates to send to the kernel, it starts applying attribute updates from echo_update messages.

Since the echo_update update messages are optional, and not all attribute updates may be echoed, it is important that only echo_update updates are ignored in the last step above, and update message updates are always applied.

Implementation note: For attributes where sending back an echo_update is considered too expensive or unnecessary, we have implemented an opt-out mechanism in the ipywidgets package. A model trait can have the echo_update metadata attribute set to False to flag that the kernel should never send an echo_update update for that attribute to the frontends. Additionally, we have a system-wide flag to disable echoing for all attributes via the environment variable JUPYTER_WIDGETS_ECHO. For ipywdgets 7.7, we default JUPYTER_WIDGETS_ECHO to off (disabling all echo messages) and in ipywidgets 8.0 we default JUPYTER_WIDGETS_ECHO to on (enabling echo messages).

[github-actions]

👈 Launch a binder notebook on branch jasongrout/ipywidgets/echostate

[jasongrout]

Here are the relevant diffs to pre-#3195:

widget.py

diff --git a/python/ipywidgets/ipywidgets/widgets/widget.py b/python/ipywidgets/ipywidgets/widgets/widget.py
index 3cc791ed..2326de34 100644
--- a/python/ipywidgets/ipywidgets/widgets/widget.py
+++ b/python/ipywidgets/ipywidgets/widgets/widget.py
@@ -5,7 +5,7 @@
 """Base Widget class.  Allows user to create widgets in the back-end that render
 in the Jupyter notebook front-end.
 """
-
+import os
 from contextlib import contextmanager
 from collections.abc import Iterable
 from IPython import get_ipython
@@ -18,8 +18,22 @@ from json import loads as jsonloads, dumps as jsondumps
 from base64 import standard_b64encode
 
 from .._version import __protocol_version__, __control_protocol_version__, __jupyter_widgets_base_version__
+
+# Based on jupyter_core.paths.envset
+def envset(name, default):
+    """Return True if the given environment variable is turned on, otherwise False
+    If the environment variable is set, True will be returned if it is assigned to a value
+    other than 'no', 'n', 'false', 'off', '0', or '0.0' (case insensitive).
+    If the environment variable is not set, the default value is returned.
+    """
+    if name in os.environ:
+        return os.environ[name].lower() not in ['no', 'n', 'false', 'off', '0', '0.0']
+    else:
+        return bool(default)
+
 PROTOCOL_VERSION_MAJOR = __protocol_version__.split('.')[0]
 CONTROL_PROTOCOL_VERSION_MAJOR = __control_protocol_version__.split('.')[0]
+JUPYTER_WIDGETS_ECHO = envset('JUPYTER_WIDGETS_ECHO', default=True)
 
 def _widget_to_json(x, obj):
     if isinstance(x, dict):
@@ -549,6 +563,21 @@ class Widget(LoggingHasTraits):
 
     def set_state(self, sync_data):
         """Called when a state is received from the front-end."""
+        # Send an echo update message immediately
+        if JUPYTER_WIDGETS_ECHO:
+            echo_state = {}
+            for attr,value in sync_data.items():
+                if not self.trait_metadata(attr, 'no_echo'):
+                    echo_state[attr] = value
+            if echo_state:
+                echo_state, echo_buffer_paths, echo_buffers = _remove_buffers(echo_state)
+                msg = {
+                    'method': 'echo_update',
+                    'state': echo_state,
+                    'buffer_paths': echo_buffer_paths,
+                }
+                self._send(msg, buffers=echo_buffers)
+
         # The order of these context managers is important. Properties must
         # be locked when the hold_trait_notification context manager is
         # released and notifications are fired.

and

widget.ts

diff --git a/packages/base/src/widget.ts b/packages/base/src/widget.ts
index 1bee2531..54ff15e1 100644
--- a/packages/base/src/widget.ts
+++ b/packages/base/src/widget.ts
@@ -115,6 +115,9 @@ export class WidgetModel extends Backbone.Model {
     attributes: Backbone.ObjectHash,
     options: IBackboneModelOptions
   ): void {
+    this.expectedEchoMsgIds = new Map<string, string>();
+    this.attrsToUpdate = new Set<string>();
+
     super.initialize(attributes, options);
 
     // Attributes should be initialized here, since user initialization may depend on it
@@ -221,13 +224,46 @@ export class WidgetModel extends Backbone.Model {
     const method = data.method;
     switch (method) {
       case 'update':
+      case 'echo_update':
         this.state_change = this.state_change
           .then(() => {
             const state = data.state;
-            const buffer_paths = data.buffer_paths || [];
-            const buffers = msg.buffers || [];
+            const buffer_paths = data.buffer_paths ?? [];
+            const buffers = msg.buffers?.slice(0, buffer_paths.length) ?? [];
             utils.put_buffers(state, buffer_paths, buffers);
+
+            if (msg.parent_header && method === 'echo_update') {
+              const msgId = (msg.parent_header as any).msg_id;
+              // we may have echos coming from other clients, we only care about
+              // dropping echos for which we expected a reply
+              const expectedEcho = Object.keys(state).filter((attrName) =>
+                this.expectedEchoMsgIds.has(attrName)
+              );
+              expectedEcho.forEach((attrName: string) => {
+                // Skip echo messages until we get the reply we are expecting.
+                const isOldMessage =
+                  this.expectedEchoMsgIds.get(attrName) !== msgId;
+                if (isOldMessage) {
+                  // Ignore an echo update that comes before our echo.
+                  delete state[attrName];
+                } else {
+                  // we got our echo confirmation, so stop looking for it
+                  this.expectedEchoMsgIds.delete(attrName);
+                  // Start accepting echo updates unless we plan to send out a new state soon
+                  if (
+                    this._msg_buffer !== null &&
+                    Object.prototype.hasOwnProperty.call(
+                      this._msg_buffer,
+                      attrName
+                    )
+                  ) {
+                    delete state[attrName];
+                  }
+                }
+              });
+            }
             return (this.constructor as typeof WidgetModel)._deserialize_state(
+              // Combine the state updates, with preference for kernel updates
               state,
               this.widget_manager
             );
@@ -300,7 +336,11 @@ export class WidgetModel extends Backbone.Model {
         this._pending_msgs--;
         // Send buffer if one is waiting and we are below the throttle.
         if (this._msg_buffer !== null && this._pending_msgs < 1) {
-          this.send_sync_message(this._msg_buffer, this._msg_buffer_callbacks);
+          const msgId = this.send_sync_message(
+            this._msg_buffer,
+            this._msg_buffer_callbacks
+          );
+          this.rememberLastUpdateFor(msgId);
           this._msg_buffer = null;
           this._msg_buffer_callbacks = null;
         }
@@ -415,6 +455,10 @@ export class WidgetModel extends Backbone.Model {
       }
     }
 
+    Object.keys(attrs).forEach((attrName: string) => {
+      this.attrsToUpdate.add(attrName);
+    });
+
     const msgState = this.serialize(attrs);
 
     if (Object.keys(msgState).length > 0) {
@@ -444,7 +488,8 @@ export class WidgetModel extends Backbone.Model {
       } else {
         // We haven't exceeded the throttle, send the message like
         // normal.
-        this.send_sync_message(attrs, callbacks);
+        const msgId = this.send_sync_message(attrs, callbacks);
+        this.rememberLastUpdateFor(msgId);
         // Since the comm is a one-way communication, assume the message
         // arrived and was processed successfully.
         // Don't call options.success since we don't have a model back from
@@ -453,6 +498,12 @@ export class WidgetModel extends Backbone.Model {
       }
     }
   }
+  rememberLastUpdateFor(msgId: string) {
+    this.attrsToUpdate.forEach((attrName) => {
+      this.expectedEchoMsgIds.set(attrName, msgId);
+    });
+    this.attrsToUpdate = new Set<string>();
+  }
 
   /**
    * Serialize widget state.
@@ -488,9 +539,9 @@ export class WidgetModel extends Backbone.Model {
   /**
    * Send a sync message to the kernel.
    */
-  send_sync_message(state: JSONObject, callbacks: any = {}): void {
+  send_sync_message(state: JSONObject, callbacks: any = {}): string {
     if (!this.comm) {
-      return;
+      return '';
     }
     try {
       callbacks.iopub = callbacks.iopub || {};
@@ -504,7 +555,7 @@ export class WidgetModel extends Backbone.Model {
 
       // split out the binary buffers
       const split = utils.remove_buffers(state);
-      this.comm.send(
+      const msgId = this.comm.send(
         {
           method: 'update',
           state: split.state,
@@ -515,9 +566,11 @@ export class WidgetModel extends Backbone.Model {
         split.buffers
       );
       this._pending_msgs++;
+      return msgId;
     } catch (e) {
       console.error('Could not send widget sync message', e);
     }
+    return '';
   }
 
   /**
@@ -624,6 +677,12 @@ export class WidgetModel extends Backbone.Model {
   private _msg_buffer: any;
   private _msg_buffer_callbacks: any;
   private _pending_msgs: number;
+  // keep track of the msg id for each attr for updates we send out so
+  // that we can ignore old messages that we send in order to avoid
+  // 'drunken' sliders going back and forward
+  private expectedEchoMsgIds: Map<string, string>;
+  // because we don't know the attrs in _handle_status, we keep track of what we will send
+  private attrsToUpdate: Set<string>;
 }
 
 export class DOMWidgetModel extends WidgetModel {

[jasongrout]

Continuing conversation from #3392, here is where I've landed so far:

1. JUPYTER_WIDGETS_ECHO can turn off all echo messages
2. In ipywidgets 7.7, JUPYTER_WIDGETS_ECHO defaults to false (turning off all echo messages), in 8.0 it defaults to true (turning on echo message updates for any trait except those for which echo_updatemetadata is False).
3. When processing an attribute update from the frontend:
   A. Immediately send out an echo_update message that has the exact same format as an update message, reflecting updates for any attributes except those that have echo_update metadata set to False. If there are no updates to send, don't send the message.
   B. Leave all other 7.x logic as-is (i.e., use the property_lock logic, only send out updates if they are different than what we think the frontend has sent us in the current update, etc.)

(Notice that I changed the metadata attribute from no_echo to echo_update - the negative in the name was a bit confusing)

This leads to some more messages that the logic currently in master, but I think it is okay, and it does a couple of things for us:

1. Since echo messages are a new type of message, there is no question about backwards compatibility - older frontends just won't handle the messages. However, since the new message shares its format with normal update messages, the logic to process them is very simple to layer on top of existing logic.
2. There is quicker turnaround time for the reflections - they are sent immediately.
3. If a widget model knows that it will often transform an update from the frontend, it can just set that attribute to echo_update=False (for example, some of the tests always transform an update from the frontend - those clearly would be echo_update=False since we know that a kernel message will be going out updating the value). Another reason to set echo_update=False is if information naturally flows one-way from the frontend to the kernel, e.g., the file upload widget.

While this approach also reflects comm_open messages from the frontend when the frontend is creating a widget, there isn't currently logic to handle those state updates. Reflecting comm open messages likely should be handled at the comm layer, not the ipywidgets layer.

I've updated the messages.md for these changes: https://github.com/jupyter-widgets/ipywidgets/blob/11c36db9b2e030433b547c07a5949bfc7caed072/packages/schema/messages.md#synchronizing-multiple-frontends-echo_update

[jasongrout]

CC @vidartf @maartenbreddels @SylvainCorlay and others: I think this is ready for review.

[jasongrout]

Today in the dev meeting, @vidartf said he would try to review this by tomorrow, and that at a glance the code looked good.

[jasongrout]

To aid in reviewing, here is a commit that is the squash of #3195 and this PR, so you can see the overall effect of both PRs: 56df723

[jasongrout]

For a follow-up, I'd like to rename the two new attributes to conform to the convention that private variables start with underscore:

  private _expectedEchoMsgIds: Map<string, string>;
  // because we don't know the attrs in _handle_status, we keep track of what we will send
  private _attrsToUpdate: Set<string>;

[vidartf]

I've reviewed it, and I agree that this PR is as correct as I can make it out to be. My mental notes on backwards compatibility:

• If the kernel is new while the frontend is old, the kernel will keep the same behavior for "update" messages. As long as the frontend successfully discards the unknown "echo_update" message without failing (and our code does this), then everything will be as expected.
• If the kernel is old, while the frontend is new, the frontend will build up a finitely sized collections of attribute keys to message ids that it is looking for, but no performance hit or change in behavior.

As such, the main point is then about correct behavior, which is compatible with the RC phase. I do have some points of improvements related to that:

• Ideally in this PR, but can be done in follow up in RC phase: We should add one or more tests for the behavior when we set the env var to use / not use echos. Currently this is not covered by the tests?
• Follow up PR: We should add tests for the JS side of echos. Currently there are no tests checking this, and I think I could simplify the JS code for echo handling some more, but it is hard to be certain since non of the expected behavior is encoded in tests.
• Follow up PR: Can we add a note in the docstring of send_sync_message about what the expected return value is? E.g. "If a message is sent successfully, this returns the message ID of that message. Otherwise it returns an empty string".

[vidartf]

I'll leave it up to @jasongrout about whether he wants to add any of the tests in this PR or to just merge and we then do a follow up. If you do decide to add some tests, I do not think it needs another review unless the actual code changes as well.

[vidartf]

For exhaustiveness, this is what I believe will happen with the code in this PR as per the case highlighted originally in #3111 :

• If we enter 'sleep', an update event is send to the kernel, which takes 6 seconds to finish.
• Before it finishes, we change the text to "bug?", which is queued at the kernel, because the time.sleep(6) is still ongoing.
• Next, we send an update event from the kernel to the frontend, changing the Text value to "I computed for 6 seconds"
• After that we process the queued message from the frontend which changes the kernel side value to "bug?".
• NEW (as compared to before the echo feature was added): The echo message gets received by the client. It sees that it is it's own message echoed back, so it sets the value to that echo (overriding any intermediate state updates).

An important question is: Is this behavior different before and after this PR? I think maybe it will? I think the version of the code that is currently in master will never set the value in the frontend to "I computed for 6 seconds" ? So I guess this boils down to whether or not this is acceptable.

[vidartf]

An important question is: Is this behavior different before and after this PR? I think maybe it will? I think the version of the code that is currently in master will never set the value in the frontend to "I computed for 6 seconds" ?

Just tested locally, and yes, the behavior is different on master compared to this PR.

[jasongrout]

I'll leave it up to @jasongrout about whether he wants to add any of the tests in this PR or to just merge and we then do a follow up. If you do decide to add some tests, I do not think it needs another review unless the actual code changes as well.

I'll merge this PR and open a follow-up one forward-porting the tests I added to 7.x checking the environment variable switch.

[jasongrout]

Just tested locally, and yes, the behavior is different on master compared to this PR.

To elaborate on the behavior change here, this is because master currently consolidates echo messages with any updates from the kernel in response to the update. This PR separates out those messages.

In the example Vidar posted, here's what I think is happening on master:

1. The kernel receives the sleep for the value of the text box.
2. The kernel changes the value to I computed for 6 seconds
3. Simultaneously, the frontend queues up an update for the value to bug? (but not sent, since we are throttling updates, waiting for the last update processing to finish)
4. The kernel sends an update message back to the frontend, with the value I computed for 6 seconds, but also noting that this is an echo of the value.
5. Since the frontend has a pending message, it ignores this update because the kernel said it was an echo, even though the kernel is actually updating to a different value than what we sent.
6. Eventually, the echo from the bug? comes back to the frontend, and the frontend is reset to bug? (i.e., no change).

With this PR, step 4 is actually 2 steps. First an echo message is sent back with the value of sleep (which is ignored by the frontend, just like in step 5 above). However, then I computed for 6 seconds is sent back to the frontend as a normal update, not an echo, which is why the frontend applies it even though it has a pending message.

[jasongrout]

To elaborate on the behavior change here, this is because master currently consolidates echo messages with any updates from the kernel in response to the update. This PR separates out those messages.

On the flip side of this, on master, other clients will not see the frontend echo update until after 6 seconds have passed, whereas with this PR, other clients will immediately see the frontend update echo, then 6 seconds later see the update from the kernel.

[jasongrout]

Based on Vidar's review, I'll merge this. @maartenbreddels - if you would rather reconsider from this behavior change (consolidating updates with echos, vs sending echos out immediately and following up with other updates), please comment.