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

[WIP] Connect to existing kernels #4184

Open
wants to merge 63 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@CrystallineCat
Copy link
Contributor

commented Feb 2, 2019

Trying to implement --existing.

renovate-bot and others added some commits Feb 2, 2019

@todo

This comment has been minimized.

Copy link

commented Feb 2, 2019

Really needs a better label

runningKernels.map(x => ({ // TODO Really needs a better label
label: `${x.ip}:${x.control_port}`,
click: createSender("menu:connect-to-kernel", x)
})),
"label"
),


This comment was generated by todo based on a TODO comment in 5054d05 in #4184. cc @CrystallineCat.

captainsafia and others added some commits Feb 2, 2019

Merge pull request #4182 from nteract/renovate/zeit-ncc-0.x
Update dependency @zeit/ncc to ^0.14.0
@rgbkrk

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

This is a cool idea!

A couple things to note that might be helpful --

  • with the command line argument for existing we don't have to detect all running kernels, only combine the path with the runtime dir on startup. I suppose this would be a new way to launch, so we'd have to do some coordination between main and renderer

  • we could potentially update the menu for running kernels by continuously watching the runtime dir while launched, updating the menu entry for it. I'm not sure of the perf & cross platform impact of this though.

BenRussert and others added some commits Feb 3, 2019

Merge pull request #4181 from rgbkrk/styles-package
create a styles package to load from
@CrystallineCat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

Thank you for your thoughts!

  • with the command line argument for existing we don't have to detect all running kernels, only combine the path with the runtime dir on startup. I suppose this would be a new way to launch, so we'd have to do some coordination between main and renderer

Yeah, I guess I've got a bit of scope creep. 😏

Just connecting to the given kernel was the initial intention, but the immediate use case I have involves a kernel whose output I don't see, so I'd have to list the kernels in a terminal (or find the relevant process ID) anyway. I think not knowing the connection file name isn't that uncommon.

Additionally, the implementation in jupyter_client has far more complicated semantics:

  • the argument is a fileglob
  • if the argument doesn't contain *, it will be surround with *
  • if nothing is supplied, it uses kernel-*.json
  • it looks for the file in the cwd as well as the runtime dir
  • if multiple files match, it uses the most recently accessed file

For compatibility I'd like to match that implementation (with the possible exception of the last point).

jupyter/jupyter_client · jupyter_client/connect.py#L169 (for my own reference)

  • we could potentially update the menu for running kernels by continuously watching the runtime dir while launched, updating the menu entry for it. I'm not sure of the perf & cross platform impact of this though.

I've thought about this, but right now I don't know how to do it cleanly. I'll probably attempt it at some point in the future.

captainsafia and others added some commits Feb 3, 2019

Publish
 - nteract@1.0.1
 - @nteract/nbextension@1.0.3
 - nteract-on-jupyter@2.0.3
 - @nteract/data-explorer@6.0.2
 - @nteract/kernel-relay@2.0.2
 - @nteract/notebook-app-component@5.0.2
 - @nteract/presentational-components@3.0.1
 - @nteract/styles@2.0.1
Merge pull request #4183 from lgeiger/remove-moment
Remove moment.js from the webpack bundle
Merge pull request #4189 from rgbkrk/kernel-relay-out
remove kernel-relay from monorepo
Merge pull request #4188 from nteract/renovate/jest-24.x
Update dependency @types/jest to v24
Merge pull request #4185 from nteract/renovate/prettier-1.x
Update dependency prettier to v1.16.4

stormpython and others added some commits Feb 8, 2019

Merge pull request #4199 from stormpython/bug/keybindings-respect-met…
…a-key

Make keybindings respect meta key
Merge pull request #4203 from nteract/renovate/react-table-hoc-fixed-…
…columns-1.x

Update dependency react-table-hoc-fixed-columns to v1.0.6
removing code from troubleshooting because it was not working. Need t…
…o find another way to get the appBase props value
placing conditional for appBase component upon component initializati…
…on. On initialization, the appBase prop is not populated, so a default is needed in that case.
Publish
 - @nteract/nbextension@1.0.5
 - nteract-on-jupyter@2.0.5
 - @nteract/actions@2.1.0
 - @nteract/connected-components@6.0.2
 - @nteract/core@10.1.0
 - @nteract/data-explorer@6.0.4
 - @nteract/epics@2.1.0
 - @nteract/fixtures@2.0.2
 - @nteract/notebook-app-component@5.0.4
 - @nteract/reducers@2.1.0
Merge pull request #4157 from stormpython/save-as-keybinding
Keybindings: Adds `open` keyboard shortcut
Merge pull request #4208 from nteract/renovate/codemirror-0.x
Update dependency @types/codemirror to ^0.0.72
@rgbkrk

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Uhoh! Looks like an interactive rebase went poorly. Let me know if you'd like help rectifying it.

@captainsafia

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Hi @CrystallineCat! How are things going with this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.