Remove nest-asyncio dependency #835
Conversation
|
Looks like you're having fun in there 😄 |
Ha, yes and no. |
|
I think we introduced a failure: |
Co-authored-by: Min RK <benjaminrk@gmail.com>
jupyter_client/ioloop/manager.py
Outdated
| try: | ||
| orig_socket = socket | ||
| socket = f(self, *args, **kwargs) | ||
| orig_socket.close() |
There was a problem hiding this comment.
Why is orig_socket created and closed, instead of removed entirely?
There was a problem hiding this comment.
Didn't we need to create it to determine it's type?
There was a problem hiding this comment.
No, we only check self.context._socket_class which we can do before creating sockets
There was a problem hiding this comment.
Okay, I removed the throwaway socket
| @@ -807,6 +807,10 @@ def send( | |||
| # ZMQStreams and dummy sockets do not support tracking. | |||
| track = False | |||
|
|
|||
| if isinstance(stream, zmq.asyncio.Socket): | |||
| assert stream is not None | |||
| stream = zmq.Socket.shadow(stream.underlying) | |||
| @@ -807,6 +807,10 @@ def send( | |||
| # ZMQStreams and dummy sockets do not support tracking. | |||
| track = False | |||
|
|
|||
| if isinstance(stream, zmq.asyncio.Socket): | |||
| assert stream is not None | |||
There was a problem hiding this comment.
I assume leftover debug, this will never be None if it's an asyncio.Socket
| assert stream is not None |
There was a problem hiding this comment.
This is there to satisfy mypy
There was a problem hiding this comment.
type checkers, always making code better!
…lient into synchronous_managers
|
zeromq/pyzmq#1785 introduces support in ZMQStream for both asyncio sockets (by immediately shadowing them to sync sockets) and allowing coroutines in on_recv callbacks. It also makes it easier to explicitly create a particular socket class per |
tests/test_restarter.py
Outdated
| @@ -191,7 +192,7 @@ async def test_async_restart_check(config, install_kernel, debug_logging): | |||
| km = AsyncIOLoopKernelManager(kernel_name=install_kernel, config=config) | |||
|
|
|||
| cbs = 0 | |||
| restarts = [asyncio.Future() for i in range(N_restarts)] | |||
| restarts = [asyncio.futures.Future() for i in range(N_restarts)] | |||
There was a problem hiding this comment.
This should still be asyncio.Future. That's the correct, public name for this. asyncio.futures is an implementation detail, which can change.
There was a problem hiding this comment.
...unless these are meant to now be concurrent.futures.Futures? concurrent futures are also awaitable with await asyncio.wrap_future(concurrent_future)
There was a problem hiding this comment.
We needed to use concurrent futures in the non-async tests, and asyncio futures in the async ones, I'll update to use asyncio.Future
|
Downstream errors are addressed, I'm going to merge and iterate. |
I know that this is already obsolete with jupyter#835. Sometimes, this loop is not marked by nest_asyncio (when calling `run_sync`) so this makes sure that the two eventloops (`asyncio.new_event_loop()` and the other created in `ioloop.IOLoop()`) do not make the system crash. Unfortunately, I can not really give you a reproducible example as I was messing with spyder code when it happened. Feel free to close this PR if you think this is not needed.
Remove the need for
nest_asyncioby only calling asynchronous methods on self from within an asynchronous method and using the task runner method described in Clean up synchronous managers #755The blocking channel/client/manager methods call
run_syncto run ad-hoc and background loop(s) as needed, and usezmq.Context. The async channel/client/manager useasync/awaitandzmq.asyncio.Context.For session objects, we make all of the methods synchronous and check for futures returned by socket methods, converting them to values as needed. We had to subclass
ZMQStreamto do the same, but perhaps this logic should go inpyzmqitself.Add tests for async and threaded clients