-
Notifications
You must be signed in to change notification settings - Fork 950
fix: _pending_msgs was negative, iopub.status was called too often #3470
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
fix: _pending_msgs was negative, iopub.status was called too often #3470
Conversation
This broke message throttling. Fixes jupyter-widgets#3469
d3fe0a4
to
97025e2
Compare
As I suggested in #3469 the following works much better: import time
import ipywidgets as w
@w.interact
def test(f = 10.1):
print(f**2)
time.sleep(1) Before this would take many seconds to finish (there is a long queue of messages that will trigger the test function), while with this PR it is only max 2 seconds (the one in flight, and the 1 pending). Also, websocket messages behave as predicted/expected now: 1 update to the kernel, followed by busy/echo/idle (for 7.x that should just be busy/idle). |
@meeseeksdev please backport to 7.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
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 If these instructions are inaccurate, feel free to suggest an improvement. |
See the discussion starting at jupyter-widgets#3469 (comment) This partially reverts changes from jupyter-widgets#3470.
This partially reverts changes from jupyter-widgets#3470. I think the underlying assumptions around send_sync_message and callbacks are: 1. send_sync_message is only ever called once per message. When the original sync happens, either we put the message on the queue or we send it immediately, and the callback modifications happen only when we actually send the message. 2. There was an implicit assumption in the send_sync_message code is that the callbacks object passed in is a new object unique to this message, i.e., we aren't modifying a global object, that then we'll modify again later. I think assumption 2 is sometimes not true, so this guards against it by making a (2-deep) copy of the callbacks object before modifying and using it.
This partially reverts changes from jupyter-widgets#3470. I think the underlying assumptions around send_sync_message and callbacks are: 1. send_sync_message is only ever called once per message. When the original sync happens, either we put the message on the queue or we send it immediately, and the callback modifications happen only when we actually send the message. 2. There was an implicit assumption in the send_sync_message code is that the callbacks object passed in is a new object unique to this message, i.e., we aren't modifying a global object, that then we'll modify again later. I think assumption 2 is sometimes not true, so this guards against it by making a (2-deep) copy of the callbacks object before modifying and using it.
…pub.status was called too often This backports commit 97025e2 from master to the 7.x branch.
Backported in #3494 |
This broke message throttling.
Fixes #3469
I think this should be backported to 7.x, since behaviour now is really badly defined. For views the throttling works the first time only (because the iopub callbacks gets reused when using .touch), for models (calling save_changes), we should not see this issue.