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

get_all_specs not compatible with subclassing #338

Closed
minrk opened this issue Jan 23, 2018 · 8 comments · Fixed by #339
Milestone

Comments

@minrk
Copy link
Member

@minrk minrk commented Jan 23, 2018

nb_conda_kernels subclasses KernelSpecManager to find conda kernels. It does so by overriding find_kernel_specs to return its name list and resource directories, and get_kernel_spec to return the specs themselves.

We've added .get_all_specs as a simpler API to get all of the specs without separately finding and loading them. But we've done this in such a way that .get_all_specs calls .find_kernel_specs, a public API, and _get_kernel_spec_by_name, a private method. This means that implementations that override .find_kernel_specs in such a way that _get_kernel_spec_by_name won't work, then .get_all_specs is broken for any such subclass (this includes nb_conda_kernels 2.1).

Now, one option is for custom kernelspec managers to implement all three methods - find_kernel_specs, get_all_specs, and get_kernel_spec, but this is a backward-incompatible change, and I think we should consider it a bug.

I think we should ensure that .get_all_specs works for KSM subclasses that have only implemented the public methods .find_kernel_specs and .get_kernel_spec to fix this backward-incompatibility, and push that out in a bugfix release.

cc @dartdog who reported the issue

@michaelaye

This comment has been minimized.

Copy link

@michaelaye michaelaye commented Jan 23, 2018

I'm puzzled why things break with conda 4.4.x while they still work with 4.3.x? Where's the conda relationship here?

@minrk

This comment has been minimized.

Copy link
Member Author

@minrk minrk commented Jan 24, 2018

@michaelaye shouldn't be a sensitivity to conda version. The relevant version change is notebook 5.3 which started using the get_all_specs method, revealing this bug.

@escorciav

This comment has been minimized.

Copy link

@escorciav escorciav commented Apr 25, 2018

Is this still a problem with jupyter-notebook==5.4.1?
I am using nb_conda_kernels==2.1.0 from conda-forge but I can't find another environment.
I double check that ipykernel is in the other environment.

@takluyver

This comment has been minimized.

Copy link
Member

@takluyver takluyver commented Apr 25, 2018

@escorciav you need to make sure you have jupyter_client version 5.2.2 or above.

@escorciav

This comment has been minimized.

Copy link

@escorciav escorciav commented Apr 25, 2018

thanks for the heads-up @takluyver
My conda environment reports jupyter_client 5.2.3 py36_0. Any other clue?
I'm using conda 4.5.1.

@takluyver

This comment has been minimized.

Copy link
Member

@takluyver takluyver commented Apr 25, 2018

If you have multiple envs, check which one the notebook server is running from, and the version of jupyter_client in there. If that's the newest version, then it's not this bug - try asking nb_conda_kernels about it.

@escorciav

This comment has been minimized.

Copy link

@escorciav escorciav commented Apr 25, 2018

Thanks. Everything seems fine in terms of package version. Although, the default kernel is called python3 rather than the name of the environment.

  • Are you using nb_conda_kernels?
    If yes, could you tell me the version of jupyter_client, jupyter_lab, nb_conda_kernels and conda that u have?
@takluyver

This comment has been minimized.

Copy link
Member

@takluyver takluyver commented Apr 25, 2018

I'm not using nb_conda_kernels. I confess I don't actually use environments very much - I tend to install everything with --user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.