From 41d6f195575cbe8e52e41ed43436546b8793b0e4 Mon Sep 17 00:00:00 2001 From: Matthias Bussonnier Date: Fri, 19 Oct 2018 17:55:17 -0700 Subject: [PATCH 1/5] Start working on ability to have async_start_kernel in KernelManagers. I would like to have ability to have async function in there in particular because I need to start a kernel via slurm, and I don not kow ahead of time which node it is going to be schedule on. So far most of the solutions there are hacky and require having ssh tunnels, which I would like to avoid. As so far the start_kernel function already setup and connect ports it requires to be made async, or deeply change the API. I went with creating and checking for a second 'async_start_kernel' on super, as it is MultiKernelManager in jupyter_clienc, and changing `start_kernel` to be a coroutine function would be a break of API. I have a patch on jupyter_client that itself check whether prorper kernel manager start_kernel function are coroutine functions or not. In the long run this should allow to progressively migrate jupyter_client to use and async start_kernel, but we can't yet as anyway we still support 2.7 on jupyter-Client. Note that gen.maybe_future in tornado is deprecated, and it is recommended to check for results and yield unknown objects: Deprecated since version 4.3: This function only handles Futures, not other yieldable objects. Instead of maybe_future, check for the non-future result types you expect (often just None), and yield anything unknown The kernel_id is a Unicode Traitlet so unless subclass overwite that should be fine. --- notebook/services/kernels/kernelmanager.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/notebook/services/kernels/kernelmanager.py b/notebook/services/kernels/kernelmanager.py index 6d2158ef8a..ee924e5224 100644 --- a/notebook/services/kernels/kernelmanager.py +++ b/notebook/services/kernels/kernelmanager.py @@ -158,9 +158,19 @@ def start_kernel(self, kernel_id=None, path=None, **kwargs): if kernel_id is None: if path is not None: kwargs['cwd'] = self.cwd_for_path(path) - kernel_id = yield gen.maybe_future( - super(MappingKernelManager, self).start_kernel(**kwargs) - ) + + sup = super(MappingKernelManager, self) + async_sk = getattr(sup, 'async_start_kernel', None) + if async_sk is not None: + res = super().async_start_kernel(**kwargs) + else: + res = super().start_kernel(**kwargs) + + if isinstance(res, str): + kernel_id = res + else: + kernel_id = yield res + self._kernel_connections[kernel_id] = 0 self.start_watching_activity(kernel_id) self.log.info("Kernel started: %s" % kernel_id) From 80d407130770041aec6d50def042aacb3c6ed171 Mon Sep 17 00:00:00 2001 From: Matthias Bussonnier Date: Mon, 22 Oct 2018 09:14:47 -0700 Subject: [PATCH 2/5] consistant naming --- notebook/services/kernels/kernelmanager.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/notebook/services/kernels/kernelmanager.py b/notebook/services/kernels/kernelmanager.py index ee924e5224..73cae9f02b 100644 --- a/notebook/services/kernels/kernelmanager.py +++ b/notebook/services/kernels/kernelmanager.py @@ -160,9 +160,9 @@ def start_kernel(self, kernel_id=None, path=None, **kwargs): kwargs['cwd'] = self.cwd_for_path(path) sup = super(MappingKernelManager, self) - async_sk = getattr(sup, 'async_start_kernel', None) + async_sk = getattr(sup, 'start_kernel_async', None) if async_sk is not None: - res = super().async_start_kernel(**kwargs) + res = super().start_kernel_async(**kwargs) else: res = super().start_kernel(**kwargs) From 20bb0e33b38d87e308e9e5b6f937aae499da8341 Mon Sep 17 00:00:00 2001 From: Matthias Bussonnier Date: Fri, 26 Oct 2018 14:27:38 -0700 Subject: [PATCH 3/5] try to handle async restart --- notebook/services/kernels/kernelmanager.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/notebook/services/kernels/kernelmanager.py b/notebook/services/kernels/kernelmanager.py index 73cae9f02b..366bce7b2c 100644 --- a/notebook/services/kernels/kernelmanager.py +++ b/notebook/services/kernels/kernelmanager.py @@ -28,6 +28,8 @@ from notebook.prometheus.metrics import KERNEL_CURRENTLY_RUNNING_TOTAL +import asyncio + class MappingKernelManager(MultiKernelManager): """A KernelManager that handles notebook mapping and HTTP error handling""" @@ -162,8 +164,10 @@ def start_kernel(self, kernel_id=None, path=None, **kwargs): sup = super(MappingKernelManager, self) async_sk = getattr(sup, 'start_kernel_async', None) if async_sk is not None: + self.log.debug('dispathcing start to async super') res = super().start_kernel_async(**kwargs) else: + self.log.debug('dispathcing start to sync super') res = super().start_kernel(**kwargs) if isinstance(res, str): @@ -306,10 +310,19 @@ def shutdown_kernel(self, kernel_id, now=False): return super(MappingKernelManager, self).shutdown_kernel(kernel_id, now=now) + @asyncio.coroutine def restart_kernel(self, kernel_id): """Restart a kernel by kernel_id""" self._check_kernel_id(kernel_id) - super(MappingKernelManager, self).restart_kernel(kernel_id) + sup = super(MappingKernelManager, self) + + if hasattr(sup, 'restart_kernel_async'): + self.log.debug('dispatching restart_kernel to async super') + yield from sup.restart_kernel_async(kernel_id) + else: + self.log.debug('dispatching restart_kernel to sync super') + sup.restart_kernel(kernel_id) + kernel = self.get_kernel(kernel_id) # return a Future that will resolve when the kernel has successfully restarted channel = kernel.connect_shell() From b7e4d4baf127d7b88d5f0e35f301842f07413f64 Mon Sep 17 00:00:00 2001 From: Matthias Bussonnier Date: Wed, 31 Oct 2018 13:19:36 -0700 Subject: [PATCH 4/5] Try to handle the case where restart_kernel is a coroutine --- notebook/services/kernels/handlers.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/notebook/services/kernels/handlers.py b/notebook/services/kernels/handlers.py index ef90bf9122..505f0f24fb 100644 --- a/notebook/services/kernels/handlers.py +++ b/notebook/services/kernels/handlers.py @@ -29,7 +29,11 @@ class MainKernelHandler(APIHandler): @gen.coroutine def get(self): km = self.kernel_manager - kernels = yield gen.maybe_future(km.list_kernels()) + res = km.list_kernels() + if not isinstance(res, list): + kernels = res + else: + kernels = yield res self.finish(json.dumps(kernels, default=date_default)) @web.authenticated @@ -82,7 +86,9 @@ def post(self, kernel_id, action): if action == 'restart': try: - yield gen.maybe_future(km.restart_kernel(kernel_id)) + res = km.restart_kernel(kernel_id) + if res is not None + yield from res except Exception as e: self.log.error("Exception restarting kernel", exc_info=True) self.set_status(500) From c16b4aac546f37d215245b62e282c53fb2a1c6ba Mon Sep 17 00:00:00 2001 From: Matthias Bussonnier Date: Wed, 31 Oct 2018 13:23:51 -0700 Subject: [PATCH 5/5] typo --- notebook/services/kernels/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook/services/kernels/handlers.py b/notebook/services/kernels/handlers.py index 505f0f24fb..7fef1963d2 100644 --- a/notebook/services/kernels/handlers.py +++ b/notebook/services/kernels/handlers.py @@ -87,7 +87,7 @@ def post(self, kernel_id, action): try: res = km.restart_kernel(kernel_id) - if res is not None + if res is not None: yield from res except Exception as e: self.log.error("Exception restarting kernel", exc_info=True)