Use jupyter_client's AsyncKernelManager #191

davidbrochart · 2020-03-16T13:50:28Z

No description provided.

kevin-bates

Thank you for adding this in @davidbrochart - this will be extremely useful to have prior to moving to kernel providers. This also appears to be a general gen.coroutine/yield -> async/await sweep - or at least a start - which is really nice!

I only had a few minor comments.

jupyter_server/services/kernels/kernelmanager.py

kevin-bates · 2020-03-16T14:39:49Z

jupyter_server/services/sessions/sessionmanager.py

-        )
-        # py2-compat
-        raise gen.Return(kernel_id)
+        kernel_id = await ensure_async(self.kernel_manager.start_kernel(path=kernel_path, kernel_name=kernel_name))


I don't think we need the ensure_async() wrapper here.

Good point, and this involves some changes in the tests too.

setup.py

Zsailer · 2020-03-16T15:17:01Z

👍 Thanks, @davidbrochart.

The failing AppVeyor test is due to a Github webhook that needed to be deleted. I just removed it, so it should drop from the test list.

davidbrochart · 2020-03-16T15:31:12Z

Not sure about these errors on Windows for python3.5 and 3.6.

davidbrochart · 2020-03-16T15:42:08Z

Trigger CI.

davidbrochart · 2020-03-16T15:48:50Z

Now the error is only on Windows/python3.5.

kevin-bates

Thanks David!

I recall seeing the Windows issue when working with kernel providers. Unfortunately, I don't recall the solution. I was hoping this issue would provide some hints, but IIRC this was following my encounter with the invalid handle issue. I'll google around and see if that rings any bells.

kevin-bates · 2020-03-16T16:33:20Z

Hmm - Windows 3.5 is sounding more familiar and I definitely recall this google hit: https://bugs.python.org/issue37380. Still trying to remove the cobwebs.

…oroutine/yield to async/await, remove run_blocking

echarles · 2020-03-16T17:31:16Z

Thx @davidbrochart

@kevin-bates When the kernel provider will be merged, my understanding is that this async kernel-client implementation will be completely replaced by the kernel-protocol code. Is this correct?

kevin-bates · 2020-03-16T17:44:54Z

@echarles - yes, that's correct. We'll want to get that client implementation up on async. I'm fairly certain @davidbrochart is aware of that: jupyter/jupyter_client#428 (comment)

davidbrochart · 2020-03-17T09:07:43Z

Hmm - Windows 3.5 is sounding more familiar and I definitely recall this google hit: https://bugs.python.org/issue37380. Still trying to remove the cobwebs.

This is not going to be fixed for python3.5 and 3.6 on Windows, and I don't have any workaround.

davidbrochart · 2020-03-17T14:47:22Z

@echarles - yes, that's correct. We'll want to get that client implementation up on async. I'm fairly certain @davidbrochart is aware of that: jupyter/jupyter_client#428 (comment)

I don't think there is anything to do in jupyter_kernel_mgmt, it already has an async client (it's IOLoopKernelClient).

kevin-bates · 2020-03-17T14:58:00Z

By golly - I didn't realize that. I knew it required changes to interact with the async kernel manager, but I never looked closely at the pure 'client' portion. If you say its good to go, then I'm good.

Looking just now, it doesn't appear to have a mixture of async/await and gen.coroutine/yield and it seems like we should convert the latter at some point.

Zsailer · 2020-03-17T16:48:47Z

This is not going to be fixed for python3.5 and 3.6 on Windows, and I don't have any workaround.

So what are our options here?

Require Python 3.6>= on Windows and keep 3.5 support on other platforms.
Or bump jupyter_server to only support Python 3.6 on all platforms.

Is that right?

And if we choose the former, we should probably add a table to the README with "supported" Python versions+platforms.

davidbrochart · 2020-03-17T16:52:44Z

We even need python>=3.7 for Windows. I would go for option 1.

Zsailer · 2020-03-17T17:00:32Z

jupyter_server/utils.py

+def ensure_async(obj):
+    """Convert a non-async object to a coroutine if needed.
    """
-    if inspect.isawaitable(obj):
-        return asyncio.ensure_future(obj)
-    elif isinstance(obj, concurrent.futures.Future):
-        return asyncio.wrap_future(obj)
-    else:
-        # not awaitable, wrap scalar in future
-        f = asyncio.Future()
-        f.set_result(obj)
-        return f
+    if asyncio.iscoroutine(obj):
+        return obj
+    if type(obj) is asyncio.Future:
+        return obj
+
+    async def _():
+        return obj

+    return _()


👍

It might be worth one more set of eyes to specifically approve this change (maybe @kevin-bates).

It LGTM, but I'm not super familiar with the async stuff. I want to make sure we're not missing any key edge cases that might lead to cryptic error messaging or bugs. Should we create a set of new tests specifically for this function?

Yes, async is not easy, so any help would be appreciated here, including ways to test this function.

Zsailer · 2020-03-17T17:09:26Z

jupyter_server/serverapp.py

@@ -1039,7 +1039,7 @@ def template_file_path(self):
    )

    kernel_manager_class = Type(
-        default_value=MappingKernelManager,
+        default_value=AsyncMappingKernelManager,


Since this changes the default manager class name, I think we need to add logic that addresses user config. If any traits of MappingKernelManager are configurable, some users may have config that will no longer be loaded by this new class, and thus, silently ignore their settings.

I see two ways to address this:

If those traits are still valid for this new class, we can write an empty configurable MappingKernelManager class that AsyncMappingKernelManager inherits and thus loads the traits as a subclass.

Throw warnings every time a MappingKernelManager trait pops up in the config.

Thanks for pointing that out @Zsailer. I think option 1 is the best.

Zsailer · 2020-03-17T17:12:08Z

We even need python>=3.7 for Windows. I would go for option 1.

Why are the 3.6 Windows tests passing, but not the 3.5?

kevin-bates · 2020-03-17T17:13:18Z

What I'm not understanding is why JKM is working on WIndows 3.5, 3.6? It's generally the same code since we can't use the async subprocess stuff on Windows there either.

davidbrochart · 2020-03-17T17:17:13Z

The tests randomly pass on Windows/python3.6, and they seem to always fail with python3.5. This issue says that the fix has been back-ported to python 3.7 only, not beyond.
Since the behavior is random, it might explain why we don't see it in JKM, but I cannot be sure.

kevin-bates · 2020-04-01T17:41:34Z

I believe this is ready to merge. I can't recall what option we should use. Deferring to @Zsailer.

Zsailer · 2020-04-01T21:28:58Z

I think the history of this PR is useful to keep around—so I'm going to merge directly (not squash-and-merge). This was a lot of work that's work tracking. Thanks @kevin-bates and @davidbrochart! Great work, here!

kevin-bates · 2020-04-01T22:30:42Z

I hate to bring this up, but in working on the NB 4479 PR, I'm finding the checks for Python 3.5 should be sys.version_info < (3, 6) rather than sys.version_info <= (3, 5). I'm running on 3.5.5 and it's not detecting "3.5" correctly.

davidbrochart · 2020-04-02T05:38:57Z

Good catch Kevin, thanks!
Zach, I think you should squash, we can always refer to this PR even though it's closed (and it has all the useful narrative too).

kevin-bates

Thanks David. I think we're getting very close here.

kevin-bates · 2020-04-02T14:08:07Z

jupyter_server/gateway/managers.py



-class GatewayKernelManager(MappingKernelManager):
+class GatewayKernelManager(AsyncMappingKernelManager):


This needs to be changed back to MappingKernelManager, otherwise we've disabled the use of a gateway on 3.5. As mentioned in the previous comment, the superclass is not critical since GatewayKernelManager serves as a proxy.

tests/test_gateway.py

tests/services/kernels/test_api.py

…n3.5 compatibility

davidbrochart · 2020-04-02T15:21:47Z

Thanks again Kevin!
I'm just adding back the removal of kernelmanager.AsyncMappingKernelManager in the test to check, I remember that it was needed but I don't know why.

kevin-bates · 2020-04-02T15:41:58Z

Hmm. That condition is impossible, by definition, so how can it be considered valid for the test? I think it might be good to understand what's going on. The 3.5 no-async-kernel-manager test is failing because the trailet for the kernel_manager_class can't be resolved...

E               traitlets.traitlets.TraitError: The 'kernel_manager_class' trait of <jupyter_server.serverapp.ServerApp object at 0x7f997c5b7b70> instance must be a type, but 'jupyter_server.services.kernels.kernelmanager.AsyncMappingKernelManager' could not be imported

and this is impossible in the real world because AsyncMappingKernelManager will always exist.

davidbrochart · 2020-04-02T15:45:04Z

I need to take a break and look more closely.

kevin-bates · 2020-04-02T15:48:13Z

tests/services/kernels/test_api.py

@@ -284,7 +285,7 @@ def no_async_mapping_kernel_manager(monkeypatch):

 @pytest.mark.skipif(
    sys.version_info < (3, 6),
-    reason="Kernel manager is AsyncMappingKernelManager, Python version < 3.6"
+    reason="Testing no AsyncMappingKernelManager on Python >=3.6


Need trailing quotes.

kevin-bates

Looks great David - thank you!

Zsailer · 2020-04-04T17:13:41Z

Congrats, y'all! Great stuff!

* Use jupyter_client's AsyncKernelManager * Rename MappingKernelManage to AsyncMappingKernelManage, convert gen.coroutine/yield to async/await, remove run_blocking * Fix Windows subprocess handle issue * Restrict Windows to python>=3.7 * Fix GH actions matrix exclusion * Make AsyncMappingKernelManager a subclass of MappingKernelManager for configuration back-compatibility * Make AsyncKernelManager an opt-in * Pin jupyter_core and jupyter_client a bit higher * Remove async from MappingKernelManager.shutdown_kernel * Hard-code super() in MappingKernelManager and AsyncMappingKernelManager * Add argv fixture to enable MappingKernelManager and AsyncMappingKernelManager * Rewrite ensure_async to not await already awaited coroutines * Add async shutdown_kernel to AsyncMappingKernelManager, keep MappingKernelManager.shutdown_kernel blocking * Add restart kwarg to shutdown_kernel * Add log message when starting (async) kernel manager * Bump jupyter_client 6.1.1 * Rename super attribute to pinned_superclass * Prevent using AsyncMappingKernelManager on python<=3.5 (at run-time and in tests) * Ignore last_activity and execution_state when comparing sessions * Replace newsession with new_session * Fix Python version check * Fix skipping of tests * GatewayKernelManager inherits from MappingKernelManager to keep python3.5 compatibility * Added back removal of kernelmanager.AsyncMappingKernelManager * Don't test absence of AsyncMultiKernelManager

davidbrochart mentioned this pull request Mar 16, 2020

New release jupyter/jupyter_client#532

Closed

kevin-bates reviewed Mar 16, 2020

View reviewed changes

davidbrochart force-pushed the AsyncKernelManager branch from 93180d3 to 88b7e15 Compare March 16, 2020 15:25

davidbrochart closed this Mar 16, 2020

davidbrochart reopened this Mar 16, 2020

kevin-bates approved these changes Mar 16, 2020

View reviewed changes

davidbrochart added 2 commits March 16, 2020 18:17

Use jupyter_client's AsyncKernelManager

5cba6d8

Rename MappingKernelManage to AsyncMappingKernelManage, convert gen.c…

104b9f1

…oroutine/yield to async/await, remove run_blocking

davidbrochart force-pushed the AsyncKernelManager branch from 88b7e15 to b471f27 Compare March 16, 2020 17:34

Fix Windows subprocess handle issue

cc55f28

davidbrochart force-pushed the AsyncKernelManager branch from b471f27 to cc55f28 Compare March 17, 2020 09:05

Zsailer reviewed Mar 17, 2020

View reviewed changes

davidbrochart added 2 commits March 17, 2020 18:44

Restrict Windows to python>=3.7

c659e16

Fix GH actions matrix exclusion

657ab1e

Fix Python version check

93a0c7a

davidbrochart added 3 commits April 2, 2020 07:48

Skip gateway tests if python<3.6

54656de

Fix skipping of tests

11f3ccf

Again

6691718

kevin-bates reviewed Apr 2, 2020

View reviewed changes

davidbrochart added 2 commits April 2, 2020 17:16

GatewayKernelManager inherits from MappingKernelManager to keep pytho…

1a6fe32

…n3.5 compatibility

Added back removal of kernelmanager.AsyncMappingKernelManager

40d017e

davidbrochart added 3 commits April 2, 2020 17:29

-

ba4dd16

-

d5d0a0d

:-)

16a7835

:-[

530b6ce

kevin-bates reviewed Apr 2, 2020

View reviewed changes

Don't test absence of AsyncMultiKernelManager

082075a

kevin-bates approved these changes Apr 2, 2020

View reviewed changes

kevin-bates merged commit d1cb381 into jupyter-server:master Apr 4, 2020

davidbrochart deleted the AsyncKernelManager branch April 4, 2020 06:27

This was referenced Apr 15, 2020

Adopt native coroutines #93

Closed

Enable ServerApp to discover ExtensionApps (and their config). #180

Merged

davidbrochart mentioned this pull request May 1, 2020

Python version per platform support for extension authors #216

Closed

davidbrochart mentioned this pull request May 27, 2020

Master test failure jupyter/nbclient#73

Closed

Zsailer pushed a commit to Zsailer/jupyter_server that referenced this pull request Nov 18, 2022

add version info and fix launch local kernel (jupyter-server#191)

85cd04b



		class GatewayKernelManager(MappingKernelManager):
		class GatewayKernelManager(AsyncMappingKernelManager):

Use jupyter_client's AsyncKernelManager #191

Use jupyter_client's AsyncKernelManager #191

Conversation

davidbrochart commented Mar 16, 2020

kevin-bates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zsailer commented Mar 16, 2020

davidbrochart commented Mar 16, 2020

davidbrochart commented Mar 16, 2020

davidbrochart commented Mar 16, 2020

kevin-bates left a comment

Choose a reason for hiding this comment

kevin-bates commented Mar 16, 2020

echarles commented Mar 16, 2020

kevin-bates commented Mar 16, 2020

davidbrochart commented Mar 17, 2020

davidbrochart commented Mar 17, 2020

kevin-bates commented Mar 17, 2020

Zsailer commented Mar 17, 2020

davidbrochart commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zsailer Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zsailer commented Mar 17, 2020

kevin-bates commented Mar 17, 2020

davidbrochart commented Mar 17, 2020

kevin-bates commented Apr 1, 2020

Zsailer commented Apr 1, 2020

kevin-bates commented Apr 1, 2020

davidbrochart commented Apr 2, 2020

kevin-bates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbrochart commented Apr 2, 2020 • edited Loading

kevin-bates commented Apr 2, 2020

davidbrochart commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-bates left a comment

Choose a reason for hiding this comment

Zsailer commented Apr 4, 2020

Zsailer Mar 17, 2020 •

edited

Loading

davidbrochart commented Apr 2, 2020 •

edited

Loading