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.

Sign up for GitHub

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

Jump to bottom

Async execute cells #1406

Closed
wants to merge 2 commits into from
Closed

Async execute cells #1406

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd7e48f
Async execute cells
davidbrochart Sep 21, 2020
16b367a
Ensure preprocess_cell is async
davidbrochart Sep 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions nbconvert/preprocessors/execute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@

# Copyright (c) IPython Development Team.
# Distributed under the terms of the Modified BSD License.
import asyncio
from typing import Optional
from nbformat import NotebookNode
from nbclient import NotebookClient, execute as _execute
from nbclient.util import run_sync
from nbclient.util import ensure_async
# Backwards compatability for imported name
from nbclient.exceptions import CellExecutionError

Expand Down Expand Up @@ -76,7 +77,12 @@ def preprocess(self, nb, resources=None, km=None):
"""
NotebookClient.__init__(self, nb, km)
self._check_assign_resources(resources)
self.execute()
try:
loop = asyncio.get_event_loop()
except RuntimeError:
loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
loop.run_until_complete(self.async_execute())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may be just moving the issue here. run_until_complete will fail if the loop is already running.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, if nbconvert is used in a notebook for instance. But not if it is used from the CLI.

return self.nb, self.resources

async def async_execute_cell(
Expand Down Expand Up @@ -120,13 +126,13 @@ async def async_execute_cell(
"""
# 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)
cell, resources = await ensure_async(self.preprocess_cell(cell, self.resources, cell_index))
# Apply rules from nbclient for where to apply execution counts
if execution_count and cell.cell_type == 'code' and cell.source.strip():
cell['execution_count'] = execution_count
return cell, resources

def preprocess_cell(self, cell, resources, index, **kwargs):
async def preprocess_cell(self, cell, resources, index, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other preprocessors' preprocess_cell are not async. Should we make an async version of that method and call it from async_execute_cell?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think we just need to ensure it is async, I'm pushing another commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will most likely have consequences, because anyone overriding preprocess_cell in a non-async way will have surprises.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, since we now call ensure_async(preprocess_cell()), it can be async or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed in person, for people who override preprocess_cell and then call super.preprocess_cell in their implementation without await this should be an issue, but I need to check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an issue and it will break downstream projects like nbdev if preprocess_cell is made async in nbconvert

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try because I don't think it is a problem with the ensure_async.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it will be an issue to mix:

In [8]: class A:
   ...:     async def foo(self):
   ...:         print("bar")
   ...: 

In [9]: class B(A):
   ...:     def foo(self):
   ...:         print("foo")
   ...:         A.foo(self)
   ...:         print("!")
   ...: 

In [10]: B().foo()
foo
<ipython-input-9-33f3ae08d87f>:4: RuntimeWarning: coroutine 'A.foo' was never awaited
  A.foo(self)
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
!

Making the subclass aware of the parent being async but not being async itself on the override does weird things too:

In [12]: class B(A):
    ...:     def foo(self):
    ...:         print("foo")
    ...:         await A.foo(self)
    ...:         print("!")
    ...: 

In [13]: B().foo()
Out[13]: <coroutine object B.foo at 0x7f1d87179c40>

"""
Override if you want to apply some preprocessing to each cell.
Must return modified cell and resource dictionary.
Expand All @@ -142,6 +148,5 @@ def preprocess_cell(self, cell, resources, index, **kwargs):
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)
await NotebookClient.async_execute_cell(self, cell, index, store_history=self.store_history)
return cell, self.resources