-
Notifications
You must be signed in to change notification settings - Fork 571
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
Execute preprocess cells #1380
Execute preprocess cells #1380
Changes from all commits
55b24b0
383117d
ed5b8f8
bd3ce89
f5fb58b
da725bb
fcf6f44
c3de1a2
57c716c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,10 @@ | |
|
||
# Copyright (c) IPython Development Team. | ||
# Distributed under the terms of the Modified BSD License. | ||
from typing import Optional | ||
from nbformat import NotebookNode | ||
from nbclient import NotebookClient, execute as _execute | ||
from nbclient.util import run_sync | ||
# Backwards compatability for imported name | ||
from nbclient.exceptions import CellExecutionError | ||
|
||
|
@@ -31,12 +34,27 @@ def __init__(self, **kw): | |
Preprocessor.__init__(self, nb=nb, **kw) | ||
NotebookClient.__init__(self, nb, **kw) | ||
|
||
def _check_assign_resources(self, resources): | ||
if resources or not hasattr(self, 'resources'): | ||
self.resources = resources | ||
|
||
def preprocess(self, nb, resources=None, km=None): | ||
""" | ||
Preprocess notebook executing each code cell. | ||
|
||
The input argument *nb* is modified in-place. | ||
|
||
Note that this function recalls NotebookClient.__init__, which may look wrong. | ||
However since the preprocess call acts line an init on exeuction state it's expected. | ||
Therefore, we need to capture it here again to properly reset because traitlet | ||
assignments are not passed. There is a risk if traitlets apply any side effects for | ||
dual init. | ||
The risk should be manageable, and this approach minimizes side-effects relative | ||
to other alternatives. | ||
|
||
One alternative but rejected implementation would be to copy the client's init internals | ||
which has already gotten out of sync with nbclient 0.5 release before nbcovnert 6.0 released. | ||
|
||
Parameters | ||
---------- | ||
nb : NotebookNode | ||
|
@@ -56,11 +74,73 @@ def preprocess(self, nb, resources=None, km=None): | |
resources : dictionary | ||
Additional resources used in the conversion process. | ||
""" | ||
# Copied from NotebookClient init :/ | ||
self.nb = nb | ||
self.km = km | ||
if resources: | ||
self.resources = resources | ||
self.reset_execution_trackers() | ||
NotebookClient.__init__(self, nb, km) | ||
self._check_assign_resources(resources) | ||
self.execute() | ||
return nb, resources | ||
return self.nb, self.resources | ||
|
||
async def async_execute_cell( | ||
self, | ||
cell: NotebookNode, | ||
cell_index: int, | ||
execution_count: Optional[int] = None, | ||
store_history: bool = False) -> NotebookNode: | ||
""" | ||
Executes a single code cell. | ||
|
||
Overwrites NotebookClient's version of this method to allow for preprocess_cell calls. | ||
|
||
Parameters | ||
---------- | ||
cell : nbformat.NotebookNode | ||
The cell which is currently being processed. | ||
cell_index : int | ||
The position of the cell within the notebook object. | ||
execution_count : int | ||
The execution count to be assigned to the cell (default: Use kernel response) | ||
store_history : bool | ||
Determines if history should be stored in the kernel (default: False). | ||
Specific to ipython kernels, which can store command histories. | ||
|
||
Returns | ||
------- | ||
output : dict | ||
The execution output payload (or None for no output). | ||
|
||
Raises | ||
------ | ||
CellExecutionError | ||
If execution failed and should raise an exception, this will be raised | ||
with defaults about the failure. | ||
|
||
Returns | ||
------- | ||
cell : NotebookNode | ||
The cell which was just processed. | ||
""" | ||
# Copied and intercepted to allow for custom preprocess_cell contracts to be fullfilled | ||
self.store_history = store_history | ||
cell, resources = self.preprocess_cell(cell, self.resources, cell_index) | ||
if execution_count: | ||
cell['execution_count'] = execution_count | ||
return cell, resources | ||
|
||
def preprocess_cell(self, cell, resources, index, **kwargs): | ||
""" | ||
Override if you want to apply some preprocessing to each cell. | ||
Must return modified cell and resource dictionary. | ||
|
||
Parameters | ||
---------- | ||
cell : NotebookNode cell | ||
Notebook cell being processed | ||
resources : dictionary | ||
Additional resources used in the conversion process. Allows | ||
preprocessors to pass variables into the Jinja engine. | ||
index : int | ||
Index of the cell being processed | ||
""" | ||
self._check_assign_resources(resources) | ||
# Because nbclient is an async library, we need to wrap the parent async call to generate a syncronous version. | ||
cell = run_sync(NotebookClient.async_execute_cell)(self, cell, index, store_history=self.store_history) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MSeal You could have called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, possibly. I was worried any refactors on the wrapping pattern for |
||
return cell, self.resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's comment this line to unpack what is happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do subclasses need to call
super().preprocess_cell()
now? (IIRC they didn't need to before; it's fine either way, although if this has changed it would be great to document the breaking change in the release notes.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MSeal ^^ I'm happy to document the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably best to always call super for this preprocessor now.
Technically you don't have to so long as you call something equivilent to or exactly
NotebookClient.async_execute_cell
, but that's fairly complex to explain or expect given the subtlety of the multi-inheritance and overrides at play.@willingc Documenting as such would be a good idea, happy to merge if you want to make the PR