Extend PapermillExecutionError to carry the executed output notebook object#873
Conversation
…otebook object Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
…otebook object Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
…otebook object Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends PapermillExecutionError so callers can access the in-memory executed notebook object directly from the exception, avoiding a write/read round-trip when execution fails.
Changes:
- Add an
output_notebookattribute (optional) toPapermillExecutionError. - Populate
output_notebookwhen raisingPapermillExecutionErrorfrom execution error detection. - Add a unit test asserting
PapermillExecutionError.output_notebookis anbformat.NotebookNode.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
papermill/exceptions.py |
Extends PapermillExecutionError to carry output_notebook. |
papermill/execute.py |
Passes the executed notebook into PapermillExecutionError when an error output is detected. |
papermill/tests/test_execute.py |
Adds a regression test validating the exception exposes an output notebook object. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ename=output.ename, | ||
| evalue=output.evalue, | ||
| traceback=output.traceback, | ||
| output_notebook=nb, | ||
| ) |
There was a problem hiding this comment.
Only the error-output branch passes output_notebook=nb. The fallback branch for CellExecutionError (the papermill.exception metadata path) still constructs PapermillExecutionError without output_notebook, so callers will sometimes see None for the new attribute. Pass the current notebook there as well to make the new API consistent.
| def __init__(self, cell_index, exec_count, source, ename, evalue, traceback, output_notebook=None): | ||
| args = cell_index, exec_count, source, ename, evalue, traceback | ||
| self.cell_index = cell_index | ||
| self.exec_count = exec_count | ||
| self.source = source | ||
| self.ename = ename | ||
| self.evalue = evalue | ||
| self.traceback = traceback | ||
| self.output_notebook = output_notebook | ||
|
|
||
| super().__init__(*args) |
There was a problem hiding this comment.
PapermillExecutionError is explicitly tested as pickle/unpickleable (see papermill/tests/test_exceptions.py). Because output_notebook isn’t included in args (and there’s no custom __reduce__), pickling an error that has output_notebook set will silently drop the notebook on unpickle. Either include output_notebook in the pickled state (e.g., via args or __reduce__) or document/ensure it’s intentionally not preserved.
| ename=output.ename, | ||
| evalue=output.evalue, | ||
| traceback=output.traceback, | ||
| output_notebook=nb, | ||
| ) |
There was a problem hiding this comment.
output_notebook is being set to nb before raise_for_execution_errors upgrades/inserts the error marker cells. Since nbformat.v4.upgrade(nb) can return a new notebook object, err.output_notebook may not reflect the final notebook that gets written. Consider assigning error.output_notebook (or constructing the exception) after the upgrade/marker insert so it matches the on-disk output notebook state.
Summary
PapermillExecutionErrorcurrently exposes execution metadata (cell index, source, traceback details) but not the executed notebook object itself. This issue’s core requirement is to make the in-memory output notebook available directly from the exception so callers can inspect outputs (e.g., scrapbook data) without round-tripping through storage.Files changed
papermill/exceptions.py(modified)papermill/execute.py(modified)papermill/tests/test_execute.py(modified)Testing
What does this PR do?
Fixes #<issue_number>
Closes #795