Skip to content

detect virtual environments created with venv#370

Merged
jjallaire merged 3 commits intomasterfrom
feature/python-venv
Oct 24, 2018
Merged

detect virtual environments created with venv#370
jjallaire merged 3 commits intomasterfrom
feature/python-venv

Conversation

@kevinushey
Copy link
Copy Markdown
Collaborator

This PR is the first step in support for Python virtual environments created by venv.

Note that these virtual environments are almost identical to those created with virtualenv. The only difference is that the activate_this.py script is missing. My understanding is that activate_this.py is a hack that tries to allow an existing Python session to bind to an existing virtual environment; whereas the preferred route is to instead run e.g.

source bin/activate && python

which works as expected for virtual environments created by both virtualenv and venv.

Something we should discuss then is whether we need any other code changes to support virtual environments that don't contain this script. I see we use it at e.g.

reticulate/src/python.cpp

Lines 1433 to 1458 in 93c9ee9

// [[Rcpp::export]]
void py_activate_virtualenv(const std::string& script)
{
// get main dict
PyObject* main = PyImport_AddModule("__main__");
PyObject* mainDict = PyModule_GetDict(main);
// create local dict with __file__
PyObjectPtr localDict(PyDict_New());
PyObjectPtr file(as_python_str(script));
int res = PyDict_SetItemString(localDict, "__file__", file);
if (res != 0)
stop(py_fetch_error());
// read the code in the script
std::ifstream ifs(script.c_str());
if (!ifs)
stop("Unable to open file '%s' (does it exist?)", script);
std::string code((std::istreambuf_iterator<char>(ifs)),
(std::istreambuf_iterator<char>()));
// run string
PyObjectPtr runRes(PyRun_StringFlags(code.c_str(), Py_file_input, mainDict, localDict, NULL));
if (runRes.is_null())
stop(py_fetch_error());
}

It's worth noting that bin/activate effectively does two things:

  1. Sets the VIRTUAL_ENV environment variable; its path points to the virtual environment,
  2. Updates the PATH, and puts the virtual environment bin directory first.

Assuming that embedded Python sessions also respect the VIRTUAL_ENV environment variable, we should validate that setting that before initializing Python is enough to ensure that the newly-launched Python session uses the virtual environment as requested.

@kevinushey
Copy link
Copy Markdown
Collaborator Author

Note that this PR does indeed ensure Python sets sys.prefix and sys.exec_prefix as we would hope for a virtual environment:

reticulate::use_virtualenv("~/scratch/venv", required = TRUE)
reticulate::py_config()
#> python:         /Users/kevin/scratch/venv/bin/python
#> libpython:      /usr/local/opt/python/Frameworks/Python.framework/Versions/3.7/lib/python3.7/config-3.7m-darwin/libpython3.7.dylib
#> pythonhome:     /usr/local/opt/python/Frameworks/Python.framework/Versions/3.7:/usr/local/opt/python/Frameworks/Python.framework/Versions/3.7
#> version:        3.7.0 (default, Sep 18 2018, 18:47:22)  [Clang 9.1.0 (clang-902.0.39.2)]
#> numpy:           [NOT FOUND]
#> 
#> NOTE: Python version was forced by use_python function

sys <- reticulate::import("sys")
sys$prefix
#> [1] "/Users/kevin/scratch/venv"
sys$exec_prefix
#> [1] "/Users/kevin/scratch/venv"

Created on 2018-10-23 by the reprex package (v0.2.1)

So at least this PR passes the smoke tests. Can you think of anything else I should double-check just to be sure?

@jjallaire
Copy link
Copy Markdown
Member

Does the PATH end up getting updated by a call we make to Sys.setenv() ? (I seem to recall that we do this).

@kevinushey
Copy link
Copy Markdown
Collaborator Author

We set the PATH just above:

reticulate/R/package.R

Lines 107 to 109 in 2fa659a

Sys.setenv(PATH = paste(paste(python_dirs, collapse = .Platform$path.sep),
Sys.getenv("PATH"),
sep = .Platform$path.sep))

@jjallaire
Copy link
Copy Markdown
Member

Will merge now, let’s be sure to add a NEWS item

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.

2 participants