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

Avoid KernelManager.pre_start_kernel. Use kernel spec for the REPL instead. #1927

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

devdanzin
Copy link
Contributor

This is in response to a discussion in #1517: KernelManager.pre_start_kernel(), added in #1240, is not compatible with old versions of jupyter_client and is actively causing crashes for our users.

This PR removes the subclassing of QtKernelManager and pre_start_kernel(). It uses a kernel spec, which has to be saved to disk, to pass the correct interpreter to jupyter_client.

The whole design is up for discussion. One issue might be persisting invalid specs, because currently it's only created on disk if no kernel exists with the same name we'd use.

@ntoll
Copy link
Member

ntoll commented Dec 20, 2021

@devdanzin 👋 thank you so much (again) for your contributions. I've taken a quick look at the code and it seems obvious, clear, simple and smaller than the current solution, so I think we're making progress..! As the CI demonstrates, the test suite doesn't pass. Care to make check locally to reveal the problem..? Thanks in advance.

@carlosperate
Copy link
Member

Discussion about this implementation can also be found here: #1517 (comment)

Taking in consideration our luck with antiviruses interfering with Mu's ability to write and read files from disk it might be more robust if we could find a way that avoid a json file being written to disk.
And if there is no way around it, it might still be good to use the old method if the installed jupiter version still allows it.
I might even go as far as suggesting setting a max jupyter version for the v1.1 Mu release (to be remove immediately after the release for v1.2 betas), just to avoid that.

It would still be good to find out if we could use start_kernel() instead of pre_start_kernel() to be able to reduce the min version though.

@ntoll
Copy link
Member

ntoll commented Dec 20, 2021

I don't have the Jupyter context to know for certain which are the best solutions. I'm happy to be guided by those in the know. @carlosperate I think the temporary pinning of Jupyter for Mu 1.1 might be a path-of-least-resistance option here, although if @devdanzin can confirm start_kernel is "good enough" and try that option too we could just go with that. Finally, there's a part of me that wonders if a user can't write JSON files to their home directory, then Mu is clearly in a problematic environment and won't be able to function correctly anyway. While I realise we spend a lot of effort making Mu "just work" for folks, if we find that antivirus software is a major cause for errors then we may have to try one of the alternatives Carlos suggests.

Thoughts..?

@carlosperate
Copy link
Member

Finally, there's a part of me that wonders if a user can't write JSON files to their home directory, then Mu is clearly in a problematic environment and won't be able to function correctly anyway.

It's not that we can't write files, I think the main issue is trying to read the files immediately after writing them to disk, as the antivirus might be locking access to the files while it is analysing them, and that's when Mu errors out.
So, due to this we have crash reports during Mu start up venv creation (so that means users unable to launch Mu) because of this, but the hope is that for the rest of the "normal Mu operation" there shoudn't be issues.

To be fair, this might not be an problem for a tiny JSON file (compare to a larger venv folder), as I would hope the antivirus would take almost no time to clear it, but this is also a case where we are trying to read the file immediately after writing it, and since we haven't replicated the other issues yet, we don't know if this will fall in the same trap.

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