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

Set jupyter-client min version needed by Python3 mode launching Jupyter Kernel #1517

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carlosperate
Copy link
Member

In PR #1240 we started using KernelManager.pre_start_kernel() via inheritance from the jupyter_clientpackage, which was only introduced in v6.1.0:

This PR configures this version as the minimum required by Mu.

However, taking in consideration this version is less than a year old, and people might be installing Mu using pip from their system python packages (#1516), it would be better if we could update Mu to be compatible with older versions of jupyter_client, as otherwise people might inadvertently upgrade their Jupyter dependencies to what could potentially be a breaking change for them.

@devdanzin can we avoid the use of KernelManager.pre_start_kernel() from PR #1240? Is it necessary to run? Or could other method like start_kernel() be enough?

I didn't see this documented in https://jupyter-client.readthedocs.io/en/latest/api/manager.html#jupyter_client.KernelManager so maybe is not an API we should be using?

@ntoll
Copy link
Member

ntoll commented Jun 19, 2021

Hi @carlosperate @devdanzin - any updates on this branch. Reading the commentary by Carlos I, too, wonder about our use of pre_start_kernel vs just start_kernel. I wouldn't feel comfortable merging this (draft) PR until we have a good / complete story to tell about this. 👍

@carlosperate
Copy link
Member Author

I haven't looked at the jupyter API at all as I have zero experience using the jupyter ecosystem, was hoping @devdanzin had some thoughts, but looks like he is offline since February (I hope he is okay).
@ntoll @tjguk do you have any experience with this? If not I can give it a go, but there are other micropython device issues I might want to look into first.

@carlosperate carlosperate mentioned this pull request Aug 30, 2021
@ntoll
Copy link
Member

ntoll commented Oct 3, 2021

@carlosperate I'll try looking at this before beta 7.

@carlosperate carlosperate modified the milestones: 1.1.0-beta.6, 1.1.0-rc.1 Oct 6, 2021
@devdanzin
Copy link
Contributor

Hey guys, sorry for going AWOL. I'll set a development environment up, investigate and report back.

As far as I remember, I only used pre_start_kernel because it was the most direct way to get the interpreter being selected (diagnosing the issue in the logs). Maybe we can simply pass the correct interpreter to start_kernel instead.

@devdanzin
Copy link
Contributor

There's a way that removes the need to both call pre_start_kernel and subclass QtKernelManager.

It requires creating a Kernel Spec on disk, which is a little JSON file pointing to the right interpreter, and then installing it via jupyter_client.kernelspec.KernelSpecManager.

It feels a little brittle, because there are a lot more moving parts (and opportunities for virus scanners to get in the way), but the simple, elegant alternative was sadly deprecated and ultimately removed.

I have it currently implemented to reduce wait time: if there's already a kernel with the same name we'd use, we skip creating the spec. But that might persist issues (maybe if users use two different Python versions?) and always creating the file might be better.

The rejected deprecated fix was basically setting QtKernelManager.kernel_cmd to our venv.interpreter. It might still be worth it to put something like that in the next release so people with older versions of jupyter_client stop experiencing crashes, in case the current solution takes too long to be reliable and ready.

@carlosperate
Copy link
Member Author

Thanks @devdanzin, this is really appreciated!

So the QtKernelManager.kernel_cmd method has been deprecated? In what version?

Can that old method still be changed to usestart_kernel instead of pre_start_kernel?

I'd prefer not to add more files if possible. Is it possible to hook into whatever is reading that file and pass the JSON directly?

@carlosperate carlosperate mentioned this pull request Dec 17, 2021
@ntoll
Copy link
Member

ntoll commented Dec 20, 2021

Aha... I should check all the relevant PRs before commenting on #1927 🤦

@carlosperate carlosperate modified the milestones: 1.1.0-beta.7, 1.1-final Dec 21, 2021
@carlosperate carlosperate modified the milestones: 1.1-final, Next release Feb 23, 2022
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

Successfully merging this pull request may close these issues.

3 participants