Execute preprocess cells#1380
Conversation
|
Forgot to mention this fixes #1370 |
willingc
left a comment
willingc
left a comment
There was a problem hiding this comment.
@MSeal Thanks for doing this. I've suggested some modifications for clarity. Consider this approved once the failing test is resolved.
There are a lot of deprecation warnings. Do we want to handle in docs or open an issue for future cleanup.
| if resources: | ||
| self.resources = resources | ||
| self.reset_execution_trackers() | ||
| # This may look wrong, but preprocess acts like an init on execution state and |
There was a problem hiding this comment.
Let's move all of this into docstring with an important directive so that this populates through to the docs.
| Index of the cell being processed | ||
| """ | ||
| self._check_assign_resources(resources) | ||
| cell = run_sync(NotebookClient.async_execute_cell)(self, cell, index, store_history=self.store_history) |
There was a problem hiding this comment.
Let's comment this line to unpack what is happening here.
There was a problem hiding this comment.
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.
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
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Future |
| """ | ||
| 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.
@MSeal You could have called NotebookClient.execute_cell here, right?
Shouldn't this cell execution be in async_execute_cell?
There was a problem hiding this comment.
Well, possibly. I was worried any refactors on the wrapping pattern for NotebookClient.execute_cell might make it lazy, causing in infinite loop here in the future since this overwrites async_execute_cell in this child class to allow for intercepting and running preprocess_cell... basically it's a hack to allow us to intercept the the execute_cell call from nbclient and run whatever the nbconvert inheritance tree demands.
|
Shouldn't |
Should nbclient have pre/post execute hooks? Yes probably. I went with a solution here that could unblock the issue with dependencies as is. I think someone was working on adding these in a similar flavor to papermill's hooks but since it hasn't landed it's likely not an active effort. I would not copy the preprocess_cell pattern itself, but if we had the ability to register callbacks in nbclient it would help make this simpler for sure. |
|
Maybe we could revive jupyter/nbclient#79? |
|
Maybe, or making a more concentrated PR for just hook operations? |
5 participants
Fixes inheritance models where subclasses are used to implement preprocess_cells on the execute preprocessor.
There's one hack in this process, but the alternatives are all worse for maintenance pov (including some ones I didn't mention like not dual inheriting classes) so it is what it is to maintain backwards and forward compatibility.