-
Notifications
You must be signed in to change notification settings - Fork 436
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
base: master
Are you sure you want to change the base?
Conversation
Hi @carlosperate @devdanzin - any updates on this branch. Reading the commentary by Carlos I, too, wonder about our use of |
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). |
@carlosperate I'll try looking at this before beta 7. |
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 |
There's a way that removes the need to both call It requires creating a Kernel Spec on disk, which is a little JSON file pointing to the right interpreter, and then installing it via 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 |
Thanks @devdanzin, this is really appreciated! So the Can that old method still be changed to use 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? |
Aha... I should check all the relevant PRs before commenting on #1927 🤦 |
In PR #1240 we started using
KernelManager.pre_start_kernel()
via inheritance from thejupyter_client
package, 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 likestart_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?