-
-
Notifications
You must be signed in to change notification settings - Fork 372
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
Changed default timeout to 0.0 seconds for stop_on_error_timeout #618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense on the short circuit. Seems fine to be based on our conversation! I'm not a real collaborator though, so will need a real review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@meeseeksdev please backport to 5.5.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. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
Changed default timeout to 0.0 seconds for stop_on_error_timeout
Back port of pull request #618 (Changed default timeout to 0.0 seconds for stop_on_error_timeout)
Resolves #609
Tested on classic, lab, papermill, testbook, and nteract. The behavior matches 5.4 kernel behavior by default now, with the ability for each interface to set the default abort time if they wish. Testing this with a higher value override worked as expected with requests aborting within the given window of time, though none of the interfaces showed that an abort occurred, they simple show no result. Also
stop_on_error: false
works to ignore the setting (nteract has this on by default for some reason).@glentakahashi I implemented the suggested change, but when testing if you short circuit the abort future, the run-all in classic and lab will no properly abort already queued cells that were sent with
Run All Cells
. It does work appropriately for papermill/nbclient/testbook since those interfaces wait for each cell completion to send the next cell to execute. Unlike the headless libraries, the browser notebook interfaces will send all the cells at once (in back-to-backexecute_request
s) when told toRun All
, relying on the kernel queue to handle execution. If the abort isn't fired with an immediate delay the queue will not be dropped.The reason for headed interfaces to do this is to partially protect users from not executing those cells if they lose network connection to the server. The ideal solution would be for the notebook server to do the queuing so that A) latency between kernel and request is minimized and B) the server can control continued execution when clients disconnect rather than relying on the kernel to know caller intent. But that's beyond the scope of the change here.