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

Changed default timeout to 0.0 seconds for stop_on_error_timeout #618

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

MSeal
Copy link
Contributor

@MSeal MSeal commented Mar 29, 2021

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-back execute_requests) when told to Run 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.

Copy link
Contributor

@glentakahashi glentakahashi left a 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

@blink1073 blink1073 added this to the 5.5 milestone Mar 29, 2021
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit 677bea4 into ipython:master Mar 29, 2021
@blink1073
Copy link
Contributor

@meeseeksdev please backport to 5.5.x

@lumberbot-app
Copy link

lumberbot-app bot commented Mar 29, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout 5.5.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 677bea4ae88074a594cb9b4482a437199c32df8b
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #618: Changed default timeout to 0.0 seconds for stop_on_error_timeout'
  1. Push to a named branch :
git push YOURFORK 5.5.x:auto-backport-of-pr-618-on-5.5.x
  1. Create a PR against branch 5.5.x, I would have named this PR:

"Backport PR #618 on branch 5.5.x"

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.

blink1073 added a commit that referenced this pull request Mar 29, 2021
Changed default timeout to 0.0 seconds for stop_on_error_timeout
blink1073 added a commit that referenced this pull request Mar 29, 2021
Back port of pull request #618 (Changed default timeout to 0.0 seconds for stop_on_error_timeout)
@blink1073
Copy link
Contributor

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.

Execute Requests Immediately Following and Error are Aborted in 5.5
3 participants