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 try to detect virtualenvs when starting a kernel #96

Merged
merged 1 commit into from Feb 24, 2016

Conversation

takluyver
Copy link
Member

I think automatically adding an active virtualenv to sys.path makes less sense in a kernel than it does in terminal IPython (and I'm less certain than I was even for terminal IPython).

If Jupyter is installed inside a virtualenv, it will still launch the default kernel in there too, but this would mean that you can no longer run e.g. 'jupyter console' with a virtualenv active and find packages from that virtualenv.

I think automatically adding an active virtualenv to sys.path makes less
sense in a kernel than it does in terminal IPython (and I'm less certain
than I was even for terminal IPython).

If Jupyter is installed inside a virtualenv, it will still launch the
default kernel in there too, but this would mean that you can no longer
run e.g. 'jupyter console' with a virtualenv active and find packages
from that virtualenv.
takluyver added a commit to takluyver/jupyter_console that referenced this pull request Jan 18, 2016
There's no point doing the virtualenv check in the frontend, and the
extra warning message is confusing.

ipython/ipykernel#96 is for discussion, this one should just be a
bugfix.
@jgosmann
Copy link

Judging from my current workflow, I would prefer a detection of the virtualenv. It would not require me to install Jupyter in every virtualenv or I won't have to create a new kernel for every virtualenv.

@minrk minrk added this to the 4.3 milestone Jan 19, 2016
@minrk
Copy link
Member

minrk commented Jan 19, 2016

Since after-the-fact env activation will always be semi-broken, +1 to this.

@takluyver
Copy link
Member Author

To expand on what Min means by 'semi-broken': virtualenvs are isolated by default, so when you're running Python inside a virtualenv, it doesn't look for imports in your system or user site-packages directories. If you started Python inside a virtualenv and did import IPython, it would fail unless IPython was installed in that virtualenv.

Our virtualenv detection can't emulate this, because IPython itself is running in a Python outside the virtualenv. We add the virtualenv's site-packages to sys.path, but we can't block imports of code from outside the virtualenv, because a lot of modules are already loaded from outside. If the virtualenv has a different version of Python, we also can't switch to that.

Rather than just removing the feature silently, perhaps we can provide some machinery or documentation to help people who liked this.

  • By putting the venv detection code in a startup script, it should be possible to restore the current behaviour exactly. This is a fairly simple option.
  • With a custom kernel manager (and/or kernel spec manager), you could have better virtualenv detection: it could detect the venv, install ipykernel into it if necessary, and launch the kernel inside the virtualenv. This would be especially neat for jupyter_console.
  • A tool that integrates with virtualenvwrapper could expose all of your environments as kernels - either with another custom kernel manager, or using some of the hooks virtualenvwrapper provides for actions when making/destroying an env.

@minrk
Copy link
Member

minrk commented Feb 1, 2016

It also means that any packages that have been imported by IPython prior to detecting the venv may be the wrong versions, causing confusion.

Installing the kernel into the env automatically would be pretty slick.

minrk added a commit that referenced this pull request Feb 24, 2016
Don't try to detect virtualenvs when starting a kernel
@minrk minrk merged commit 25591f8 into ipython:master Feb 24, 2016
@takluyver
Copy link
Member Author

Do we think this is appropriate for a 4.x release? It is a change in behaviour.

@minrk
Copy link
Member

minrk commented Feb 24, 2016

I think it is, since it seems to me like fixing a bug (virtualenvs messed up your kernel's sys.path). But we can be conservative and call it 5.0. But if we do call it 5.0, I don't think that changes the scope of this release, it just means that everything currently marked for 5.0 becomes 6.0.

@minrk
Copy link
Member

minrk commented Feb 24, 2016

Alternately, we can branch 4.x prior to this merge (or revert it) and continue discussing before to a later 5.0.

@takluyver
Copy link
Member Author

I don't have a strong feeling, but given that some people are using this behaviour currently, the change should be in a major release. It would also be nice to have something to point those users to when they find it's gone. I think that points to branching 4.x before this merge.

@willingc
Copy link
Member

Very interesting discussion. FWIW, I didn't realize that this was the behavior happening when running in a virtualenv. My workflow is typically to create a virtualenv for each project and then install all needed into the virtualenv.

It also means that any packages that have been imported by IPython prior to detecting the venv may be the wrong versions, causing confusion.

This explains some of the technical issues re: installation folks have had.

Installing the kernel into the env automatically would be pretty slick.

It would. However, it would nice to explicitly tell the user when automatically loading something new.

@minrk minrk modified the milestones: 5.0, 4.3 Feb 25, 2016
@minrk minrk modified the milestones: 5.0, 4.4 Aug 8, 2016
@skycaptain
Copy link

Ran into this today. Updating to 5.0 broke my setup, since I installed jupyter globally and use it inside virtualenvs, so I would not have to install ipykernel inside every venv nor would I have to create a kernel for each.

By putting the venv detection code in a startup script, it should be possible to restore the current behaviour exactly. This is a fairly simple option.

Do you mean putting the old code inside e.g. c.InteractiveShellApp.exec_lines? Any chance we can get the old behaviour as an opt-in?

@takluyver
Copy link
Member Author

You can use exec_lines, or a startup file. Sorry, but I don't think we're interested in maintaining the old behaviour as an option.

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

5 participants