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

Don't rely on jupyter to discover kernel specs #1031

Merged
merged 4 commits into from
Oct 9, 2017

Conversation

lgeiger
Copy link
Member

@lgeiger lgeiger commented Oct 6, 2017

This PR brings Hydrogen inline with nteract regarding kernel discovery. It now uses kernelspecs to get the available kernels from the system.

The main advantage of using kernelspecs over jupyter kernelspec list --json is the speed. Our current solution takes approximately 800ms to get all available kernels. This time can easily be over 2s during initial startup (note my shell is minimal and quite fast, on other system this can take a lot longer). I think this long startup time is unacceptable. The new solution will take only 10ms for updating the kernels (during initial startup it will take around 30-50ms) which makes startup noticable faster 🚀.
I haven't looked at kernelspecs and jupyter-paths in detail, but I think we can even optimize the speed further.

Appart from that this removes jupyter as a dependency. This means people using other kernels than IPython no longer need a Python installation just to find the kernels. This brings us inline with nteract and allows us to ensure a seamless back and forth between nteract and Hydrogen.

Main changes

  • Use kernelspecs over jupyter kernelspec list --json
  • Use async/await and promises for managing kernel specs

BREAKING CHANGES

  • Remove Kernelspec setting. This setting was introduced because we ran into some issues where jupyter was not available from Atom. Editing this setting is a horrible user experience and everything can be achived by properly installing the kernels too.
  • Remove support for old python installations, relying on deprecated ipython kernel directories. Come on it's 2017 😉
  • The default Python installation won't be discovered anymore. The IPython kernel needs to be installed with:
python -m pip install ipykernel
python -m ipykernel install --user

I think the advantages are well worth the breaking change. They also fit quite nicely in our release cycle since #123 can be also considered as a breaking change. I guess we're on the road to 2.0.0 now 😇

@rgbkrk
Copy link
Member

rgbkrk commented Oct 7, 2017

Remove support for old python installations, relying on deprecated ipython kernel directories. Come on it's 2017

Definitely a fan there.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 7, 2017

I am a tad worried about the PATH not being fully configured well. A lot of people have their python3 kernel installed as:

{
 "argv": [
  "python",
  "-m",
  "ipykernel_launcher",
  "-f",
  "{connection_file}"
 ],
 "display_name": "Python 3",
 "language": "python"

Which, at least on the mac, ends up being their default Python 2 environment (which tends to have no ipython installed). They then report that "Python 3" is running "Python 2", "for some reason". I guess I should say I'm in favor of merging this since it means we'll do more reliance on and fixing of jupyter-paths + kernelspecs. jupyter-paths even has a flag to use jupyter underneath if necessary to get the right paths.

Just let me know if you want me to move jupyter-paths and kernelspecs back in the monorepo at any point -- I figured out how to do it sometime ago then didn't revisit it because it was no longer a priority.

README.md Outdated
@@ -4,7 +4,7 @@
[![Greenkeeper badge](https://badges.greenkeeper.io/nteract/hydrogen.svg)](https://greenkeeper.io/)
[![Build Status](https://travis-ci.org/nteract/hydrogen.svg?branch=master)](https://travis-ci.org/nteract/hydrogen)

This package lets you run your code directly in Atom using any [Jupyter](https://jupyter.org/) kernels you have installed.
Hydrogen is a interactive coding environment that supports Python, R, JavaScript and [other Jupyter kernels](https://github.com/jupyter/jupyter/wiki/Jupyter-kernels).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...is an interactive...

@@ -1,24 +1,24 @@
# Installation

For all systems, you'll need
Hydrogen requires **[Atom](https://atom.io/)** `1.20.0+` and a **[Kernel](##kernels)** for the language you intend to use Hydrogen with.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably say "...and kernels for the languages you intend..."

Tested and works with:

- [IPython](http://ipython.org/)
- [IRkernel](https://github.com/IRkernel/IRkernel) `0.4+` requires
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you intending to put more after the requires here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, you probably want the language-r bits.

@lgeiger
Copy link
Member Author

lgeiger commented Oct 9, 2017

I am a tad worried about the PATH not being fully configured well.

I just tried the latest ipykernel release and it properly installs the kernels using the full path.

jupyter-paths even has a flag to use jupyter underneath if necessary to get the right paths.

I know, but that mitigates the upsides of this PR, since we would need to spawn a child process which is a lot slower.

Just let me know if you want me to move jupyter-paths and kernelspecs back in the monorepo at any point -- I figured out how to do it sometime ago then didn't revisit it because it was no longer a priority.

That's a good idea. Though it might be difficult to get the unit test working properly.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 9, 2017

Though it might be difficult to get the unit test working properly.

You'd know the best there. 😉

@lgeiger
Copy link
Member Author

lgeiger commented Oct 9, 2017

Though it might be difficult to get the unit test working properly.

I think we'd need to install Python on Travis to make it work. If I recall correctly the unit tests of kernelspecs are actually a integration test.

@lgeiger
Copy link
Member Author

lgeiger commented Oct 9, 2017

I am a tad worried about the PATH not being fully configured well.

Do you think this change will cause a lot of trouble for people?
I hope it makes it easier for new people to get started with Hydrogen and only introduce minor hurdles for people who switch from weird custom setups.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 9, 2017

If I recall correctly the unit tests of kernelspecs are actually a integration test.

That is correct.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 9, 2017

I am a tad worried about the PATH not being fully configured well.

Do you think this change will cause a lot of trouble for people?

I'm primarily worried about Anaconda users that want their root install working well. I'm sure that we'll be able to fix it up / dig into where we need to find that root install though.

@rgbkrk rgbkrk merged commit 2498e5e into nteract:master Oct 9, 2017
@rgbkrk
Copy link
Member

rgbkrk commented Oct 9, 2017

Merging, let's see how this goes.

@lgeiger lgeiger deleted the kernelspecs branch October 9, 2017 04:32
@lgeiger lgeiger mentioned this pull request Oct 14, 2017
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.

2 participants