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 change PATH when activating the test virtualenv #16614

Merged
merged 1 commit into from Oct 23, 2020

Conversation

mppf
Copy link
Member

@mppf mppf commented Oct 23, 2020

Follow-up to PR #16560

The python interoperability tests rely on finding cython and numpy.
For these tests, we assume that these libraries are installed system-wide.
However, the Python test scripts have their own virtualenv which,
after PR #16560, includes a python3 command for the virtualenv that
is first in PATH. The test system activates a virtualenv to have access
to packages that support testing (notably PyYAML for import yaml and
filelock for import filelock).

This PR adjusts the Python code that activates the test virtualenv to avoid
modifying PATH. That way, the Python modules are still available in the
test Python program itself. But, it does not interfere with which python3
is invoked in a subprocess. Python scripts that are created as subprocesses
during testing need to import activate_chpl_test_venv in order to have
access to any dependencies from the virtualenv.

  • full local testing
  • spot-checked a performance test
  • CHPL_TEST_ARKOUDA=true ./util/start_test test/studies/arkouda/ passes

Reviewed by @lydia-duncan - thanks!

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
@mppf mppf merged commit 6dfa331 into chapel-lang:master Oct 23, 2020
@mppf mppf deleted the activate_chpl_test_venv-no-PATH branch October 23, 2020 14:37
@ben-albrecht
Copy link
Member

This works, but seems fragile.

Rephrasing the problem, the main issue is that start_test has one set of python dependencies specified in test-venv and tests (such as Arkouda or Chapel-python-interop tests) have a different set of dependencies either specified in their own virtualenv or expected as system dependencies.

What we could do instead is deactivate the test virtualenv (test-venv) such that the test program runs in a clean environment, where it can source a new virtualenv or use system-installed dependencies.

Unfortunately, we are using test-venv/bin/activate_this.py because we activate test-venv from the system python rather than test-venv/bin/python, and virtualenv does not support a deactivate_this.py. So in order to deactivate, we'll need to implement that. There's an implementation provided in this SO post that may serve as a good starting point.

Once implemented, we could remove this fix and maybe change our test-venv-imports from this:

try:
    import activate_chpl_test_venv
    import filelock
except ImportError:
    pass

to this:

try:
    import activate_chpl_test_venv
    import filelock
    import dectivate_chpl_test_venv
except ImportError:
    pass

Note: we could do fancier things like a context manager or function to handle venv-dependent imports, but that feels like over-engineering at this point.

mppf added a commit that referenced this pull request Nov 3, 2020
Use python3 -m venv rather than virtualenv

Follow-up to PR #16560 and PR #16614 

This PR addresses a problem with `make docs` on Ubuntu 18.04 where we
were getting an error like

```
ERROR: Can not combine '--user' and '--prefix' as they imply different installation locations
```

in a `make docs` (when doing `make chpldoc-venv`).

To do so, the PR takes these general steps:
 * uses `python3 -m venv` rather than installing and running
   `virtualenv`, and as a result adds a dependency on python3-venv for
   `make docs`
 * changes the strategy for handling `start_test` venv dependencies.

Note that the change to `python3 -m venv` from `virtualenv` also changes
the order in which `pip` is used:
 * Before, we needed to make sure `pip` was available, and we downloaded
   it if not. Then, we used `pip` to install `virtualenv` and then from
   there used `pip` to install dependencies in the virtualenv.
 * Now, we run `python3 -m venv` to create the venv, and then we use
   `pip` to install dependencies in the venv. Since `python3 -m venv`
   will fail if `pip` is not available, from that point we can assume
   that `python3 -m pip` will work in the venv even if `pip` is not
   installed systemwide.

In more detail, the PR: 
* stops doing `--force-reinstall` on the `python3 -m pip` install
  commands. As far as I can tell, this was always a workaround (see
  51cace8) and doing a `touch`
  afterwards is a better workaround.
* stops running `virtualenv --relocateable` since `venv` has no analogue
  and the `venv-use-sys-python.py` script should handle it anyway
* installs `wheel` in the venv since I needed that to get the chpldocs
  dependencies to install for some reason
* removes version numbers in
  third-party/chpl-venv/sphinxcontrib-chapeldomain/setup.py - this is
  parallel to a change in
  chapel-lang/sphinxcontrib-chapeldomain#38 and
  solves a problem with `make docs`
* removes `util/test/activate_chpl_test_venv.py` in favor of making
  `start_test` a wrapper script that runs `start_test.py` in the venv. We
  had to change something here since moving away from `virtualenv` meant
  that `activate_this.py` is no longer installed. Instead of trying to
  add Python venv paths after the Python interpreter is running, the
  approach here is to run the Python code for `start_test` in a venv in
  the first place. This required some care in order to preserve #16614 -
  that is, the tested code and its prediffs need to use the system python
  rather than running in the venv. In particular, this PR takes the
  approach of removing all `PATH` components starting with `CHPL_HOME` in
  `start_test` and `sub_test`. This prevents the venv from interfering
  with tests and also makes the test environment more consistent. I have
  observed cases in the past where a paratest would fail but `start_test`
  would not for tests that assumed `CHPL_HOME/bin/linux64-x86` (e.g.) was
  in the `PATH`. In practice, the testing infrastructure uses absolute
  paths for things like the `chpl` command to invoke.
* Due to the `PATH` adjustments above, it was also necessary to make
  `sub_test` a wrapper script that runs `sub_test.py` in the venv.

Reviewed by @ben-albrecht and @ronawho - thanks!

- [x] full local futures testing
- [x] `make && make docs && make check-chpldoc` succeeds on:
  - [x] Ubuntu 20.04
  - [x] Ubuntu 18.04
  - [x] Fedora 32
  - [x] on a Cray XC with the system python
  - [x] on a Cray XC with a custom python

Future Work:
 * Since we `python3 -m venv` will fail if it could not install a `pip`
   to the virtualenv, we can remove getpip support and the check for pip.
   We can also simplify the `pip install` commands to run in the
   activated venv rather than adjusting environment variables in
   `third-party/chpl-venv/Makefile` targets.
 * Consider having `start_test` / `sub_test` call `deactivate` for each
   subprocess as an alternative to manipulating the `PATH` explicitly
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

3 participants