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

caching available kernels into config #177

Merged
merged 6 commits into from Feb 25, 2016
Merged

caching available kernels into config #177

merged 6 commits into from Feb 25, 2016

Conversation

slavaGanzin
Copy link
Contributor

No description provided.


updateKernels: ->
save = (out) =>
@availableKernels = _.pluck JSON.parse(out).kernelspecs, 'spec'
Copy link
Collaborator

Choose a reason for hiding this comment

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

JSON.parse(out) throws because out is not valid JSON when using IPython v1.2.1

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we need tooling for upgrading from old specs to new specs, though I think it can be a follow on PR after.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about guarding with a try/catch block?

         save = (out) =>
            try
                @availableKernels = _.pluck JSON.parse(out).kernelspecs, 'spec'
                @setConfigJson 'kernelspecs', @availableKernels
                atom.notifications.addInfo 'Hydrogen Kernels updated:',
                  detail: (_.pluck @availableKernels, 'display_name').join('\n')
            catch

@n-riesco
Copy link
Collaborator

Also you need to update lib/main.coffee:

module.exports = Hydrogen =
    config:
        kernelspecs:
            title: "Kernel Specs"
            description: 'Kernel specs as reported by:

                            $ jupyter kernelspecs list --json

                          E.g.:

                            {
                              "ijavascript": {
                                "spec": {
                                  "display_name": "IJavascript",
                                  "env": {},
                                  "argv": [
                                    "node",
                                    "/home/user/node_modules/ijavascript/lib/kernel.js",
                                    "--protocol=5.0",
                                    "{connection_file}"
                                  ],
                                  "language": "javascript"
                                },
                                "resources_dir": "/home/user/node_modules/ijavascript/images"
                              }
                            }'
            type: 'string'
            default: '{}'

@n-riesco
Copy link
Collaborator

As a user, I don't want to type my custom kernelspecs every time I launch Atom. I think if a user defines manually the kernel specs, Hydrogen shouldn't delete the user's settings.

@slavaGanzin
Copy link
Contributor Author

@n-riesco And it wouldn't. If you have non working ipython kernelspec and jupyter kernelspec, it would use your kernelspec option.
What for you want to edit kernelspecs? If you want to optimize something or provide keys to kernel I though it should be done internally in jupyter config (if any)

@n-riesco
Copy link
Collaborator

#38

@n-riesco
Copy link
Collaborator

This commit is not working for me.

I'm still debugging, but I've noticed typos here and here

@n-riesco
Copy link
Collaborator

I don't understand the logic here. If we don't return, kernelspec is undefined.

@n-riesco
Copy link
Collaborator

Why is the setting kernelspec and not kernelspecs?

PS: Does anyone know where atom stores the settings? /home/user/.atom/config.cson

@slavaGanzin
Copy link
Contributor Author

I'm still debugging, but I've noticed typos here and here

That typos were for my debug purposes. And I left them - my fault

Why is the setting kernelspec and not kernelspecs?

Because it's an output of kernelspec command, which contain kernelspecs property.

@slavaGanzin
Copy link
Contributor Author

#38
You definitely shouldn't use that param for remote kernels.
You can specify kernel.json with same functionality in your jupyter|ipython home directory and use it for this.

@slavaGanzin
Copy link
Contributor Author

PS: Does anyone know where atom stores the settings? /home/user/.atom/config.cson
Yes

@slavaGanzin
Copy link
Contributor Author

@n-riesco
Copy link
Collaborator

@slavaGanzin I disagree on #38 (we shouldn't force users to install IPython to use Hydrogen, specially users of languages other than Python).

https://jupyter-client.readthedocs.org/en/latest/kernels.html#connection-files

Connection files represent actual connections (they don't define a kernel spec).

@slavaGanzin
Copy link
Contributor Author

https://jupyter-client.readthedocs.org/en/latest/kernels.html#kernel-specs
Sorry wrong link.
Nobody forces to use Ipython, you can use jupyter it works the same

@n-riesco
Copy link
Collaborator

This line is causing the kernelspec spec settings to be reset each time Atom is launched. It should be:

save stdout unless e

@n-riesco
Copy link
Collaborator

I reckon this line should be:

kernels = _.pluck @getConfigJson('kernelspec', {kernelspecs:{}}).kernelspecs, 'spec'

@slavaGanzin
Copy link
Contributor Author

save stdout unless e

I rewrite a bit and now it's guarded with:
a579155#diff-aa15d36609a9a564a24156e6778cd62fR28

it can't be unless e because we should show this message a579155#diff-aa15d36609a9a564a24156e6778cd62fR23

@n-riesco
Copy link
Collaborator

Nobody forces to use Ipython, you can use jupyter it works the same

That's not my point. Ideally, there shouldn't be any need for Hydrogen users to install Jupyter or IPython.

The current state of Hydrogen is that either a recent version of IPython or Jupyter need to be present to run Hydrogen.

Previous versions of Hydrogen used to work on a box with IPython 1.2.1 (as long as the kernel specs were installed in one of the locations known to Hydrogen).

When I reported #173 , I was hoping to find a friendly compromise. I'm not asking to stop using the newer versions. What I'm asking is that we write robust code (i.e. doesn't throw uncaught exceptions) and that allows Hydrogen to run in a standard Ubuntu 14.04 box.

@n-riesco
Copy link
Collaborator

I've tested the latest commits and it's working in my machine.
Thanks!

@slavaGanzin slavaGanzin mentioned this pull request Feb 25, 2016
@slavaGanzin
Copy link
Contributor Author

@n-riesco Thanks for review
@rgbkrk I think this is ready to merge. And release

@rgbkrk
Copy link
Member

rgbkrk commented Feb 25, 2016

What would you say our version bump is here?

rgbkrk added a commit that referenced this pull request Feb 25, 2016
caching available kernels into config
@rgbkrk rgbkrk merged commit 3ba76d7 into master Feb 25, 2016
@rgbkrk rgbkrk deleted the availableKernels branch February 25, 2016 22:19
@n-riesco
Copy link
Collaborator

@rgbkrk I haven't followed some of the latest commits, so I can't answer your question. But I would suggest that:

  • if new dependencies have been added, we would bump at least the minor number (I wouldn't bump to 1.0.0 just yet)
  • if no new dependencies are required, then I would only bump the patch number.

@slavaGanzin
Copy link
Contributor Author

0.7.1

@rgbkrk
Copy link
Member

rgbkrk commented Feb 26, 2016

Great, done! 🎉

$ apm publish patch
Preparing and tagging a new version ✓
Pushing v0.7.1 tag ✓
Publishing Hydrogen@v0.7.1 ✓

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.

None yet

3 participants