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

Re-introduce #539, fix abort to not abort non-execute requests #572

Merged
merged 6 commits into from
Dec 15, 2020

Conversation

glentakahashi
Copy link
Contributor

@glentakahashi glentakahashi commented Dec 14, 2020

Fixes #571

As described in the original issue (#539), the old stop_on_error_timeout functionality didn't actually work asynchronously, and would cause new requests to block until the error was finished. The PR #539 fixed this to work async without changing any of the other logic.

This caused occasional issues in other libraries that depend on the kernel_info request, when the notebook had cells with python errors in them due to the race condition of sending off the request.

This new PR re-introduces the async stop_on_error_timeout fix, but changes the logic to abort /only/ execute_request's. This means that running multiple cells in a row where the first cell has an error will still cause the remaining cell executions to abort, but you can still continue to do things like kernel_info, inspect, completions, etc.

It also adds a couple of tests for those.

Also tested against papermill in nteract/papermill#519 to verify that the fix allows papermill to still function.

@glentakahashi
Copy link
Contributor Author

Hmm, i'm pretty sure the failing test is a flake, but I don't know how to re-trigger a step in github actions. Do I just have to push up an empty no-op commit or do I lack permissions?

@blink1073
Copy link
Member

Thanks for adding the test and for testing against papermill! I kicked CI. Closing and re-opening the PR works I think, otherwise you can rebase and force push to force a rebuild.

Copy link
Member

@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! A different CI build failed this time, so it looks like flakiness.

@blink1073 blink1073 merged commit a19319e into ipython:master Dec 15, 2020
@glentakahashi glentakahashi deleted the gt/reintroduce-539 branch December 15, 2020 17:33
@Carreau Carreau added this to the 5.5 milestone Jun 14, 2021
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 this pull request may close these issues.

Re-introduce #539 after understanding language_info KeyError
3 participants