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

Traceback in cell execution error #521

Merged
merged 3 commits into from Feb 15, 2017

Conversation

Projects
None yet
5 participants
@mupdt
Copy link
Contributor

mupdt commented Jan 27, 2017

We're executing Notebooks during CI to make sure the content is always up-to-date.

However, if the execution fails, we get the following error message fro nbconvert:

[NbConvertApp] Converting notebook /u/matej/Test.ipynb to notebook
[NbConvertApp] Executing notebook with kernel: python2
[NbConvertApp] ERROR | Error while converting '/u/matej/Test.ipynb'
Traceback (most recent call last):
...UNINFORMATIVE INTERNAL STACK TRACE...
File "/u/matej/git/open-source/nbconvert/nbconvert/preprocessors/execute.py", line 113, in preprocess_cell
raise CellExecutionError(msg)
CellExecutionError
[1] 110973 exit 1 jupyter nbconvert --to notebook --stdout --execute ~/Test.ipynb

The message only says CellExecutionError, which makes it hard to pinpoint the cell and code where things went wrong.

This patch adds a traceback to the error message. It now looks like this:

[NbConvertApp] Converting notebook /u/matej/Test.ipynb to notebook
[NbConvertApp] Executing notebook with kernel: python2
[NbConvertApp] ERROR | Error while converting '/u/matej/Test.ipynb'
Traceback (most recent call last):
...UNINFORMATIVE INTERNAL STACK TRACE...
File "/u/matej/git/open-source/nbconvert/nbconvert/preprocessors/execute.py", line 113, in preprocess_cell
raise CellExecutionError(msg)
CellExecutionError: An error occurred while executing the following cell:
------------------
import dakjdnsak
------------------
ImportError: No module named dakjdnsak
[1] 111347 exit 1 jupyter nbconvert --to notebook --stdout --execute ~/Test.ipynb
@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 28, 2017

This looks like useful functionality and a straightforward change.

I'd prefer to see a test to make sure that we never break this functionality once it's included.

I could imagine testing against the content of a traceback might be an issue… but it seems like that would be the appropriate approach for testing this feature.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 28, 2017

I think it should be sufficient to test that the error string is shown. I.e. have a cell:

raise Exception("Can you see me?")

Then run that in nbconvert and assert that the exception shows "Can you see me?".

@Carreau

Carreau approved these changes Feb 1, 2017


def __init__(self, message):
super(ConversionException, self).__init__(message)

This comment has been minimized.

@Carreau

Carreau Feb 1, 2017

Contributor

Is that necessary ? Wouldn't message be passed to Exception if there is no init ?

This comment has been minimized.

@mupdt

mupdt Feb 8, 2017

Author Contributor

Oh, yes, that's right. Will remove.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 14, 2017

@mupdt982 it looks like we're waiting on a couple of changes here (test, remove unnecessary __init__). Just wanted to check you haven't forgotten about it. :-)

@mupdt

This comment has been minimized.

Copy link
Contributor Author

mupdt commented Feb 14, 2017

@takluyver: I've got a fix now, will push soon.

Adds test for CellExecutionExceptin message.
Removes __init__ from empty exception constructor.

Also adds tests for unicode in message exceptions.
@mupdt

This comment has been minimized.

Copy link
Contributor Author

mupdt commented Feb 15, 2017

Please take another look.

  • removed the explicit super constructor call,
  • added tests (I reused existing notebooks for the tests; I also added a test that verifies utf-8 is handled correctly).
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 15, 2017

Thanks, that looks good to me.

@takluyver takluyver added this to the 5.2 milestone Feb 15, 2017

@takluyver takluyver merged commit 59bd150 into jupyter:master Feb 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mpacer mpacer added to_changelog and removed to_changelog labels May 24, 2017

@MattFaus

This comment has been minimized.

Copy link

MattFaus commented on 6ff7e8d Oct 5, 2018

thank you for this, it resolves the problem discussed here: https://bugs.python.org/issue9400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment