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.

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

Async support #23

Merged
merged 1 commit into from
Oct 8, 2019
Merged

Async support #23

merged 1 commit into from
Oct 8, 2019

Conversation

kevin-bates
Copy link
Collaborator

Got start and restarts working. I've switched to a single launch method that is defined as aync. This is because we can't have both due to the way restarts work (calling the (previously synchronous) launch method and we shouldn't require providers to implement both forms - especially when async support is required to be implemented deep in the provider (i.e., it may not be practical to implement both forms). Since async support is clearly warranted and in line with future directions, we should adopt at as the single mechanism.

By switching to a single method, providers that ONLY support synchronous operations can still implement their provider that way, but will need to invoke the KernelFinder (or provider's) launch method using something like the following (from tests/utils.py):

def run_sync(coro_method):
    return asyncio.get_event_loop().run_until_complete(coro_method)

Removed support for Python 3.4 so we could just focus on 3.5+ with async/await.

The experimental notebook branch has been modified to make calls with knowledge that async support is implemented.

@codecov-io
Copy link

codecov-io commented Aug 26, 2019

Codecov Report

Merging #23 into master will decrease coverage by <.01%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   71.64%   71.63%   -0.01%     
==========================================
  Files          27       27              
  Lines        2084     2073      -11     
==========================================
- Hits         1493     1485       -8     
+ Misses        591      588       -3
Impacted Files Coverage Δ
jupyter_kernel_mgmt/tests/utils.py 53.65% <100%> (+3.65%) ⬆️
jupyter_kernel_mgmt/tests/test_async_manager.py 100% <100%> (ø) ⬆️
jupyter_kernel_mgmt/tests/test_restarter.py 100% <100%> (ø) ⬆️
jupyter_kernel_mgmt/restarter.py 58.9% <100%> (ø) ⬆️
jupyter_kernel_mgmt/discovery.py 70.32% <75%> (+6.1%) ⬆️
jupyter_kernel_mgmt/tests/test_discovery.py 91.39% <91.66%> (-5.95%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update debe5a9...fbccf7e. Read the comment docs.



def run_sync(coro_method):
return asyncio.get_event_loop().run_until_complete(coro_method)
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like there is a pytest-asyncio plugin which can streamline writing tests for async code.

@takluyver
Copy link
Owner

To record our conversation from other venues, I am mostly OK with deduplicating the code paths by only having async kernel management methods. However, I would like to know:

  1. How awkward is it to wrap these async APIs from synchronous code? I've asked @kevin-bates to try adapting https://github.com/jupyter/nbconvert/pull/809/files to use the APIs proposed here as an example use case. Another use case would be jupyter_kernel_test, which already uses jupyter_kernel_mgmt in master.
  2. How would async/await APIs work for code built around the Qt event loop, e.g. the Qt console? @ccordoba12 might have some useful input here.

@ccordoba12
Copy link
Contributor

How would async/await APIs work for code built around the Qt event loop, e.g. the Qt console?

I think to deal with the Qt event loop + asyncio you need an extra layer of coordination, something like what's provided by this package:

https://github.com/gmarull/asyncqt

@takluyver
Copy link
Owner

Thanks Carlos. Have you used that before? Do you have a feeling for how hard it would be if you needed to use async/await code for things like launching kernels?

@ccordoba12
Copy link
Contributor

Have you used that before?

Nop, I saw it in Reddit some time ago and saved the reference in case we needed to use it in Spyder.

Do you have a feeling for how hard it would be if you needed to use async/await code for things like launching kernels?

It doesn't seem that hard to use, at least according to its examples. But I'm really not sure.

So there won't be a way to ask for synchronous launching of kernels in the future? I mean, that would allow qtconsole to keep working without problems.

@takluyver
Copy link
Owner

I think it's easy enough if you want to run the async calls synchronously - you can call something like asyncio.run(start_kernel()) to run a temporary event loop until it's done. But if the notebook can launch kernels asynchronously, kernel providers can take advantage of that to do more complex things, and simply calling it synchronously will lock up the UI while it runs, as I understand it.

@ccordoba12
Copy link
Contributor

and simply calling it synchronously will lock up the UI while it runs, as I understand it

Right, I forgot about it.

Start and restart are working using async/await.

Switch to single (async) launch method.

Removed support for Python 3.4 so we could just focus on 3.5+ with async/await.

Updated tests after rebase of launch-params PR.
@kevin-bates
Copy link
Collaborator Author

@takluyver - the momentum is increasing to get kernel-mgmt into jupyter_server. Since we definitely want async kernel management support, and given we've been able to show existing (sync) applications could utilize this async-based library, is there anything else preventing this being merged?

When migrating to jupyter_server, I'd kinda like async support to part of that initial effort.

@takluyver
Copy link
Owner

No, go for it. At some point I think it would be a good idea to use the pytest-asyncio plugin for the tests, but that's not high priority.

@takluyver takluyver merged commit 9e7aa09 into takluyver:master Oct 8, 2019
@kevin-bates kevin-bates deleted the async-startup branch November 8, 2019 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants