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

Ending notebook execution early with sys.exit without raising an exception #449

Merged
merged 4 commits into from
Dec 9, 2019

Conversation

yusuphisms
Copy link
Contributor

Hi!

I'm a newbie around these parts (and contributing to open source in general!) but thought I might make an attempt to resolve #435 . Noticed that without arguments to exit() or with None, evalue returns an empty string; that should also be evaluated the same as 0 I imagine? Any and all feedback is welcome 🙏🏾

Thank you!
Yusuph

@MSeal
Copy link
Member

MSeal commented Dec 3, 2019

The change looks good, we just need a couple unittests to cover the behavior. Probably one unittest each with an exit(), exit(0), and finally an exit(1) added to test_execute.py. Don't worry about the build failing in travis / azure, there's an issue with azure blob dependency causing it to fail atm.

@yusuphisms
Copy link
Contributor Author

Thanks for the review! Added tests -- tried to follow convention but let me know if I need to make any changes.

Also, this is strange but when I was running my tests, I occasionally got this in the output: Timeout waiting for IOPub output . It would cause that test to fail. When that did not happen, the tests would pass. Not sure if that's related to the new tests or not.

Copy link
Member

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

LGTM -- the test failure you saw is a known issue in ipython versions for python 2. Either your dependencies were old or you were testing in python 2, which we're going to drop direct support for at the end of the year.

@MSeal MSeal merged commit 0ee0da7 into nteract:master Dec 9, 2019
@MSeal MSeal added this to the Papermill 2.0 milestone Dec 30, 2019
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.

a good way to early stop the script
2 participants