Inconsistency in the KernelManager.get_kernel API implementations

[declanvk]

I wasn't sure where the best place for this was, given that this issue touches on jupyter_client, jupyter_server, and voila. jupyter_server seemed best as a middle point.

Description

There is an inconsistency between different implementations of the KernelManager interface, specifically in the output of the get_kernel method.

In the GatewayKernelManager implementation, the get_kernel method returns a dictionary that is parsed from the response from the remote gateway:

jupyter_server/jupyter_server/gateway/managers.py

Lines 412 to 440 in 74801f4

	async def get_kernel(self, kernel_id=None, **kwargs):
	"""Get kernel for kernel_id.
	Parameters
	----------
	kernel_id : uuid
	The uuid of the kernel.
	"""
	kernel_url = self._get_kernel_endpoint_url(kernel_id)
	self.log.debug(f"Request kernel at: {kernel_url}")
	try:
	response = await gateway_request(kernel_url, method='GET')
	except web.HTTPError as error:
	if error.status_code == 404:
	self.log.warn(f"Kernel not found at: {kernel_url}")
	self.remove_kernel(kernel_id)
	kernel = None
	else:
	raise
	else:
	kernel = json_decode(response.body)
	# Only update our models if we already know about this kernel
	if kernel_id in self._kernels:
	self._kernels[kernel_id] = kernel
	self.log.debug(f"Kernel retrieved: {kernel}")
	else:
	self.log.warning(f"Kernel '{kernel_id}' is not managed by this instance.")
	kernel = None
	return kernel

In the MultiKernelManager implementation, the get_kernel method returns an object from a lookup table that it maintains:

https://github.com/jupyter/jupyter_client/blob/master/jupyter_client/multikernelmanager.py#L321-L330

The object in that lookup table is created as part of the pre_start_kernel method, from a specified factory:

https://github.com/jupyter/jupyter_client/blob/50dff2e809cf7b67128e7e2f0cb422e751011ef9/jupyter_client/multikernelmanager.py#L156-L175

Expected Behavior

I was interested in supporting Voila with a custom KernelManager implementation that is very similar to the GatewayKernelManager, but ran into this issue when Voila was attempting to use the value returned by get_kernel in ways that I had not accounted for. Specifically, the code that it was trying to run:

https://github.com/voila-dashboards/voila/blob/ae3d025dda21f72c2a663d04cf4ff10dc9215f08/voila/handler.py#L177-L181

where it is using methods that are not present on a simple dictionary.

Depending on how the maintainers feel about this, either the GatewayKernelManager should have some changes to bring it in line with the other KernelManager implementations, or I could ask the voila maintainers if they would accept a solution that detects the return type and then use different implementations for the dict vs object result.

[davidbrochart]

We can make get_kernel(kernel_id) async in jupyter_client's AsyncMultiKernelManager, as usually we try to support the most general use case there. Note that I am about to merge a big refactor in jupyter/jupyter_client#623, so I could make this change part of the PR.

[declanvk]

Thanks @davidbrochart, I think that would help with the consistency issue!

[kevin-bates]

I agree there's an inconsistency here - especially with the GatewayKernelManager. The normal result of calling MappingKernelManager.get_kernel(kernel_id) should be a KernelManager instance corresponding to the given kernel id.

In the case of the GatewayKernelManager, when the user explicitly intends to have their kernels managed remotely, the only consumer of GatewayKernelManager should be the kernel handlers where, in the case of get_kernel(kernel_id), a JSON model is returned.

If we really needed to, we could fabricate a KernelManager instance that essentially does the same thing as the GatewayKernelManager (which should probably be named GatewayMappingKernelManager to convey that it really manages all the KM instances). The issue is with the client property that reflects the KernelClient instance. That too would need to route requests to the remote server. (FYI, both of these classes are implemented in Elyra so we could use Papermill against a gateway server. If you find them suitable, we could probably look into splitting them out.)

To be honest, GKM was never intended to be used outside of the handlers. If we really (really) see the need to bring this in line with how normal KMs are used today, then I think we'd be looking at using the likes of the HTTPKernel{Manager,Client} classes within the gateway package - which has yet to show a need for that.

[declanvk]

@kevin-bates if we were to extract the HTTPKernelManager, where could we put those classes? Could jupyter_server take a dependency on Elyra? Also just to confirm, the HTTPKernelManager would be a manager for a single kernel, while we could keep the GatewayKernelManager as the mapping kernel manager?

The classes in Elyra look good to me.

[ellisonbg]

A few questions/comments:

• Is there a single, well-defined set of interfaces in in jupyter-server and the notebook server for this?
• If so, isn't this just a matter of allowing multiple implementations of those interfaces?
• If not, it seems like the first step is to create those interfaces in both jupyter-server and the notebook server. It would be great if that didn't involve breaking changes, but that might not be possible. I think it is critical that the interfaces and their default implementations are in jupyter-server, but fine with alternative implementations being in other repos/orgs.
• Can Voila and other consumers of the existing APIs write code to work around the different interfaces? Would an adapter pattern help here?

[kevin-bates]

Thanks for the questions @ellisonbg.

Is there a single, well-defined set of interfaces in in jupyter-server and the notebook server for this?

The KernelManager and KernelClient interfaces are fairly well-defined in jupyter_client with instances of the former collected via the MultiKernelManager. Notebook & Server do leverage "Python" to tack on attributes (namely for activity monitoring) that probably should have been done via subclassing - but I think the idea may have been to allow for custom subclasses to operate against both jupyter_client (directly) and via notebook server and that wouldn't have been possible if NB introduced its own subclass. However, because we do permit subclassing, that water is now under the bridge (i.e., introducing such a subclass in NB/Server would break existing subclasses).

It's unfortunate that the get_kernel(kernel_id) method of GatewayKernelManager (that is a subclass of MappingKernelManager and thus MultiKernelManager) returns a kernel model rather than a KernelManager instance, but that's because it has always been intended to be used via the REST APIs - not via other server extensions - since it, itself originated as a server extension.

I'm hoping we can bring the GKM more in line with the MKM interfaces, which I think makes sense, but then would require a KernelManager implementation and, with KernelManager comes KernelClient, etc.

Can Voila and other consumers of the existing APIs write code to work around the different interfaces? Would an adapter pattern help here?

Yes, an adapter pattern could be something to use as a contingency should b/c support become a concern but hopefully unnecessary.

@declanvk - I had been writing a response to your comment when Brian's came in, but feel I still owe that to you. I think it's "in tune" with my response above, but may require we wait for the dust to settle.

if we were to extract the HTTPKernelManager, where could we put those classes?

I think we'd place these into the gateway package and need to refactor the existing GatewayKernelManager much like the MappingKernelManager where it essentially pushes ops down to HTTPKernelManager.

Could jupyter_server take a dependency on Elyra?

No. Elyra already has a dependency on jupyter_server and all that we need to do is move the HKM - which Elyra can then make accessible to Papermill/nbclient.

Also just to confirm, the HTTPKernelManager would be a manager for a single kernel, while we could keep the GatewayKernelManager as the mapping kernel manager?

Yes - although the "manager" work would move to the HTTPKernelManager (as noted). The kernel client functionality would only come into play for instances like this (from Viola) while message interaction with a "gateway kernel" from browser-based apps (e.g., lab) would still go through the gateway package's handler.

Also, note that HTTPKernelManager was implemented solely with Papermill in mind. As a result, manager operations like interrupt and restart have not been exercised like they would need to be when exposing full, interactive, functionality: caveat emptor. In addition, I don't know if Voila makes any assumptions about the kernel's locality.

If this is something you'd still like to pursue, perhaps I could get a branch going and we could work out any kinks there - keeping in mind the end goal is a general-purpose kernel management alternative that essentially routes to a Gateway server.

[kevin-bates]

I've created a branch in my fork (https://github.com/kevin-bates/jupyter_server/tree/refactor-gateway-km) that uses HTTPKernelManager (renamed to GatewayKernelManager with the original renamed to GatewayKernelManagers (sigh)) to coordinate with the Gateway server. Since the front end goes through the socket handler directly, GatewayKernelClient (renamed from HTTPKernelClient) doesn't come into play in that interaction.

I tried running Voila but could not (quickly) figure out how to make it run using Jupyter server or add options like --gateway-url to force the use the KM and KC classes. @declanvk - could you take this branch for a spin relative to Voila and see where we're at with things? I really don't want to make any guarantees with this.

Also, note that I couldn't get the kernel client stuff to go through the handlers (this pick was busted back when it was originally implemented in Elyra) and so a dependency on websocket-client has been added which may cause heartburn for minimalists. If someone can figure out how that could be removed by leveraging the handlers, that would be great.

[declanvk]

I'll try to take a look at this sometime this week, thank you for starting this branch.

[kevin-bates]

Hi @declanvk. I'd like to submit a PR for this given the refactoring doesn't affect the current behavior and I've confirmed that the Elyra server extension can successfully use the refactoring in place of the aforementioned HTTPKernelManager and HTTPKernelClient implementations (on which these changes are based).

I think with these changes basic "nbclient" functionality can take place (e.g., like that performed by papermill). Where I suspect things could get dicey is if the server extension were to use KernelManager ports directly or make locality assumptions. I believe we can cross that bridge at that time rather than try to cover all scenarios before initial exposure.

I suspect this might make your investigations easier as well.