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

Kernel is the source of truth: support multiple clients and fixes out of sync bug #3195

Merged
merged 15 commits into from
Dec 22, 2021

Conversation

maartenbreddels
Copy link
Member

As described in #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 #3111

@jasongrout
Copy link
Member

In our dev meeting today, we were discussing this, and whether it should be a target for 8.0, or whether it should be tackled in the (hopefully upcoming) explorations in moving the syncing to a real-time collaboration backend.

@jasongrout
Copy link
Member

jasongrout commented Jun 1, 2021

In the dev meeting today, we propose moving this to 9.0 with the (hopefully upcoming) move to an RTC framework for syncing data. Maarten, what do you think?

@jasongrout jasongrout modified the milestones: 8.0, Future Jul 6, 2021
@jasongrout
Copy link
Member

Moving to Future (e.g., 9.0) since there was no follow-up. Maarten, if you'd like to discuss this more, please comment. We can always move it back if that's what everyone decides.

@maartenbreddels
Copy link
Member Author

I think that makes sense!

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
@SylvainCorlay
Copy link
Member

This poses an issue in the following situation:

  • A client moves continuously a slider from left to right
  • It gets the echos back from past positions of the handles, making the handle jump to lesser values of the slider.

We have seen this bug make slider handles jitter.

Copy link
Member

@SylvainCorlay SylvainCorlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my last comment.

@maartenbreddels
Copy link
Member Author

We can see this from the parent header, if we get an update from the kernel, where the parent header msg_id equals the last msg_id of the update we send to the kernel, (and our value has changed, but that is something backbone should figure out), we should update our back model model. If the msg_id is not equal to our last update message (on a per property/trait basis), it's an old message, and we can ignore it.

@maartenbreddels
Copy link
Member Author

on the client that is changing the slider, there is not jitter at all, actually I also fixed an old but I think that caused more jitter that needed since we set the state of a property, even though it was already planned for sending to the kernel (attr present in _msg_buffer)
widgets-magic4

@maartenbreddels
Copy link
Member Author

with the env var JUPYTER_WIDGETS_ECHO=0 the echo can be turned off, of simply from the Python module.

Copy link
Member

@jasongrout jasongrout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This protocol change should be documented in https://github.com/jupyter-widgets/ipywidgets/blob/master/packages/schema/messages.md#widget-messaging-protocol-version-2. Since it is backwards compatible, perhaps we bump the protocol version to 2.1?

@maartenbreddels
Copy link
Member Author

Note from @jasongrout:
If we get an update from the frontend, but that trait is marked 'no_echo', we do send it back to the frontend in the 'echo' list, but it does not get the updated value. The frontend code currently handles this, and it's informative, but we should maybe explicitly state this.

@SylvainCorlay
Copy link
Member

@maartenbreddels @jasongrout @ibdafna was there a final decision to go ahead and merge after I left the call?

Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested running of the code, but AFAICT this code now meets all the targets we said we needed for including this in the RC

@vidartf
Copy link
Member

vidartf commented Dec 22, 2021

On things that were not a prerequisite for inclusion in the RC, but I would consider a prerequisite for release the final version with this on as the default: I think that we should make sure we have tests that properly cover the cases where the previous property lock prevented flickering of values to occur. I'm uncertain if the new code properly handles all of the corner cases there, and more regression tests would go a long way in acting as a proof of that.

@ibdafna
Copy link
Member

ibdafna commented Dec 22, 2021

@SylvainCorlay I believe we agreed to merge, release a new beta tomorrow, and for @maartenbreddels to add documentation about the changes this PR introduces, before releasing a new RC in the new year.

@SylvainCorlay
Copy link
Member

On things that were not a prerequisite for inclusion in the RC, but I would consider a prerequisite for release the final version with this on as the default: I think that we should make sure we have tests that properly cover the cases where the previous property lock prevented flickering of values to occur. I'm uncertain if the new code properly handles all of the corner cases there, and more regression tests would go a long way in acting as a proof of that

Yes, we spent an hour this morning trying to break it, both by inspecting the code and trying different scenarios.

@SylvainCorlay SylvainCorlay merged commit f7c48af into jupyter-widgets:master Dec 22, 2021
@SylvainCorlay
Copy link
Member

Merged!

@jasongrout
Copy link
Member

I thought we had decided to document this in the widget protocol (i.e., #3195 (review)), then merge. I do think it is important to document this before releasing a release with it, since it is big enough change (especially with default on).

@jasongrout
Copy link
Member

I've opened #3330 to track the documentation and protocol version changes.

@ibdafna
Copy link
Member

ibdafna commented Dec 22, 2021

@jasongrout apologies for the mixup. @maartenbreddels will wait for a PR with documentation changes before releasing

@jasongrout
Copy link
Member

jasongrout commented Dec 22, 2021

No problem. We could also just cut the beta from just before the merge commit (i.e., branch off, do the beta release from the branch and tag it there, then merge back into master).

@ibdafna
Copy link
Member

ibdafna commented Dec 22, 2021

@jasongrout will do that!

@ibdafna ibdafna mentioned this pull request Dec 23, 2021
@vidartf vidartf mentioned this pull request Feb 28, 2022
6 tasks
jasongrout added a commit to jasongrout/ipywidgets that referenced this pull request Mar 2, 2022
jasongrout added a commit to jasongrout/ipywidgets that referenced this pull request Mar 2, 2022
jasongrout added a commit to jasongrout/ipywidgets that referenced this pull request Mar 4, 2022
This copies the test_set_state file from before jupyter-widgets#3195
to make sure that it still works unchanged when disabling
echo_update messages.
@jasongrout jasongrout mentioned this pull request Mar 4, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Widget out of sync: fundamental issue?
6 participants