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

Add support for async kernel management #4479

Merged
merged 10 commits into from Apr 4, 2020

Conversation

kevin-bates
Copy link
Member

This PR adds support for asynchronous kernel management in an optional manner. This approach will allow the project as a whole to move forward while applications that wish to leverage the new functionality can do so at their convenience until the appropriate time. At some point, we should converge the implementations to async only.

Async kernel management is dependent on changes in jupyter_client PR 428. In that PR, both MultiKernelManager and KernelManager have been extended with direct subclasses AsyncMultiKernelManager and AsyncKernelManager. Same goes for the pair of IOLoop classes. Please view that PR for details.

With these changes, the MappingKernelManager has been refactored as follows:

  1. A separate, and disjoint, class has been introduced named AsyncMappingKernelManager. This class is essentially the equivalent of MappingKernelManager, but for complete async behavior. It is disjoint because it must derive from AsyncMultiKernelManager, so cannot derive from notebook's MappingKernelManager (which derives from MultiKernelManager).
  2. To minimize duplication, the buffering, activity monitoring and culling pieces have been factored into separate internal classes such that the two respective mapping kernel manager classes have a has a relationship to each. Note that in order to facilitate any existing subclasses of MappingKernelManager, the existing methods relative to each of these classes remain in place in case they were overridden in subclasses. It would be good to determine if there are overrides (somehow). If none exist, those place-holder methods should be removed.
  3. To further minimize duplication, a second superclass has been introduced - MappingKernelManagerBase. This class is needed for a couple reasons.
    1. It holds config properties and methods that do not require access to the true super classes (since those are disjoint).
    2. It serves has the required class for class-based traits relative to either of the MappingKernelManager classes.
      Once we converge back to a single mapping kernel manager, we can remove this class - moving its contents back onto the one mapping kernel manager class.

To switch to using AsyncMappingKernelManager, the following command-line option is necessary:

--NotebookApp.kernel_manager_class=notebook.services.kernels.kernelmanager.AsyncMappingKernelManager

leveraging the existing ability to customize kernel manager classes.

Also note, that code exists to NOT require upgrade of juptyer_client until the project is ready to update dependencies. As a result, no upgrade of jupyter_client is required unless the kernel_manager_class override is in place. In addition, should someone enable that option without having upgraded jupyter_client, the following RuntimeError will be produced (along with its traceback) when Notebook is started:

RuntimeError: Using AsyncMappingKernelManager without an appropriate jupyter_client installed! Upgrade jupyter_client and try again.

Similarly for the notebook kernel api tests. If the appropriate jupyter_client is not available, the async-related tests will be skipped - logging an appropriate message. Those tests derive directly from the non-async tests by merely setting the NotebookApp.kernel_manager_class.

Note: If AsyncMappingKernelManager is in use, then any command-line or configuration options referencing MappingKernelManager need to be replaced with AsyncMappingKernelManager. If you need to switch back and forth, it may be easier to just reference MappingKernelManagerBase instead.

Here's a class diagram, extending the one shown in the jupyter_client PR.
notebook_kernel_manager_hierarchy

Although this has been tested with and without the appropriate jupyter_client present, I'd like to keep this PR as WIP until a POC with Enterprise Gateway has been completed and the jupyter_client PR has been resolved.

@kevin-bates kevin-bates changed the title [WIP] Add support for async kernel management Add support for async kernel management Mar 16, 2019
@kevin-bates
Copy link
Member Author

The POC with Enterprise Gateway has completed so I've removed the [WIP] status. There are some test failures in python 3.7 that I'm struggling to figure out since I can't reproduce them in my 3.7 env. In addition, they appear to be socket related - so I'm struggling to see the link. Any help/guidance would be appreciated.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/scalable-enterprise-gateway/2014/2

Use composition in preparation for AsyncMappingKernelManager.
Supports running against incompatible jupyter_client so long as the
desired kernel_manager_class is not `AsyncMappingKernelManager`.
@davidbrochart
Copy link
Member

Yes, please go ahead. No need to co-author me in.

@kevin-bates
Copy link
Member Author

I think we need to revisit David's proposed class hierarchy approach: jupyter-server/jupyter_server#191 (comment)

@kevin-bates
Copy link
Member Author

I've pushed changes that switch to the approach taken in jupyter-server/jupyter_server#191 after we addressed the concerns I raised in the previous comment. This push also includes the display of the notebook server's version when starting (mentioned for the sake of transparency).

This commit uses the approach used in jupyter_server jupyter#191 first proposed
by David Brochart.  This reduces code duplication and alleviates redundancy
relative to configurable options.

Also, the startup message now includes the version information.

Co-authored-by: David Brochart <david.brochart@gmail.com>
@kevin-bates
Copy link
Member Author

@Zsailer, @davidbrochart, @lresende could you please review this PR. It's essentially the same as jupyter-server/jupyter_server#191 except that the changes to async/await are focused on kernel management and not other services in order to minimize changes.

Once approved and merged, I think we should cut a 6.1.0 release soon after as other projects are awaiting (like that? 😄) this functionality.

Comment on lines 1804 to 1808
# If we're using async kernel management, we need to invoke the async method via the event loop.
if isinstance(self.kernel_manager, AsyncMappingKernelManager):
asyncio.get_event_loop().run_until_complete(self.kernel_manager.shutdown_all())
else:
self.kernel_manager.shutdown_all()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you want to be more cautious, and use the convenient run_sync we will use in jupyter_server:

run_sync(self.kernel_manager.shutdown_all())

Comment on lines 125 to 127
def __init__(self, **kwargs):
super(MappingKernelManager, self).__init__(**kwargs)
super().__init__(**kwargs)
self.last_kernel_activity = utcnow()
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed?

Copy link
Member

Choose a reason for hiding this comment

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

Haha I just added mentioned this too! 👍

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

Clever work, @kevin-bates and @davidbrochart! Great job!

Just some minor comments. I'm a little worried about naming the important attribute here super (read my inline comments for more info). It's a non-blocking change, so if you want to keep it, I'm fine with it.

Comment on lines 125 to 127
def __init__(self, **kwargs):
super(MappingKernelManager, self).__init__(**kwargs)
super().__init__(**kwargs)
self.last_kernel_activity = utcnow()
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed? It looks like a near duplicate of lines 139-142.

Comment on lines 139 to 142
def __init__(self, **kwargs):
self.super = MultiKernelManager
self.super.__init__(self, **kwargs)
self.last_kernel_activity = utcnow()

Copy link
Member

Choose a reason for hiding this comment

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

Duplicate of above?

@@ -130,6 +136,11 @@ def __init__(self, **kwargs):
# Methods for managing kernels and sessions
#-------------------------------------------------------------------------

def __init__(self, **kwargs):
self.super = MultiKernelManager
Copy link
Member

Choose a reason for hiding this comment

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

Clever! I was slightly uncomfortable with this when I first saw it—I thought you just wanted an alias for super(...), but I now recognize this is the magic for the class hierarchy.

That said, I have two recommendations...

  1. I think we need to add some thorough inline comments here (and below, in AsyncMultiKernelManager. Especially using the attr name super, I'm afraid someone reading this a few years from now might think this is a mistake... that you actually mean super().
  2. Maybe even consider renaming this attribute? Something like base_multikernel_manager or something that makes it clear you're calling a class other than super().

Copy link
Member

Choose a reason for hiding this comment

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

I agree it could be confusing to name it super, my bad!

Add comments and rename self.super to self.pinned_superclass to
clarify intent.

Add run_sync() util method to clean up shutdown_all() invocation.
@kevin-bates
Copy link
Member Author

@Zsailer - Any idea why Travis is not happening? It is happening on my repo at least: https://travis-ci.org/github/kevin-bates/notebook

@Zsailer
Copy link
Member

Zsailer commented Mar 27, 2020

Hm, that's weird. It's not showing up here, but I see it running on travis-ci.org.

@Zsailer
Copy link
Member

Zsailer commented Mar 27, 2020

@kevin-bates Travis is all green. This PR is good to go from my perspective.

Copy link
Member

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

LGTM

@Zsailer
Copy link
Member

Zsailer commented Apr 1, 2020

Is this ready for merge, @kevin-bates ?

@kevin-bates
Copy link
Member Author

Thanks for asking Zach. I was just about to add the logic to prevent async kernel management on 3.5 - like we're doing elsewhere in the ecosystem. I'll ping you once completed.

@kevin-bates
Copy link
Member Author

Hi @Zsailer - this should be ready for review/merge.

Aside from adding the checks for 3.5, I found the test-skipping logic wasn't working correctly because the server startup occurs as part of the setup_class() method, while I had been "skipping" the test in the setUp() method - which was too late. The python 3.5 group now shows this: SKIP: AsyncKernelAPITest tests skipped due to Python < 3.6!

I also needed to add an initializer to the fake AsyncMultiKernelManager class that's fabricated. This allows the initial load (of the fabricated class) to succeed in order for the server to determine that an async kernel manager is being configured when it's not available/supported.

Lastly, the test failure appears to be transient related to nvm installation. I got the same installation failure on a different group in my repo. I reran the build and the nvm installation succeeded.

@kevin-bates kevin-bates closed this Apr 2, 2020
@kevin-bates
Copy link
Member Author

Trigger CI

@kevin-bates kevin-bates reopened this Apr 2, 2020
@Zsailer
Copy link
Member

Zsailer commented Apr 2, 2020

This looks good, @kevin-bates. I'm ready to merge.

@kevin-bates
Copy link
Member Author

@Zsailer - per your suggestion in today's server meeting, I added comments indicating when a given dependency check (jupyter_client/Python versions) can be removed. Thanks for the tip.

@Zsailer
Copy link
Member

Zsailer commented Apr 4, 2020

@kevin-bates Awesome work here! This is good to go. Merging away!

@Zsailer Zsailer merged commit d04cbb6 into jupyter:master Apr 4, 2020
@kevin-bates kevin-bates mentioned this pull request May 5, 2020
24 tasks
@kevin-bates kevin-bates deleted the async-subclasses branch May 26, 2020 21:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants