Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement update echo messages in a backward-compatible way #3392

Closed
jasongrout opened this issue Feb 19, 2022 · 11 comments · Fixed by #3394
Closed

Implement update echo messages in a backward-compatible way #3392

jasongrout opened this issue Feb 19, 2022 · 11 comments · Fixed by #3394
Milestone

Comments

@jasongrout
Copy link
Member

I think we can implement the update echo feature from #3195 and further discussed in #3343 in a backward-compatible way.

To summarize, when the kernel receives an update message, it will apply the message and then always broadcast out a message to clients, where some of the attribute updates in the broadcast message may be designated as echoed updates. Clients ignore echoed updates until their own update is echoed back to them. They should ignore because it lets them optimistically update their own UX. Essentially, seeing the echo of their own update lets them know the order in which the kernel is applying updates from multiple clients, and ignore those echoed updates if it knows that the kernel still has to apply its own update.

If a client does not ignore echo updates until it sees its own update echoed, and if the client is optimistically updating its own UX to reflect its update, we will have jitter. For example, imagine two clients displaying the same slider model, and both sliders are being dragged, and slider A sends a message updating the value to 5 while slider B sends a message updating the value to 10. Each slider is updated to its own update value, but we don't have consensus on what the kernel is going to pick. The kernel processes these messages in order, and sends out slider A's update as an echo, then slider B's update as an echo. Slider A receives its own echoed update, then B's echoed update, and applies B's update since it came after its own echo. However, slider B receives A's echoed update and ignores it, since it knows the kernel has yet to apply its own update. Then slider B receives its own echoed update, which it can ignore since it has already updated the UX optimistically for that value. The end result is that both clients and the kernel have agreed that B's update wins.

We said this protocol wasn't backwards compatible since if B doesn't know to ignore comm echo updates, it will jitter since it will apply A's update, then apply its own echo update. The real problem, though, is that the echo updates and normal updates are intermingled in the single key in the update message, so a client that doesn't know about comm echo updates cannot distinguish between the two types of updates.

However, what if we send echoed state updates in their own echo_state key, rather than conflating it with the normal state key for normal updates from the kernel? Then I think things are backwards compatible - clients that don't know about the echoed updates will ignore any echoed updates, so there is no change in their behavior. Clients that understand the echoed update messages can implement this consensus protocol from above. I think in this case, we can implement this logic in 7.7, to give it to people that won't be able to update to 8.0 right away.

Disclosure: At Databricks, my current employer, we'd like to get this feature in 7.7 if we can, which led me to reevaluating the problem to see if there was a way we could do it in a backwards compatible way.

@jasongrout jasongrout added this to the 8.0 milestone Feb 19, 2022
@jasongrout
Copy link
Member Author

In other words, I'm suggesting that the protocol be amended this way:

Synchronizing multiple frontends: update with echo

Starting with protocol version 2.1.0 the kernel can send a special update message back, to allow all connected frontends to be in sync with the kernel state. This allows multiple frontends to be connected to a single kernel but also resolves a possible out of sync situation when the kernel and a frontend send out an update message at the same time, causing both to think they have the latest state.

In protocol version 2.1.0 the kernel is considered the single source of truth and is expected to send back to the frontends an update message that contains an extra state update dictionary indicating which state updates are just reflections of frontend state updates.

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

The state dictionary contains updates directly from the kernel, while echo_state contains updates from other frontends. If the same attribute is in both dictionaries, the state update takes precedence. state must be present, but may be empty, and echo_state is optional.

In situations where a user does many changes to a widget on the frontend (e.g. moving a slider), the frontend will receive from the kernel many update messages (with the echo_state key containing echoed updates) from the kernel that can be considered old values. A frontend can choose to ignore all updates in echo_state that are not originating from the last update it send to the kernel. This can be implemented by keeping track of the msg_id for each attribute for which we send out an update message to the kernel, and ignoring all echo_state updates as a result from an echo for which the msg_id of the parent header is not equal to msg_id we kept track of.

For situations where sending back an echo update for a property is considered to expensive, we have implemented an opt-out mechanism in ipywidgets. A trait can have a no_echo metadata attribute to flag that the kernel should not send back an update to the frontends. We suggest other implementations implement a similar opt-out mechanism.

@jasongrout
Copy link
Member Author

CC @tkrabel-db

@jasongrout
Copy link
Member Author

CC @maartenbreddels

@vidartf
Copy link
Member

vidartf commented Feb 22, 2022

Discussion points from today's dev meeting:

  • We should pay careful attention to whether or not the switch away from the property lock will still make this backwards incompatible.
  • We should try to capture all corner cases in tests (even if that means disabled / expected failure tests for "wishlist" behavior) so that we can have a clear list of what behaviors are changing / problematic.

@maartenbreddels
Copy link
Member

Sounds solid, I'm in favour of putting this in 7.7, possibly off by default?

@vidartf
Copy link
Member

vidartf commented Feb 22, 2022

We should try to capture all corner cases in tests

@maartenbreddels already did some good work on this, much appreciated.

@jasongrout
Copy link
Member Author

possibly off by default?

Yes, I think we should default off in 7.7, and probably default on for 8.0.

@jasongrout
Copy link
Member Author

jasongrout commented Feb 23, 2022

I'm working through a PR on this, and I think we can consolidate the property_lock logic a bit too. Here is my current working draft of the logic:

  1. JUPYTER_WIDGETS_ECHO being false is equivalent to no_echo on every attribute.
  2. In ipywidgets 7.7, JUPYTER_WIDGETS_ECHO defaults to false, in 8.0 it defaults to true.
  3. When processing an attribute update from the frontend, first store all the attribute values the frontend sent us.
    A. if the attribute is no_echo, we compare the current attribute with what the frontend sent us. If the current value is different, we have updated the attribute value, so send the update back to the frontend in state. If the current value is the same, don't send anything back. This is essentially what the property_lock stuff is doing in widget.py
    B. If the attribute is not no_echo (i.e., echos), then we always send back the value of the attribute after processing the update. If the value has not changed from what the kernel sent us, we send back its value in echo_state. If the value has changed from what the kernel sent us, we send back the value in state, but we also send a None value in the echo_state so that the frontend knows this is an echo, even though the value has changed. If an attribute is in both state and echo_state, the state value takes precedence.

@jasongrout
Copy link
Member Author

jasongrout commented Feb 23, 2022

I think I am way overthinking this. Here's a much simpler design:

  1. JUPYTER_WIDGETS_ECHO being false is equivalent to no_echo on every attribute.
  2. In ipywidgets 7.7, JUPYTER_WIDGETS_ECHO defaults to false, in 8.0 it defaults to true.
  3. When processing an attribute update from the frontend:
    A. Immediately send out an echo update message with state as {} and echo_state as the value of every attribute in the update that is not no_echo.
    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.)

jasongrout added a commit to jasongrout/ipywidgets that referenced this issue Feb 23, 2022
@jasongrout
Copy link
Member Author

One thing I like about the simpler design is that it preserves the order of updates: you first get the update the kernel is applying, and then you get any updates in response from the kernel. If we were just sending the echo updates after the kernel processed the update, we would get the kernel updates first, then other attribute echos.

jasongrout added a commit to jasongrout/ipywidgets that referenced this issue Feb 23, 2022
jasongrout added a commit to jasongrout/ipywidgets that referenced this issue Feb 23, 2022
jasongrout added a commit to jasongrout/ipywidgets that referenced this issue Feb 23, 2022
jasongrout added a commit to jasongrout/ipywidgets that referenced this issue Feb 23, 2022
@tkrabel-db
Copy link

For what it's worth, I find the simpler design very elegant :)

jasongrout pushed a commit to jasongrout/ipywidgets that referenced this issue Mar 2, 2022
As described in jupyter-widgets#3111 it can happen that a frontend can get out of sync
due to not echoing back messages from the frontend.

Widgets can opt out of this behaviour if it is too costly and/or does
not make sense, e.g. the data from the file widget.

Fixes jupyter-widgets#3111

avoid echo/jitter in frontend

put jupyter echo behaviour behind a flag

minimize diff

remove spacing

prettier

lint

fix: handle messages correctly when echoing disabled

minimize diff

minimize diff

fix and test

fix test, do not use set (no determinstic order)

fix: echos from other clients are not unexpected

comment and reformat

better variable name

Work-in-progress update for echo updates to have their own key in update messages

Update protocol description

Update js echo logic to handle echo_state key

Starts fixing jupyter-widgets#3392

WIP consolidating logic for property lock.

WIP simple reflection immediately on setting state.

WIP Remove code about sending echo messages combined with any kernel updates

Remove comments and simplify so the final diff is simpler

Add debug test runner

Experiment making echo updates a separate message

Bump widget protocol version number back to 2.1.0

Update no_echo metadata name to echo_update for clarity.

Having a negative in the name was confusing to me, so I renamed the property echo_update. Set it to False to disable echos for that attribute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants