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

raise PapermillExecutionError when CellExecutionError is raised without cell error output #786

Merged
merged 5 commits into from
Mar 28, 2024

Conversation

mbektas
Copy link
Contributor

@mbektas mbektas commented Mar 13, 2024

When notebooks with cell / line magics are executed and if the magic command fails, papermill doesn't throw any exception and returns exit code 0. This is because papermill relies on cell error output to raise PapermillExecutionError.

When magic commands fail, CellExecutionError is still thrown and recorded as cell metadata, and notebook execution stops. However, since these errors don't produce any cell error output, no exception is raised.

Changes in this PR check cell metadata for exceptions recorded by papermill (due to CellExecutionError) in addition to cell error outputs and raise PapermillExecutionError in those cases.

fixes #501, #381

@ibdafna
Copy link

ibdafna commented Mar 27, 2024

@MSeal apologies for the direct ping. Let us know if there's anything we can do to get this one over the line. Many thanks

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.

There's a subtle case where non-blocking exceptions are raised are intended to not fail cell execution, but it's a much smaller edge case than the exception with no output this PR is fixing (plus I believe other interfaces are leaning into the same tradeoff). Thanks for putting this together!

@mbektas
Copy link
Contributor Author

mbektas commented Mar 28, 2024

thanks for the review @MSeal! I ran unit tests locally and they were passing. I will look into failing CI checks.

@MSeal
Copy link
Member

MSeal commented Mar 28, 2024

@mbektas No problem. I should be checking these more regularly.

The current test failure is unrelated to this PR. If you have a few minutes to add a test for the new exception detection it'd be appreciated. Independently I will go fix the unrelated test if you don't patch it yourself and get this merged when I get back from this work trip.

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

Merging #786 (5d69044) into main (cb2eb37) will decrease coverage by 0.26%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #786      +/-   ##
==========================================
- Coverage   91.54%   91.29%   -0.26%     
==========================================
  Files          17       17              
  Lines        1621     1631      +10     
==========================================
+ Hits         1484     1489       +5     
- Misses        137      142       +5     

@mbektas
Copy link
Contributor Author

mbektas commented Mar 28, 2024

The current test failure is unrelated to this PR.

I just fixed the failing test, it was due to a deprecated pytest feature. I will also be adding test(s) for the magic command error handling.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@mbektas
Copy link
Contributor Author

mbektas commented Mar 28, 2024

@MSeal I added a unit test for magic command failures as well. Looks like workflow needs your approval to run again.

@MSeal MSeal merged commit a2f4f2d into nteract:main Mar 28, 2024
12 of 13 checks passed
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.

No error message on magic command error
3 participants