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

AsyncKernelClient.is_alive() returns non-awaited coroutine? #621

Closed
davidbrochart opened this issue Mar 5, 2021 · 3 comments · Fixed by #623
Closed

AsyncKernelClient.is_alive() returns non-awaited coroutine? #621

davidbrochart opened this issue Mar 5, 2021 · 3 comments · Fixed by #623

Comments

@davidbrochart
Copy link
Member

async is_alive() can return:

  1. an awaited coroutine if it was created by an AsyncKernelManager.
  2. or its parent's is_alive() if it was created by a KernelManager which is not AsyncKernelManager.
  3. or the heartbeat's is_beating() if we don't have access to the kernel manager that created it but we have a heartbeat.
  4. or True if we don't even have a heartbeat.

Cases 1. and 4. are fine, but 2. and 3. could potentially return a non-awaited coroutine. This is causing problems e.g. in jupyter/nbclient#138, but I think the main issue is that we don't know the objects that we are manipulating any more, because we are trying to have a single code base that works for async and non-async. I also think that we are allowing for too much freedom, for instance mixing a non async kernel manager and an async kernel client. This is becoming too complicated.
I think adding type annotations, as @MSeal suggested in #553, will help us understand and improve the situation.

@kevin-bates
Copy link
Member

Hi @davidbrochart - I've been following the discussion on nbclient and, until now, didn't realize the is_alive() being discussed was on AsyncKernelClient rather than AsyncKernelManager. Could we perhaps update the title to include AsyncKernelClient.is_alive?

I also think that we are allowing for too much freedom, for instance mixing a non async kernel manager and an async kernel client.

I totally agree. I was surprised to see this in nbclient a few months back.

I'm not really following how 3 can return a coroutine (awaited or otherwise) and the same goes for 2 unless there's a subclass of KernelManager that is not AsyncKernelManager that overrides is_alive() to be a coroutine.
Could you point out how the 2 or 3 could return non-awaited coroutines? (mostly for my own education as this stuff isn't always obvious to me.)

A refactoring of AsyncKernelClient.is_alive() that cuts out some of the duplication could be:

    async def is_alive(self):
        """Is the kernel process still running?"""
        from ..manager import AsyncKernelManager
        if isinstance(self.parent, AsyncKernelManager):
                return await self.parent.is_alive()
        return super().is_alive() # defer to KernelClient

or perhaps testing if self.parent.is_alive is a coroutine directly - which might help with the async subclass scenario.

@davidbrochart
Copy link
Member Author

Hi Kevin, I'm not sure 2. or 3. are returning a coroutine indeed, maybe you're right they can never do so, it's just that looking at the code it is not obvious, and making any change in this code base requires such a deep knowledge of how everything works that this is becoming complicated. Having type annotations would help us get some certitudes about what's going on, and maybe fix bugs or refactor.

@davidbrochart davidbrochart changed the title async is_alive() returns non-awaited coroutine? AsyncKernelClient.is_alive() returns non-awaited coroutine? Mar 5, 2021
@kevin-bates
Copy link
Member

Having type annotations would help us get some certitudes about what's going on, and maybe fix bugs or refactor.

I totally agree and have found this exercise helpful in other projects.

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 a pull request may close this issue.

2 participants