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

Jupyter explicit kernel #3337

Merged
merged 63 commits into from Mar 28, 2019
Merged

Jupyter explicit kernel #3337

merged 63 commits into from Mar 28, 2019

Conversation

haraldschilly
Copy link
Contributor

@haraldschilly haraldschilly commented Nov 7, 2018

Description

Jupyter notebooks without a kernel or an unknown one are a source of confusion. This patch presents a clear selection dialog with some explanations about what a kernel actually is to the user.

Reference: there are two situations the dialog shows up. either user requested, or when opening and the kernel is "bad". here is a side-by-side view of running this in two instances. The dialog only closes when a kernel is set by itself, when it wasn't opened by the user in the first place.

Testing Steps

important: switch to experimental image to see the suggestions.

  1. existing notebook with known kernel → this doesn't show up
  2. click on kernel info at the top right → kernel "change" dialog shows up
  3. open a new ipynb file → dialog asks what to do (slightly different from above)
  4. an unknown kernel can be created like that:
~$ cat <<EOF > unknown-7.ipynb
{
 "cells": [ ],
 "metadata": {
  "kernelspec": {
   "name": "pythonXY"
  }
 },
 "nbformat": 4,
 "nbformat_minor": 0
}
EOF

~$ open unknown-7.ipynb

Relevant Issues

I didn't check

Checklist:

  • No debugging console.log messages.
  • All new code is actually used.
  • Non-obvious code has some sort of comments.

Front end:

  • Restart at least one project and check its ~/.smc/local_hub/local_hub.log
  • Completely restart Webpack with ./w in /src
  • Screenshots if relevant.

Back end:

  • it touches the "project"!

@haraldschilly haraldschilly force-pushed the jupyter-explicit-kernel branch 2 times, most recently from 591507f to 82607bc Compare November 8, 2018 17:45
@haraldschilly haraldschilly force-pushed the jupyter-explicit-kernel branch 7 times, most recently from e163416 to dcc4929 Compare November 8, 2018 19:48
@williamstein
Copy link
Contributor

I tested this (with the experimental image, and after restarting my dev project). Step 1 of your testing step, namely "existing notebook with known kernel → this doesn't show up" fails in multiple ways.

  • Make a new notebook, select kernel, save to disk, duplicate, and open the duplicate. FAIL -- the select kernel screen shows up anyways. Saving and mucking around a bit and suddenly it goes away.
  • Open an old notebook that I had from 2 months ago. FAIL -- the select kernel screen shows up anyways.
  • Click on the bell and click to open a random old notebook from the history in a project that is not running. FAIL seeing this:

image


What you're implementing here reminds me of all the MANY subtle bugs we had with the default kernel, and kernels changing from what is in the notebook to the default, etc. They were all very subtle race conditions involving the project and client(s) opening the notebook at the same time, then making decisions. This kernel selection you're implementing here is like that, but even more difficult...

For example, what if two people open the same notebook. If person A selects a kernel, then the "select kernel" screen for person B must properly update. Your current implementation fails in this case too:

screenshot 2018-11-08 at 4 38 45 pm

I strongly encourage you to spend at least 1-2 more days on this, and either finish it completely so it really works right and is in CoCalc. Or delete it. One or the other. Don't let it bitrot!

@williamstein
Copy link
Contributor

I also have to wonder -- aren't there a lot of people that only ever use one kernel? E.g., I imagine basically all sage users only ever use Sage (I'm thinking, e.g., of my phd student Kevin). With this PR, he'll go from never thinking about kernel selection to having to make a choice and click a button every single time he makes a new notebook. And he has no possible way out of that added friction -- it would be good to have an account preference to have a default kernel. (In contrast, Sage worksheets continue to not have extra friction like this.)

@haraldschilly
Copy link
Contributor Author

Hmm, I tested some of what you describe and it worked for me. It's not easy, yes. I'm wondering if what you really mean is a "do not ask again" boolean option. (if it is true && there is a default kernel in the settings → pick it without asking). I'm using that last-kernel setting for the quick selection as the first option, as a compromise to avoid searching around too much.

I'll look into this, sure.

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging 95818ab into 98b993e - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging 4481910 into f5827a7 - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging 38bf546 into f5827a7 - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging 8b9e86f into 9307736 - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging 984dd84 into 91d2639 - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging 303d3d8 into b0df27e - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging f6d5202 into d412a05 - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging f72380d into 1a1f026 - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging 906749d into 9a78ddc - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@haraldschilly
Copy link
Contributor Author

This pull request introduces 2 alerts when merging 6d5823f into 0a3e655 - view on LGTM.com

new alerts:

  • 1 for Assignment to constant
  • 1 for Comparison between inconvertible types

Comment posted by LGTM.com

@williamstein
Copy link
Contributor

I didn't realize this is needs review. I'm refactoring a lot of relevant code which will directly conflict with this. Ugh.

@williamstein williamstein merged commit 6f5fd61 into master Mar 28, 2019
@williamstein
Copy link
Contributor

Merged. This is really good. Any bitrot/merge conflicts are on me now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants