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

Implement a conda_kernel_provider #32

Closed
echarles opened this issue Nov 9, 2019 · 13 comments
Closed

Implement a conda_kernel_provider #32

echarles opened this issue Nov 9, 2019 · 13 comments

Comments

@echarles
Copy link
Contributor

echarles commented Nov 9, 2019

Opening this to discuss how we can contribute the current nb_conda_kernel to be upgraded as a provider for kernel_mgmt.

cc/ @Zsailer @kevin-bates

@kevin-bates
Copy link
Collaborator

I've looked into nb_conda_kernels and found they already have an implementation of CondaKernelProvider (good news). The bad news is that they based its implementation on code that was placed into jupyter_client relative to an old design (uses make_manager() rather than launch()) and prior to the split of client into jupyter_kernel_mgmt and jupyter_protocol. This lead me to create this deprecation issue yesterday. Fortunately, there's no jupyter_client release that includes the discovery module, and we need to ensure that remains the case.

Once we have 0.5 out and we know we're moving forward with kernel providers, we will want to work with the conda folks to develop a provider. Given the conda folks jumped on as early adopters is a great sign in my opinion.

In the spirit of transparency, I'm copying @mcg1969 here since he implemented the CondaKernelProvider in the first place. Michael, please let us know if you have any questions. I'm hoping this could get resolved relatively easily once the appropriate pieces are in place.

@mcg1969
Copy link

mcg1969 commented Nov 9, 2019

Thanks for the ping. I would absolutely love to be at the front of the line on this one. I haven't looked at the code in a little bit, nor have I been watching these developments. I am more than happy to accept PRs, or clear direction on how to do it myself.

@echarles
Copy link
Contributor Author

echarles commented Nov 9, 2019

@mcg1969 I am more a supporter of @takluyver and @kevin-bates work than a real active/expert developer, but I will be in Austin the first week of December for the JupyterLab in-person dev meeting. @Zsailer will also be there to work on jupyter_server and @kevin-bates could join via conf-call. Would it make sense for you to join a few hours (in place or remote) to review together the needed changes"?

@mcg1969
Copy link

mcg1969 commented Nov 9, 2019

If the team feels that would be useful part of the agenda, by all means! It would certainly be convenient. But I will leave the decision to y'all. I'm not interested in stepping on tors and this does seem like something readily handled over GitHub

@kevin-bates
Copy link
Collaborator

Yeah, I think this will be fairly straightforward. All that is necessary, besides pointing to jupyter_kernel_mgmt, is to bring find_kernels() up to date relative to @asyncio.coroutine, then replace make_manager() with an implementation of launch() that will (very likely) resemble this:

    async def launch(self, name, cwd=None, launch_params=None):
        spec = self.cksm.get_kernel_spec(name)
        return await SubprocessKernelLauncher(
            kernel_cmd=spec.argv, extra_env=spec.env, cwd=cwd, launch_params=launch_params).launch()

That said, I would recommend this effort be differed until 0.5.0 is available.

Should Anaconda decide they want to implement their own launch, they're free to do so whenever.

One thing that might be worth considering is the current conda- prefix may be redundant when the provider id of conda is used. Not sure if you'll want to strip that off if the kernelspec is being found via kernel providers. The plan is to have the application (Lab/Notebook/etc) display/convey the provider id in some form - although this won't include the env name, but that's already part of the DisplayName - correct?

@takluyver
Copy link
Owner

My thinking is that the UIs should display the kernel type ID, including both the provider ID and the name that kernel knows for the provider, so you'd see something like:

  • Python (spec/python3)
  • Python (conda/env-foo-python)

So I would recommend that they strip the extra conda- prefix off the kernel name when it's used as part of the provider system. But obviously it's up to providers what naming convention they use for their kernels - that's the aim of namespacing them.

@mcg1969
Copy link

mcg1969 commented Nov 12, 2019

I agree with the recommendation. It shouldn't be too difficult for nb_conda_kernels to offer a different naming convention for the kernel provider mechanism.

@echarles echarles changed the title [DISCUSS] conda_kernel_provider Implement a conda_kernel_provider Dec 1, 2019
@echarles
Copy link
Contributor Author

echarles commented Dec 1, 2019

Just renamed the title to reflect that we can go to an implementation based on the kernel management solution as described in https://github.com/takluyver/jupyter_kernel_mgmt

@takluyver
Copy link
Owner

Though, to be clear, I'm still expecting that the implementation will be maintained separately, not as part of this repository.

@echarles
Copy link
Contributor Author

echarles commented Dec 1, 2019

Sure. I guess it will be inside https://github.com/Anaconda-Platform/nb_conda_kernels but as for now we don't have open issue/pr, I was thinking to use this open issue to further comment on any progress.

@mcg1969 Feel free to jump online or in-person this coming week at the jupyterlab dev meeting in Austin.

@kevin-bates
Copy link
Collaborator

I plan on helping with this (in the nb-conda-kernels repo), along with progressing jupyter_server with JKM, once 0.5 is available.

@mcg1969
Copy link

mcg1969 commented Dec 1, 2019

Thanks to everyone! I will do what I can to move this forward as well. I'm out of pocket right now but when I get to my desk I'll create a formal issue.

@echarles
Copy link
Contributor Author

echarles commented Dec 2, 2019

Closing in favor of anaconda/nb_conda_kernels#151

@echarles echarles closed this as completed Dec 2, 2019
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