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

Solves hanging indefinitely when kernel dies #959

Merged
merged 3 commits into from Mar 23, 2019

Conversation

@dwjbosman
Copy link
Contributor

dwjbosman commented Feb 17, 2019

I use Jupyter notebooks for automatic regression testing. I use nbconvert to execute notebooks containing tests:

jupyter nbconvert ..... --allow-errors .... --ExecutePreprocessor.timeout=-1 .... --execute test.ipynb

In one of the tests the kernel dies. Unfortunately nbconvert will hang on this. I can't use timeouts as some cells in the test notebooks take a long time to execute. If the kernel dies I want nbconvert to detect this almost immediately.

The patch checks that the kernel is alive while waiting for a message.

dwjbosman added 2 commits Feb 17, 2019
@dwjbosman dwjbosman changed the title Dwjbosman patch 1 Solves hanging indefinitely when kernel dies Feb 17, 2019
@dwjbosman

This comment has been minimized.

Copy link
Contributor Author

dwjbosman commented Feb 17, 2019

Travis CI reports a test failed on Python 3.5. From the logs I can't see how my modifications could have impacted this test.

@MSeal

This comment has been minimized.

Copy link
Collaborator

MSeal commented Feb 23, 2019

Thanks for submitting this -- I was planning on tackling it soon as well (see nteract/papermill#125). I've started seeing this error message occasionally during tests with papermill and nbconvert and will take a look if I can figure out why. I'll do a more thorough testing for this PR before merging as it's really critical that this operate correctly for papermill.

@MSeal

This comment has been minimized.

Copy link
Collaborator

MSeal commented Mar 2, 2019

I dug through all of the client code and this reads as the correct fix for the issue.

I will say though that given how critical this piece section of code is for the library it should have a unittest for each branch possible here. This area may be undertested today as well. If you can add unittests for this pattern I'm willing to go dig deeper in to the unrelated build failure and trace back what's causing that.

I will be out of town for a bit, so might be slow on responding to this thread. But when I get back I'll want to get this and a few other changes merged for a new release.

@MSeal

This comment has been minimized.

Copy link
Collaborator

MSeal commented Mar 23, 2019

Tested this out locally and will add tests in a separate PR given how important this is to get added.

@MSeal MSeal merged commit 6cbc541 into jupyter:master Mar 23, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@MSeal MSeal added this to the 5.5 milestone Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.