search

nteract / papermill

📚 Parameterize, execute, and analyze notebooks
http://papermill.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
6k stars 429 forks source link

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

Closed mbektas closed 8 months ago

mbektas commented 8 months ago

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 https://github.com/nteract/papermill/issues/501, https://github.com/nteract/papermill/issues/381

ibdafna commented 8 months ago

@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

mbektas commented 8 months ago

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

MSeal commented 8 months ago

@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.

codecov[bot] commented 8 months ago

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 ```diff @@ 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 commented 8 months ago

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.

review-notebook-app[bot] commented 8 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

mbektas commented 8 months ago

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