Async execute cells #1406
Async execute cells #1406
Conversation
| # 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): |
There was a problem hiding this comment.
other preprocessors' preprocess_cell are not async. Should we make an async version of that method and call it from async_execute_cell?
There was a problem hiding this comment.
No, I think we just need to ensure it is async, I'm pushing another commit.
There was a problem hiding this comment.
it will most likely have consequences, because anyone overriding preprocess_cell in a non-async way will have surprises.
There was a problem hiding this comment.
I don't think so, since we now call ensure_async(preprocess_cell()), it can be async or not.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is an issue and it will break downstream projects like nbdev if preprocess_cell is made async in nbconvert
There was a problem hiding this comment.
We should try because I don't think it is a problem with the ensure_async.
There was a problem hiding this comment.
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>
| except RuntimeError: | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
| loop.run_until_complete(self.async_execute()) |
There was a problem hiding this comment.
We may be just moving the issue here. run_until_complete will fail if the loop is already running.
There was a problem hiding this comment.
I agree, if nbconvert is used in a notebook for instance. But not if it is used from the CLI.
|
Note: it seems that the nest-asyncio author just pushed a fix: erdewit/nest_asyncio#34 If a patch release is published, we should probably just use that and close this. |
|
Closing for now, since this approach will probably not work. However, I would like to make a pass on the interface between nbclient and the execute preprocessor. |
Fixes jupyter/nbclient#108