c.Kubespawner.modify_pod_hook does not support co-routines

[stv0g]

Bug description

I am facing an issue when providing a co-routine to c.Kubespawner.modify_pod_hook.

The Tornado docs for gen.maybe_future() say that maybe_future can only handle Futures and no other yieldable objects.

Does this mean, we can pass coros as a hook function? I mean a coro which as been declared via async def ...

Expected behaviour

c.Kubespawner.modify_pod_hook should support co-routines.

Actual behaviour

c.Kubespawner.modify_pod_hook only supports normal functions or futures.

If passed a co-routine, I will pass the co-routine into the Kubernetes client which fails.

How to reproduce

1. Define a co-routine via the async def syntax:

import asyncio

async def my_pod_modify_hook(spawner):
     await asyncio.sleep(1)
     return

c.Kubespawner.modify_pod_hook = my_pod_modify_hook

Your personal set up

A pretty standard Z2JH setup (master)

[welcome[bot]]

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively. You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

[consideRatio]

@stv0g can you try make this bug report (?) clearer by providing:

• a way to reproduce the faulty behavior
• a description of the faulty behavior that follows
• a description of what you expected

[stv0g]

@consideRatio I updated the issue description.

[consideRatio]

Thanks @stv0g!

Not being an expert about this, this statement leaves me with open questions.

Expected behaviour

c.Kubespawner.modify_pod_hook should support co-routines.

Do you mean that the modify_pod_hook should be allowed to be a function with async for example?

I'm also a bit uncertain about the example. I assume what my_pod_modify_hook actually should do, is supposed to return the modification rather than a future.

import asyncio

async def my_pod_modify_hook(spawner):
-    return asyncio.sleep(1)
+    return await asyncio.sleep(1)

c.Kubespawner.modify_pod_hook = my_pod_modify_hook

[stv0g]

Do you mean that the modify_pod_hook should be allowed to be a function with async for example?

Exactly. The hook functions of JupyterHub support this now already for some time by replacing Tornados gen.maybe_future: https://github.com/jupyterhub/jupyterhub/blob/0ca2ef68f04b59648dd9ad4ce85454c24f533f5d/jupyterhub/utils.py#L493

I propose that Kubespawner should use the same implementation as JupyterHub.

I'm also a bit uncertain about the example. I assume what my_pod_modify_hook actually should do, is supposed to return the modification rather than a future.

You are right. This is an error in my example.

[consideRatio]

Ah okay excellent now I understand and agree with this being something to address!

I'll go ahead and frame this as a enhancement request rather than a bug =)

Could you also help indicate what kind symptoms following setting c.Kubespawner.modify_pod_hook = my_async_func ? Does it explicitly error or does nothing happen etc?

I think with that the issue should be quite easy to get actionable with even for someone like me not so confident about Python async stuff in general.

[consideRatio]

@stv0g current master is using kubespawner 0.16.1 as of a day or two, are you using 0.16.1?

@minrk @stv0g I'm not confident what should be fixed, but perhaps KubeSpawner should stop using gen.maybe_future and instead rely on from jupyterhub.utils import maybe_future? Do you think this makes sense? I'm just guessing.

KubeSpawner's current implementation

https://github.com/jupyterhub/kubespawner/blob/386a24c8085d90de21c498c50484db4a236479c9/kubespawner/spawner.py#L879-L899

https://github.com/jupyterhub/kubespawner/blob/386a24c8085d90de21c498c50484db4a236479c9/kubespawner/spawner.py#L2391-L2392

JupyterHub implementation

Not sure about this either... Hmm...

https://github.com/jupyterhub/jupyterhub/blob/0ca2ef68f04b59648dd9ad4ce85454c24f533f5d/jupyterhub/spawner.py#L645-L665

    pre_spawn_hook = Any(
    # ...

https://github.com/jupyterhub/jupyterhub/blob/0ca2ef68f04b59648dd9ad4ce85454c24f533f5d/jupyterhub/spawner.py#L1029-L1032

    def run_pre_spawn_hook(self):
        """Run the pre_spawn_hook if defined"""
        if self.pre_spawn_hook:
            return self.pre_spawn_hook(self)

https://github.com/jupyterhub/jupyterhub/blob/77220d6662200bf1931ebd26bbac6580789ecd6d/jupyterhub/user.py#L597

            await maybe_future(spawner.run_pre_spawn_hook())

https://github.com/jupyterhub/jupyterhub/blob/0ca2ef68f04b59648dd9ad4ce85454c24f533f5d/jupyterhub/spawner.py#L43

from .utils import maybe_future

https://github.com/jupyterhub/jupyterhub/blob/0ca2ef68f04b59648dd9ad4ce85454c24f533f5d/jupyterhub/utils.py#L493-L517

def maybe_future(obj):
    """Return an asyncio Future
    Use instead of gen.maybe_future
    For our compatibility, this must accept:
    - asyncio coroutine (gen.maybe_future doesn't work in tornado < 5)
    - tornado coroutine (asyncio.ensure_future doesn't work)
    - scalar (asyncio.ensure_future doesn't work)
    - concurrent.futures.Future (asyncio.ensure_future doesn't work)
    - tornado Future (works both ways)
    - asyncio Future (works both ways)
    """
    if inspect.isawaitable(obj):
        # already awaitable, use ensure_future
        return asyncio.ensure_future(obj)
    elif isinstance(obj, concurrent.futures.Future):
        return asyncio.wrap_future(obj)
    else:
        # could also check for tornado.concurrent.Future
        # but with tornado >= 5.1 tornado.Future is asyncio.Future
        f = asyncio.Future()
        f.set_result(obj)
        return f

[stv0g]

I'm not confident what should be fixed, but perhaps KubeSpawner should stop using gen.maybe_future and instead rely on from jupyterhub.utils import maybe_future? Do you think this makes sense? I'm just guessing.

Yes, I would support this. I could also submit a PR :)

[minrk]

KubeSpawner should stop using gen.maybe_future and instead rely on from jupyterhub.utils import maybe_future

Yes! That's the right thing to do.

If you want, though, I don't think kubespawner needs to be as fully general as jupyterhub's own implementation, which aims for maximum compatibility to ensure it didn't break anything when we made the big asyncio transition. If you are a little more strict about what things may return, you can even optimize a bit by avoiding the unnecessary await if it's not awaitable:

result = some_function()
if inspect.isawaitable(result):
    result  = await result

The only case that won't handle is concurrent.futures which is not really important to handle yourself because it's only asking the author of the function to add asyncio.wrap_future() on the end before returning it to kubespawner, if they happen to be using thread pools (tornado's @run_in_executor helper already does this). The only reason jupyterhub handles this is tornado's maybe_future handled it before and we wanted to make sure nothing broke.

Or, alternately, for APIs that should always return None:

result = some_function()
if result is not None:
    await result

which is an even cheaper check than isawaitable():

• x is None ~ 30ns
• inspect.isawaitable(x) ~ 700ns
• jupyterhub.utils.maybe_future(x) ~ 2µs (not counting extra await for non-async callables)

(all these timescales are negligible when adding kubernetes API requests to the mix)