1473727 - Every mach python-test run prompts to reinstall the virtualenv

Status: RESOLVED FIXED

(Testing :: Python Test, defect)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

$ ./mach python-test taskcluster/taskgraph/test/test_util_taskcluster.py                                                                                                                                                                                                          
Virtualenv already exists!
Remove existing virtualenv? [Y/n]: y
Removing existing virtualenv…
Creating a virtualenv for this project…
Using /home/dustin/p/m-c/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python (2.7.12) to create virtualenv…
⠋Running virtualenv with interpreter /home/dustin/p/m-c/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
Using real prefix '/usr'
New python executable in /home/dustin/p/m-c/obj-x86_64-pc-linux-gnu/_virtualenvs/m-c-ZJOly36j/bin/python
Installing setuptools, pip, wheel...done.

Virtualenv location: /home/dustin/p/m-c/obj-x86_64-pc-linux-gnu/_virtualenvs/m-c-ZJOly36j                                                                                                                                                                                                  running build_ext
copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil

Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
Build configuration changed. Regenerating backend.
No build detected, test metadata may be incomplete.
No handlers could be found for logger "mozbuild.frontend.reader"
 0:10.69 /home/dustin/p/m-c/taskcluster/taskgraph/test/test_util_taskcluster.py
 0:11.16 ============================= test session starts ==============================
 0:11.16 platform linux2 -- Python 2.7.12, pytest-3.6.2, py-1.5.4, pluggy-0.6.0 -- /home/dustin/p/m-c/obj-x86_64-pc-linux-gnu/_virtualenvs/m-c-ZJOly36j/bin/python
 0:11.16 rootdir: /home/dustin/p/m-c/taskcluster/taskgraph/test, inifile: /home/dustin/p/m-c/config/mozunit/mozunit/pytest.ini
 0:11.16 collecting ... collected 1 item
 0:11.17 
 0:11.17 taskcluster/taskgraph/test/test_util_taskcluster.py::TestTCUtils::test_parse_time PASSED [100%]
 0:11.17 
 0:11.17 =========================== 1 passed in 0.01 seconds ===========================
 0:11.21 Return code from mach python-test: 0

This occurs on re-runs locally even with no changes whatsoever.  Ideally it would only regenerate the virtualenv as necessary (since it's a bit slow), but at the least it would be great if it didn't ask permission to do so first.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Per :ahal's advice, setting PIPENV_TRUE=1 seems to help with the prompting.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

sorry, PIPENV_YES=1

Comment 3

[Andrew Halberstadt [:ahal]]

For context this is fallout from adding python 3 support to |mach python-test|. We decided to switch the |mach python-test| framework to use pipenv. Pipenv gives us nice benefits like lockfiles and being able to easily run with either python 2 or 3. But it's also very heavy weight for our needs. When pip has built-in support for lockfiles we'll likely move away from Pipenv, but for now it's the only way to get that feature.

I'm not sure why Pipenv needs to blow away the virtualenv every time. Dave, do you know if we can prevent that from happening somehow? At the very least we should set PIPENV_YES implicitly for people (though for some reason I don't have that set and am not seeing any prompt).

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

I did try deleting my objdir in case something was stale in there, but it did not help.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

It appears that if you let pipenv use the default version of Python (whatever this may be on your system) then it doesn't remove and recreate the virtual environment each time. We however are explicitly setting the Python version (or path) to use, which causes pipenv to recreate the virtual environment. It also seems that if the `VIRTUAL_ENV` environment variable is set, pipenv prompts because it could mean you're about to remove the virtual environment that is currently active.

The relevant code in pipenv is here:
https://github.com/pypa/pipenv/blob/3dc35330266c5ea0bc5266b3742067a2d959956d/pipenv/core.py#L570-L594

It could be that this is a bug in pipenv, and recreating the virtual environment should only happen when the version of Python has changed. Alternatively, we might be able to set `PIPENV_DEFAULT_PYTHON_VERSION` to the version we want to use instead of using `--python` when invoking pipenv. This will likely cause issues when switching between versions locally, though perhaps we could have another option when calling `mach python-test` to force recreation of the virtual environment.

I'll dig through the issues for pipenv and if I don't see anything related to this I'll open a new issue for discussion.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I've discovered that the `PIPENV_PYTHON` environment variable adds a suffix to the virtual environment path. This will allow us to have unique virtual environments for each Python version, and therefore we can avoid unecessarily creating the environments when they already exist. I'll provide a patch for review shortly.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Bug 1473727 - Avoid recreating virtual environment every time by using a unique environment for each Python version;

This patch uses the PIPENV_PYTHON environment variable to append a suffix to the created virtual environment path according to the version specified. It also uses the PIPENV_DEFAULT_PYTHON_VERSION environment variable to avoid recreating the virtual environment every time. With these changes we are able to switch back and forth between Python versions without the expense of recreating environments, however there is a risk of these environments becoming stale. In this scenario it may be necessary to clobber the virtual environment root within the obj dir.

Review commit: https://reviewboard.mozilla.org/r/255796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255796/

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8990709 [details]
Bug 1473727 - Avoid recreating virtual environment every time by using a unique environment for each Python version;

https://reviewboard.mozilla.org/r/255796/#review262550

I'm not familiar enough with pipenv to be confident I've spotted any issues here, but a read through the diff makes sense, and I can confirm it works well for me in my environment.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8990709 [details]
Bug 1473727 - Avoid recreating virtual environment every time by using a unique environment for each Python version;

https://reviewboard.mozilla.org/r/255796/#review262820

Thanks, lgtm.

When will this get stale for the python-test case? When modifying virtualenv_packages.txt?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d75218b99a04
Avoid recreating virtual environment every time by using a unique environment for each Python version; r=ahal,dustin

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8990709 [details]
Bug 1473727 - Avoid recreating virtual environment every time by using a unique environment for each Python version;

https://reviewboard.mozilla.org/r/255796/#review262820

Yes, if a package was removed from `virtualenv_packages.txt` then I wouldn't expect it to be removed from the virtual environment. This is probably a rare occurance, and it's more likely packages will be added or modified, which should be reflected in the environment.

Comment 12

[Andreea Pavel [:apavel]]

Backed out for build bustages

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d75218b99a04ba1f6c6df6278d1ec37879e8271d

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187393669&repo=autoland&lineNumber=48655

Backout: https://hg.mozilla.org/integration/autoland/rev/1df82d9ff1d445f07b68eeb234c30f73299d2319

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Sorry for that. I even pushed to try but somehow missed the bustage. I know what's caused this, and I have pushed a fix to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f2b11b8c121b8ef7eaa52136e6a582711bd9aed

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8990709 [details]
Bug 1473727 - Avoid recreating virtual environment every time by using a unique environment for each Python version;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255796/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2cd0e01b655
Avoid recreating virtual environment every time by using a unique environment for each Python version; r=ahal,dustin

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b2cd0e01b655