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

Changing `--pants-python-setup-interpreter-constraints` does not invalidate Python interpreter selection #7510

Closed
Eric-Arellano opened this issue Apr 5, 2019 · 0 comments

Comments

Projects
None yet
1 participant
@Eric-Arellano
Copy link
Contributor

commented Apr 5, 2019

We would expect changing PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS to invalidate pyprep.interpreter so that the subprocess can pick up the new constraints, but it does not.

First disable our auto-clean-all logic by applying this diff:

diff --git a/pants b/pants
index 416d6e191..10a4500a3 100755
--- a/pants
+++ b/pants
@@ -105,7 +105,8 @@ export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRE

 # We can currently assume the CI environment does not switch python versions within a shard.
 if [[ "${ONLY_USING_SINGLE_PYTHON_VERSION:-false}" != 'true' ]]; then
-  clean_if_interpreter_constraints_changed "${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}"
+  :
+  # clean_if_interpreter_constraints_changed "${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}"
 fi

 PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"

Then run:

PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==3.6.*']" ./pants test --cache-ignore tests/python/pants_test/util:strutil

...
15:30:37 00:02   [pyprep]
15:30:37 00:02     [interpreter]
15:30:38 00:03     [build-local-dists]
15:30:38 00:03     [requirements]
15:30:38 00:03     [sources]
15:30:38 00:03   [test]
15:30:38 00:03     [test-jvm-prep-command]
15:30:38 00:03       [jvm_prep_command]
15:30:38 00:03     [test-prep-command]
15:30:38 00:03     [test]
15:30:38 00:03     [pytest-prep]
                   Invalidated 4 targets.
15:30:38 00:03     [pytest]
                   Invalidated 1 target.
                   scrubbed PYTHONPATH=/Users/eric/DocsLocal/code/projects/pants/src/python: from py.test environment
15:30:38 00:03       [run]
                     ============== test session starts ===============
                     platform darwin -- Python 3.6.8, pytest-3.6.4, py-1.7.0, pluggy-0.7.1

Then run:

$ PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==3.7.*']" ./pants test --cache-ignore tests/python/pants_test/util:strutil

...
15:30:49 00:02   [pyprep]
15:30:49 00:02     [interpreter]
15:30:49 00:02     [build-local-dists]
15:30:49 00:02     [requirements]
15:30:49 00:02     [sources]
15:30:49 00:02   [test]
15:30:49 00:02     [test-jvm-prep-command]
15:30:49 00:02       [jvm_prep_command]
15:30:49 00:02     [test-prep-command]
15:30:49 00:02     [test]
15:30:49 00:02     [pytest-prep]
                   Invalidated 4 targets.
15:30:50 00:03     [pytest]
                   Invalidated 1 target.
                   scrubbed PYTHONPATH=/Users/eric/DocsLocal/code/projects/pants/src/python: from py.test environment
15:30:50 00:03       [run]
                     ============== test session starts ===============
                     platform darwin -- Python 3.6.8, pytest-3.6.4, py-1.7.0, pluggy-0.7.1

To fix this, we likely need to change the fingerprint. John mentioned src/python/pants/backend/python/interpreter_cache.py and src/python/pants/backend/python/tasks/select_interpreter.py may be good starting points. We should also add an integration test.

Once this is fixed, we can revert #7151.

@Eric-Arellano Eric-Arellano added the python label Apr 5, 2019

@Eric-Arellano Eric-Arellano self-assigned this Apr 5, 2019

Eric-Arellano added a commit that referenced this issue Apr 19, 2019

Rerun `select-interpreter` if global Python interpreter constraints h…
…ave changed (#7586)

### Problem
Currently we only check for changes to the file's `compatibility` entry in its `BUILD` file, but really we should be also checking global constraints.

As it stands now, changing `--python-setup-interpreter-constraints` will have no impact until running `./pants clean-all`, which is not meant to be.

This closes #7510.

### Solution
Use `PythonSetup`'s `compatibility_or_constraints` in the fingerprint strategy, which first checks if the targets have `compatibility` marked in their BUILD entries, and then falls back to the global interpreter constraints.

#### Alternative solution: invalidate upon subsystem changes
Alternatively, we could add `self.fingerprint` to our file naming scheme, so that _any_ invalidation to `PythonSetup` or to `PythonInterpreterCache` would also invalidate this task. See d35e80c#diff-3d23a0b5af958441c095d5acd551ac16R101 for this implementation.

We did not go down this route because it would result in more invalidation than necessary. This task is only ever impacted by changes to `--python-setup-interpreter-constraints` and `compatibility` entries.

#### Also revert auto clean-all
#7151 and #7522 were workarounds to this underlying issue.

### Result
Calling `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==3.6.*']" ./pants test --cache-ignore tests/python/pants_test/util:strutil` the first time will cause an invalidation, then the interpreter selection task will not run upon subsequent invocations.

Running `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="['CPython==2.7.*']" ./pants test --cache-ignore tests/python/pants_test/util:strutil` right after will properly cause an invalidation and result in Python 2.7 being used for the tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.