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

Why is the kernel client class pinned to AsyncKernelClient? #116

Closed
kevin-bates opened this issue Oct 10, 2020 · 8 comments
Closed

Why is the kernel client class pinned to AsyncKernelClient? #116

kevin-bates opened this issue Oct 10, 2020 · 8 comments

Comments

@kevin-bates
Copy link
Member

Hello - great project folks!

I'm trying to bring my own kernel manager class for running a notebook through papermill which needs a specific kernel client class. However, nbclient is forcing the use of AsyncKernelClient despite my kernel manager specifying that a different client class be used. I suspect this is to trigger the use of AsyncKernelClient even when the synchronous jupyter_client KernelManager class is used but would like to get a better understanding.

If this is the reason, it seems like this forced override should be conveyed via a configuration option so that custom kernel client classes can be used. In reading through #10, I didn't find any discussion of this particular line of code - which I also found surprising since the kernel manager class is already responsible for determining its corresponding kernel client class.

I'm happy to submit a PR that introduces a boolean to trigger this override - or something to that effect - so long as there's an ability to use the desired kernel client class specified by the kernel manager. Thanks.

@davidbrochart
Copy link
Member

Hi Kevin,
If you want to use your own kernel manager, you can pass it to the NotebookClient. Have you tried that?
Otherwise, nbclient having inherently an async execution model, it will indeed set up an async kernel client.

@kevin-bates
Copy link
Member Author

Hi David - thanks for the quick response.

Because I'm really going through Papermill here, I am specifying kernel_manager_class and need my NotebookClient subclass to instantiate the specified kernel manager instance and its specific (async) kernel client. I do not want to have to instantiate an instance first.

So is the reason for this forced override purely to ensure an async kernel client is used even in the classic case where a synchronous kernel manager is used? That seems to be the case since the default value for the kernel_manager_class trait is already AsyncKernelManager and it already specifies AsyncKernelClient as its client.

If nbclient requires an async kernel client (and a subclass of AsyncKernelClient for that matter), it seems it should only set the client_class trait on the KernelManager if it currently specifies juptyer_client.client.KernelClient and not unconditionally. Would that be sufficient?

if self.km.client_class == 'jupyter_client.client.KernelClient':
    self.km.client_class = 'jupyter_client.asynchronous.AsyncKernelClient'

This way, other kernel managers can bring their own async kernel client implementations.
(I get the expected results in my implementation after making this change.)

@davidbrochart
Copy link
Member

Would that be sufficient?

if self.km.client_class == 'jupyter_client.client.KernelClient':
    self.km.client_class = 'jupyter_client.asynchronous.AsyncKernelClient'

Yes, this looks good to me. Thanks for looking into it Kevin!
If you feel like opening a PR, I'll be glad to merge it. Otherwise I can make the change.

@MSeal
Copy link
Contributor

MSeal commented Oct 10, 2020

It might be nice for KernelManagers to be able to optionally supply a suggested KernelClient class so we could let a kernel manager assignment alone apply sane default client associations when the user isn't sure.

@davidbrochart
Copy link
Member

Isn't it why we have client_class in the kernel managers?

@MSeal
Copy link
Contributor

MSeal commented Oct 10, 2020

Yes. That's what I get for a quick response from my phone without thinking. Apologies and ignore that comment.

@davidbrochart
Copy link
Member

No worries Matthew! My question was really for me to check if I understood correctly 😃

@kevin-bates
Copy link
Member Author

Addressed by #117

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

No branches or pull requests

3 participants