Decide and stick once and for all if functions are async or not ?

[Carreau]

Description

Right now I'm starting to have problem with mypy/async and sync, a couple of functions like km.start_kernel/ks._async_start_kernel are sync/async in upstream jupyter_client, but there are places where there is start_kernel = _async_start_kernel, where upstream it's explicitly start_kernel = run_sync(_async_start_kernel). Now it's problematic as you can't swap one class for the other and mypy complain because:

1) obviously if someone expect a sync method and call an async one, it's won't get run. 2) if someone expect an async and we await a sync method you get error like "str is not an awaitable".

So right now in #1100, I can't fix mypy by correcting start_kernel = _async_start_kernel to start_kernel = run_sync(_async_start_kernel), because then a bunch of test are failing, and can't leave it as is, as it's incorrect WRT jupyter_client.

One possible change is to update jupyter_client to make start_kernel always async ?

I'm not sure which direction to go anymore.

[davidbrochart]

start_kernel should be async in AsyncKernelManager, and sync in KernelManager. I think in jupyter-server we always use AsyncKernelManager, right?

[Carreau]

Sorry the problem is in class MappingKernelManager(MultiKernelManager) (services/kernel/kernelmanager.py). And in this one start_kernel is async, while the superclass is sync.

Maybe it's a question of moving things to AsyncMappingKernelManager ? But as AsyncMappingKernelManager inhering from MappingKernelManager, mypy will stil complain.

[davidbrochart]

We should probably inherit from AsyncMappingKernelManager indeed. start_kernel seems to have the right type there, but maybe it doesn't play well with inheritance. Last resort you could ignore types for these sync/async methods, but I agree it's not great. Supporting sync/async with the same API was for backwards compatibility, but it has its limits.

[kevin-bates]

I think in jupyter-server we always use AsyncKernelManager, right?

By default, instances of ServerKernelManager (which derives from AsyncKernelManager) are created via AsyncMappingKernelManager. Because folks can similarly configure their own MKM, folks can switch to KernelManager by configuring MappingKernelManager.

Sorry the problem is in class MappingKernelManager(MultiKernelManager) (services/kernel/kernelmanager.py). And in this one start_kernel is async, while the superclass is sync.

FWIW - MappingKernelManager.start_kernel() was decorated with @gen.coroutine prior to its transition to an async function, while MultiKernelManager.start_kernel() was not a coroutine so things have been this way for quite a while. This was before the popularity of linters and such and probably falls into python's "loose" checking "feature".

We should probably inherit from AsyncMappingKernelManager indeed. start_kernel seems to have the right type there, but maybe it doesn't play well with inheritance.

Won't this cause issues when synchronous methods on MappingKernelManager are invoked?

I'd rather see us drop the synchronous implementation of MappingKernelManager altogether. We've already diverged between sync and async KernelManager classes since AsyncMappingKernelManager now creates instances of ServerKernelManager. This class was introduced so we could apply server-specific functionality at the kernel manager level (like events) and we require that folks bringing their own KernelManager implementations derive from ServerKernelManager. But folks won't be privy to any built-in functionality when using MappingKernelManager since it still creates instances of KernelManager. As a result, this is a "carrot" for folks to build on async kernel manager functionality.

@Carreau - if we were to drop MappingKernelManager entirely, would #1100 be able to move forward?