[MRG] Don't use shellEnv when launched from terminal #1906

betatim · 2017-09-24T08:25:20Z

Related to #620

This is pretty hacky but does mean you get a kernel that corresponds to the conda environment in which you typed npm run start. There are two things that puzzle me:

the process.env print out I inserted shows various custom env variables I set in my terminal, as well as CONDA_blah variables that belong to the current conda env, but it shows a different PATH. So it seems to inherit some variables but does something special to PATH. Any ideas where that is happening?
only seems to work with ipykernel~=4.6.x, at least in some of my envs where the version is 4.3.x executing a cell never completes. Just seems to hang and I can't find an error message anywhere.

If we can track down when/where PATH gets modified maybe we can prevent it from removing (or re-add) the path for the current conda environment. Maybe the real way to do this is to add a kernel spec like this if we detect that we are in either a virtualenv or a conda env and do "the right thing" for either to work.

WDYT?

rgbkrk · 2017-09-24T20:11:23Z

only seems to work with ipykernel~=4.6.x, at least in some of my envs where the version is 4.3.x executing a cell never completes. Just seems to hang and I can't find an error message anywhere.

~~That might be related to the whole ipykernel_launcher switchover.~~ yes yes indeed

rgbkrk · 2017-09-24T20:12:34Z

packages/desktop/src/main/kernel-specs.js

+      display_name: "python (nteract)",
+      language: "python",
+      env: {
+        ELECTRON_RUN_AS_NODE: "1",


You won't need this here since this was just something we passed through to enable our embedded JS kernel.

rgbkrk · 2017-09-24T20:14:09Z

packages/desktop/src/main/kernel-specs.js

+    spec: {
+      argv: ["python", "-m", "ipykernel_launcher", "-f", "{connection_file}"],
+      display_name: "python (nteract)",
+      language: "python",


We really need this to get merged successfully in with the user's default python if they happen to be the same path as you'll want the notebook to point to the right kernelspec in the document (here it would identify as python_nteract)

Double checking: You mean we should set the language to something else aka python_nteract?

I'm trying to say that if this kernelspec is exactly the same as their python kernelspec (by name -- folder on disk, key in the overall kernelspecs object here).

However, I'm thinking that naming it by env and only populating it when we know we're launched from the terminal may be a better approach (see below).

rgbkrk · 2017-09-24T20:15:07Z

Maybe the real way to do this is to add a kernel spec like this if we detect that we are in either a virtualenv or a conda env and do "the right thing" for either to work.

Sounds about right.

rgbkrk · 2017-09-24T20:22:40Z

For a random user with a standalone install on a mac who is using anaconda, their flow might end up looking like this:

Install desktop app
Launch by double-clicking a notebook (yes outside your use case)
This kernelspec will (possibly) be their system python (ew)
"It's named nteract, is this the right one?"
Be confused
Learn about the "right way"
Complain that this didn't make things simpler

I'm not actually sure so I think this could be destructive at the moment. I think there's a decent way to do this that is going to involve checking some environment variables as well as seeing which version of ipython is installed (I did some of this in a branch I closed a while ago). However... I think there's a way to do this for today in a way that makes the nteract shell script work ok in a way that the naming (hopefully) makes sense:

Special case CLI launches

When the nteract app is started from the CLI, set a special environment variable that says NTERACT_SOURCE_FROM_ENV_AT_CLI=1 (I'm being a little verbose). Then, make this kernel's display name be "python (current env)". It can also be a little more aware of whether it's in a virtual environment or a conda environment, showing a bit more information.

Generally that would make me a little more comfortable for one of these semi-generated kernelspecs.

betatim · 2017-09-24T20:31:45Z

Wouldn't checking for CONDA_PREFIX in the environment take care of the NTERACT_SOURCE_FROM_ENV_AT_CLI=1? It is only set if you source activate some-env so if nteract is started by double clicking it won't be set. So I'd propose to only add this kernel spec if we detect/are convinced we are being started "inside" a conda environment. Should also work out what virtualenv does which is the other popular way of having virtual environments.

rgbkrk · 2017-09-24T20:34:28Z

So I'd propose to only add this kernel spec if we detect/are convinced we are being started "inside" a conda environment.

👍

Wouldn't checking for CONDA_PREFIX in the environment take care of the NTERACT_SOURCE_FROM_ENV_AT_CLI=1?
Should also work out what virtualenv does which is the other popular way of having virtual environments.

Combine these two questions/statements and this is why I'm suggesting you set a var to communicate the intent of launching from the CLI. You can make the "Python (current env)" spec use whatever Python is on the path here.

rgbkrk · 2017-09-24T20:35:13Z

If we can track down when/where PATH gets modified maybe we can prevent it from removing (or re-add) the path for the current conda environment.

Right here: https://github.com/nteract/nteract/blob/d813188524733b825b3150b7be833912246c3d22/packages/desktop/src/main/prepare-env.js

rgbkrk · 2017-09-24T20:36:55Z

I'm going to add some notes to that file to state why we're doing certain things before they're lost to time. 😛

rgbkrk · 2017-09-24T20:41:25Z

Damn it, I just committed straight to master in the GitHub UI instead of making a PR like I meant to for the the comments on the env prep. See the updated version here: https://github.com/nteract/nteract/blob/d84fcc3d2ac3b16987ba3b6548fdeda76feda91d/packages/desktop/src/main/prepare-env.js

betatim · 2017-09-27T20:02:45Z

Some notes to (mainly) self: when launching the app by double click shellEnv gives me the following for PATH:

/Users/thead/.nvm/versions/node/v5.11.0/bin:./bin:/usr/local/bin:/usr/local/sbin:/Users/thead/.dotfiles/bin:/Developer/NVIDIA/CUDA-8.0/bin:/Users/thead/anaconda/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/opt/X11/bin:/Library/TeX/texbin:/node_modules/.bin:/Users/thead/git/nteract/nteract/packages/desktop/dist/mac/nteract.app/Contents/MacOS:/usr/texbin:/Users/thead/.rvm/bin

This is also what I get if I start a new terminal. So far so good.

Start from terminal, shellEnv gives me:

/Users/thead/.nvm/versions/node/v5.11.0/bin:./bin:/usr/local/bin:/usr/local/sbin:/Users/thead/.dotfiles/bin:/Developer/NVIDIA/CUDA-8.0/bin:/Users/thead/anaconda/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/opt/X11/bin:/Library/TeX/texbin:/Users/thead/git/nteract/nteract/packages/desktop/node_modules/.bin:/Users/thead/git/nteract/nteract/packages/node_modules/.bin:/Users/thead/git/nteract/nteract/node_modules/.bin:/Users/thead/git/nteract/node_modules/.bin:/Users/thead/git/node_modules/.bin:/Users/thead/node_modules/.bin:/Users/node_modules/.bin:/node_modules/.bin:/Users/thead/git/nteract/nteract/packages/desktop/node_modules/electron/dist/Electron.app/Contents/MacOS:/Users/thead/.nvm/versions/node/v8.1.3/lib/node_modules/npm/bin/node-gyp-bin:/Users/thead/anaconda/envs/skopt/bin:./bin:/usr/local/sbin:/Users/thead/.dotfiles/bin:/Developer/NVIDIA/CUDA-8.0/bin:/Users/thead/anaconda/bin:/usr/texbin:/Users/thead/.rvm/bin:/usr/texbin:/Users/thead/.rvm/bin

this somehow duplicates stuff. As a result the conda env path is towards the back and get's shadowed by the /usr/bin etc from the first set of paths.

So I (currently) think that all that needs to happen is to not use shellEnv when we detect that nteract was started from a terminal. If I by hand comment out the relevant bit in prepare-env.sh I indeed get a "python 3" kernel that matches my conda env. That is without the kernelspec hackery in this PR!

betatim · 2017-09-27T20:55:45Z

Checking if TERM is set is the best idea I have for detecting if we were launched from a terminal or not. Apparently you can also check if stdin is a file/tty??

The detection works for me on Mac. 🎉

codecov-io · 2017-09-27T21:07:59Z

Codecov Report

Merging #1906 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1906   +/-   ##
=======================================
  Coverage   79.62%   79.62%           
=======================================
  Files         116      116           
  Lines        4859     4859           
=======================================
  Hits         3869     3869           
  Misses        990      990

lgeiger · 2017-09-27T21:13:06Z

Checking if TERM is set is the best idea I have for detecting if we were launched from a terminal or not.

I think process.argv should be different in the main process if it's started via the CLI.

rgbkrk · 2017-09-27T21:16:41Z

packages/desktop/src/main/prepare-env.js

@@ -10,7 +10,9 @@ import "rxjs/add/operator/publishReplay";
 const env$ = Observable.fromPromise(shellEnv())
  .first()
  .do(env => {
-    Object.assign(process.env, env);
+    if (!process.env.TERM) {


I'm ok with merging this if it's working on your mac if you make the conditional process.platform !== 'darwin' || !process.env.TERM (or you can DeMorgan's Law this and do !(process.platform === 'darwin' && process.env.TERM))

If nteract was launched from a terminal there is no need to use shellEnv.

betatim · 2017-09-29T11:03:23Z

Waiting for travis, then good to go.

betatim · 2017-09-29T11:05:55Z

I got the following three values for process.argv: launch with npm run star: [ '/Users/thead/git/nteract/nteract/packages/desktop/node_modules/electron/dist/Electron.app/Contents/MacOS/Electron', '.' ]

launch with open packages/desktop/dist/mac/nteract.app:
[ '/Users/thead/git/nteract/nteract/packages/desktop/dist/mac/nteract.app/Contents/MacOS/nteract' ]

and when double clicking nteract.app from Finder:
['/Users/thead/git/nteract/nteract/packages/desktop/dist/mac/nteract.app/Contents/MacOS/nteract' ]

The last two cases are the ones we'd want to tell apart, so sad panda.

rgbkrk · 2017-09-29T23:18:31Z

packages/desktop/src/main/prepare-env.js

@@ -10,7 +10,10 @@ import "rxjs/add/operator/publishReplay";
 const env$ = Observable.fromPromise(shellEnv())
  .first()
  .do(env => {
-    Object.assign(process.env, env);
+    // no need to change the env if started from the terminal on Mac
+    if (process.platform !== "darwin" || !process.env.TERM) {


rgbkrk · 2017-09-29T23:19:00Z

Thanks for tackling this and going back+forth on it Tim to find the simplest way to solve this.

betatim · 2017-09-30T06:41:41Z

Years of physics training make me be unhappy if the solution is too complex :p

lock · 2018-04-02T13:28:20Z

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

rgbkrk reviewed Sep 24, 2017

View reviewed changes

rgbkrk reviewed Sep 27, 2017

View reviewed changes

Check if nteract was launched from a terminal

7e6cd75

If nteract was launched from a terminal there is no need to use shellEnv.

betatim force-pushed the inherit-conda-env branch from 52af39c to 7e6cd75 Compare September 29, 2017 11:02

betatim changed the title ~~Add a kernel that uses the current conda environment~~ [MRG] Don't use shellEnv when launched from terminal Sep 29, 2017

rgbkrk reviewed Sep 29, 2017

View reviewed changes

rgbkrk approved these changes Sep 29, 2017

View reviewed changes

rgbkrk merged commit c0dbe2e into nteract:master Sep 29, 2017

betatim deleted the inherit-conda-env branch September 30, 2017 06:40

lock bot locked and limited conversation to collaborators Apr 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Don't use shellEnv when launched from terminal #1906

[MRG] Don't use shellEnv when launched from terminal #1906

betatim commented Sep 24, 2017

rgbkrk commented Sep 24, 2017 •

edited

rgbkrk Sep 24, 2017 •

edited

rgbkrk Sep 24, 2017

betatim Sep 24, 2017

rgbkrk Sep 24, 2017

rgbkrk commented Sep 24, 2017

rgbkrk commented Sep 24, 2017 •

edited

betatim commented Sep 24, 2017

rgbkrk commented Sep 24, 2017

rgbkrk commented Sep 24, 2017 •

edited

rgbkrk commented Sep 24, 2017

rgbkrk commented Sep 24, 2017

betatim commented Sep 27, 2017

betatim commented Sep 27, 2017

codecov-io commented Sep 27, 2017 •

edited

lgeiger commented Sep 27, 2017

rgbkrk Sep 27, 2017

betatim commented Sep 29, 2017

betatim commented Sep 29, 2017

rgbkrk Sep 29, 2017

rgbkrk commented Sep 29, 2017 •

edited

betatim commented Sep 30, 2017

lock bot commented Apr 2, 2018

[MRG] Don't use shellEnv when launched from terminal #1906

[MRG] Don't use shellEnv when launched from terminal #1906

Conversation

betatim commented Sep 24, 2017

rgbkrk commented Sep 24, 2017 • edited

rgbkrk Sep 24, 2017 • edited

Choose a reason for hiding this comment

rgbkrk Sep 24, 2017

Choose a reason for hiding this comment

betatim Sep 24, 2017

Choose a reason for hiding this comment

rgbkrk Sep 24, 2017

Choose a reason for hiding this comment

rgbkrk commented Sep 24, 2017

rgbkrk commented Sep 24, 2017 • edited

Special case CLI launches

betatim commented Sep 24, 2017

rgbkrk commented Sep 24, 2017

rgbkrk commented Sep 24, 2017 • edited

rgbkrk commented Sep 24, 2017

rgbkrk commented Sep 24, 2017

betatim commented Sep 27, 2017

betatim commented Sep 27, 2017

codecov-io commented Sep 27, 2017 • edited

Codecov Report

lgeiger commented Sep 27, 2017

rgbkrk Sep 27, 2017

Choose a reason for hiding this comment

betatim commented Sep 29, 2017

betatim commented Sep 29, 2017

rgbkrk Sep 29, 2017

Choose a reason for hiding this comment

rgbkrk commented Sep 29, 2017 • edited

betatim commented Sep 30, 2017

lock bot commented Apr 2, 2018

rgbkrk commented Sep 24, 2017 •

edited

rgbkrk Sep 24, 2017 •

edited

rgbkrk commented Sep 24, 2017 •

edited

rgbkrk commented Sep 24, 2017 •

edited

codecov-io commented Sep 27, 2017 •

edited

rgbkrk commented Sep 29, 2017 •

edited