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

Having trouble running nbclient 0.3.0 #56

Closed
golf-player opened this issue May 16, 2020 · 8 comments
Closed

Having trouble running nbclient 0.3.0 #56

golf-player opened this issue May 16, 2020 · 8 comments

Comments

@golf-player
Copy link
Contributor

golf-player commented May 16, 2020

Apologies if I'm doing something dumb. I've just started trying to play around with 0.3.0, and I'm running into some issues.

Context: I'm trying to get enterprise gateway's RemoteKernelManager to work on its own as a KernelManager (jupyter-server/enterprise_gateway#803). This is a sync KernelManager, so I'm guessing that's why it's not working as expected.

import nbformat
from nbclient import NotebookClient
from enterprise_gateway.services.kernels.remotemanager import RemoteKernelManager

with open("/home/ish/notebooks/Untitled.ipynb") as fp:
    test_notebook = nbformat.read(fp, as_version=4)

client = NotebookClient(nb=test_notebook, kernel_manager_class=RemoteKernelManager)
client.execute()

The above code works as expected wth 0.2.0, but there's a lot of strange behaviour in 0.3.0.

It also works with 0.1.0, but doesn't shutdown. I don't see shutdown_kernel called anywhere besides on client startup failure, so I guess that's to be expected? Although the docstring does say the kernel gets shut down.

  • When I run the above, the kernel executes as expected, but is never shut down. Still looking into why this is the case (is this intended? possibly an issue on my end)
  • When I run the above, but with reset_kc=True, I run into a lot of issues, first of which is that in execute, self.kc has not been initialized from the class before self._async_cleanup_kernel is called, so that fails and while it's handling that failure, it fails again on self.km.cleanup() since self.km doesn't exist.

I am using an unusual KernelManager, but I don't think that's the cause of these issues.

Couple other things I noticed; the changelog link is broken, and there's a typo in the docstring for NotebookClient (Kernel spelled Kernerl)

@golf-player
Copy link
Contributor Author

CC: @kevin-bates

@kevin-bates
Copy link
Member

Hi @golf-player - thanks for the copy. I haven't used nbclient but have experienced odd issues when mixing @gen.coroutine/yield methods with those using async/await. Are you seeing any 'method not awaited' messages or anything of that ilk buried in the logs?

Once NB 6.1 is finalized, I'll be merging the async RMK in EG. I'd like to get your changes merged prior to that, so I wonder if holding off looking into nbclient 0.3.0 would be worthwhile until the async RMK is, um, "out of the rough and in the fairway". (sorry, couldn't resist)

@golf-player
Copy link
Contributor Author

golf-player commented May 16, 2020

In this case, I don't see the issues as being specific to async vs sync. These are attribute errors, and the kernel manager hasn't even been created yet.

As for the cases where things don't shutdown, that appears to be intentional.

Agreed, it would make sense to not worry about 0.3.0 as things work as expected in 0.2.0

@davidbrochart
Copy link
Member

When I run the above, but with reset_kc=True, I run into a lot of issues, first of which is that in execute, self.kc has not been initialized from the class before self._async_cleanup_kernel is called, so that fails and while it's handling that failure, it fails again on self.km.cleanup() since self.km doesn't exist.

We should probably check if self.kc is not None in _async_cleanup_kernel, in case self.km doesn't exist.

@golf-player
Copy link
Contributor Author

golf-player commented May 19, 2020

@davidbrochart thanks for the reply. Agreed, that's probably the way to go.

If y'all are willing, I can make a pull request changing that

@davidbrochart
Copy link
Member

That would be great!

@golf-player
Copy link
Contributor Author

golf-player commented May 19, 2020

#61

not much to it, tbh.

@choldgraf
Copy link
Contributor

many thanks for the patch @golf-player 👍

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

4 participants