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.
pantsbuild · Apr 19, 2019 · e1d16a8 · e1d16a8
1 parent 36fdbd8
commit e1d16a8
Show file tree

Hide file tree

Showing 11 changed files with 42 additions and 90 deletions.
diff --git a/.gitignore b/.gitignore
@@ -53,5 +53,3 @@ GRTAGS
 GSYMS
 GTAGS
 .mypy_cache/
-# Stores the version of python last used to run pants. See ./pants.
-/.python-interpreter-constraints
diff --git a/.travis.yml b/.travis.yml
@@ -20,10 +20,6 @@ env:
   global:
     - PANTS_CONFIG_FILES="${TRAVIS_BUILD_DIR}/pants.travis-ci.ini"
     - LC_ALL="en_US.UTF-8"
-    # This tells the ./pants runner script to avoid trying to clean the workspace when changing
-    # python versions. CI starts off without the .python-interpreter-constraints file, so it would otherwise
-    # run a clean-all without this env var.
-    - ONLY_USING_SINGLE_PYTHON_VERSION='true'
     - BOOTSTRAPPED_PEX_BUCKET=ci-public.pantsbuild.org
     - BOOTSTRAPPED_PEX_KEY_PREFIX=${TRAVIS_BUILD_NUMBER}/${TRAVIS_BUILD_ID}/pants.pex
     - BOOTSTRAPPED_PEX_URL_PREFIX=s3://${BOOTSTRAPPED_PEX_BUCKET}/${BOOTSTRAPPED_PEX_KEY_PREFIX}

diff --git a/build-support/docker/travis_ci/Dockerfile b/build-support/docker/travis_ci/Dockerfile
@@ -28,11 +28,6 @@ USER ${TRAVIS_USER}:${TRAVIS_GROUP}
 # Our newly created user is unlikely to have a sane environment: set a locale at least.
 ENV LC_ALL="en_US.UTF-8"
 
-# This tells the ./pants runner script to avoid trying to clean the workspace when changing python
-# versions. CI starts off without the .python-interpreter-constraints file, so it would otherwise run a
-# clean-all without this env var.
-ENV ONLY_USING_SINGLE_PYTHON_VERSION 'true'
-
 # Expose the installed gcc to the invoking shell.
 ENTRYPOINT ["/usr/bin/scl", "enable", "devtoolset-7",  "--"]
 

diff --git a/build-support/docker/travis_ci_py27_ucs2/Dockerfile b/build-support/docker/travis_ci_py27_ucs2/Dockerfile
@@ -50,9 +50,4 @@ USER ${TRAVIS_USER}:${TRAVIS_GROUP}
 # Our newly created user is unlikely to have a sane environment: set a locale at least.
 ENV LC_ALL="en_US.UTF-8"
 
-# This tells the ./pants runner script to avoid trying to clean the workspace when changing python
-# versions. CI starts off without the .python-interpreter-constraints file, so it would otherwise run a
-# clean-all without this env var.
-ENV ONLY_USING_SINGLE_PYTHON_VERSION 'true'
-
 WORKDIR /travis/workdir
diff --git a/build-support/travis/travis.yml.mustache b/build-support/travis/travis.yml.mustache
@@ -13,10 +13,6 @@ env:
   global:
     - PANTS_CONFIG_FILES="${TRAVIS_BUILD_DIR}/pants.travis-ci.ini"
     - LC_ALL="en_US.UTF-8"
-    # This tells the ./pants runner script to avoid trying to clean the workspace when changing
-    # python versions. CI starts off without the .python-interpreter-constraints file, so it would otherwise
-    # run a clean-all without this env var.
-    - ONLY_USING_SINGLE_PYTHON_VERSION='true'
     - BOOTSTRAPPED_PEX_BUCKET=ci-public.pantsbuild.org
     - BOOTSTRAPPED_PEX_KEY_PREFIX=${TRAVIS_BUILD_NUMBER}/${TRAVIS_BUILD_ID}/pants.pex
     - BOOTSTRAPPED_PEX_URL_PREFIX=s3://${BOOTSTRAPPED_PEX_BUCKET}/${BOOTSTRAPPED_PEX_KEY_PREFIX}

diff --git a/pants b/pants
@@ -2,42 +2,6 @@
 # Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
-REPO_ROOT=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd -P)
-
-interpreter_constraints_sentinel_file_path='.python-interpreter-constraints'
-
-function clean_if_interpreter_constraints_changed() {
-  new_interpreter_constraints="$1"
-
-  if [[ -n "${recursive_pants_shell_script_invocation:-}" ]]; then
-    return 0
-  fi
-
-  interpreter_constraints_file="${REPO_ROOT}/${interpreter_constraints_sentinel_file_path}"
-
-  if [[ -f "${interpreter_constraints_file}" ]]; then
-    previous_interpreter_constraints="$(cat "${interpreter_constraints_file}")"
-    if [[ "${new_interpreter_constraints}" == "${previous_interpreter_constraints}" ]]; then
-      return 0
-    fi
-  else
-    cat >&2 <<EOF
-${interpreter_constraints_sentinel_file_path} not found in the buildroot.
-Forcing an initial clean-all.
-EOF
-  fi
-
-  export recursive_pants_shell_script_invocation='y'
-  cat >&2 <<EOF
-Different interpreter constraints were set than were used in the previous Pants run.
-Clearing the cache and preparing a Python environment with these interpreter constraints:
-${new_interpreter_constraints}
-EOF
-  ./pants clean-all
-  echo "${new_interpreter_constraints}" > "${interpreter_constraints_file}"
-  unset recursive_pants_shell_script_invocation
-}
-
 # This bootstrap script runs pants from the live sources in this repo.
 #
 # Further support is added for projects wrapping pants with custom external extensions.  In the
@@ -103,11 +67,6 @@ export PY="${PY:-python3}"
 py_major_minor_patch=$(${PY} -c 'import sys; print(".".join(map(str, sys.version_info[0:3])))')
 export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS:-['CPython==${py_major_minor_patch}']}"
 
-# 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}"
-fi
-
 PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"
 
 if [[ ! -z "${WRAPPER_REQUIREMENTS}" ]]; then

diff --git a/src/python/pants/backend/python/interpreter_cache.py b/src/python/pants/backend/python/interpreter_cache.py
@@ -46,13 +46,13 @@ def _matching(cls, interpreters, filters=()):
       if cls._matches(interpreter, filters=filters):
         yield interpreter
 
-  @memoized_property
-  def _python_setup(self):
+  @property
+  def python_setup(self):
     return PythonSetup.global_instance()
 
   @memoized_property
   def _cache_dir(self):
-    cache_dir = self._python_setup.interpreter_cache_dir
+    cache_dir = self.python_setup.interpreter_cache_dir
     safe_mkdir(cache_dir)
     return cache_dir
 
@@ -70,7 +70,7 @@ def partition_targets_by_compatibility(self, targets):
 
     for target in targets:
       if isinstance(target, PythonTarget):
-        c = self._python_setup.compatibility_or_constraints(target)
+        c = self.python_setup.compatibility_or_constraints(target)
         tgts_by_compatibilities[c].append(target)
         filters.update(c)
     return tgts_by_compatibilities, filters
@@ -150,8 +150,8 @@ def setup(self, filters=()):
     # We filter the interpreter cache itself (and not just the interpreters we pull from it)
     # because setting up some python versions (e.g., 3<=python<3.3) crashes, and this gives us
     # an escape hatch.
-    filters = filters if any(filters) else self._python_setup.interpreter_constraints
-    setup_paths = self._python_setup.interpreter_search_paths
+    filters = filters if any(filters) else self.python_setup.interpreter_constraints
+    setup_paths = self.python_setup.interpreter_search_paths
     logger.debug(
       'Initializing Python interpreter cache matching filters `{}` from paths `{}`'.format(
         ':'.join(filters), ':'.join(setup_paths)))

diff --git a/src/python/pants/backend/python/subsystems/python_setup.py b/src/python/pants/backend/python/subsystems/python_setup.py
@@ -26,7 +26,7 @@ class PythonSetup(Subsystem):
   def register_options(cls, register):
     super(PythonSetup, cls).register_options(register)
     register('--interpreter-constraints', advanced=True, default=['CPython>=2.7,<3', 'CPython>=3.6,<4'], type=list,
-             metavar='<requirement>',
+             metavar='<requirement>', fingerprint=True,
              help="Constrain the selected Python interpreter.  Specify with requirement syntax, "
                   "e.g. 'CPython>=2.7,<3' (A CPython interpreter with version >=2.7 AND version <3)"
                   "or 'PyPy' (A pypy interpreter of any version). Multiple constraint strings will "

diff --git a/src/python/pants/backend/python/tasks/select_interpreter.py b/src/python/pants/backend/python/tasks/select_interpreter.py
@@ -23,12 +23,13 @@
 
 class PythonInterpreterFingerprintStrategy(DefaultFingerprintHashingMixin, FingerprintStrategy):
 
+  def __init__(self, python_setup):
+    self.python_setup = python_setup
+
   def compute_fingerprint(self, python_target):
-    # Only consider the compatibility requirements in the fingerprint, as only
-    # those can affect the selected interpreter.
-    hash_elements_for_target = []
-    if python_target.compatibility:
-      hash_elements_for_target.extend(sorted(python_target.compatibility))
+    # Consider the target's compatibility requirements, and if those are missing then fall back
+    # to the global interpreter constraints. Only these two values can affect the selected interpreter.
+    hash_elements_for_target = sorted(self.python_setup.compatibility_or_constraints(python_target))
     if not hash_elements_for_target:
       return None
     hasher = hashlib.sha1()
@@ -44,7 +45,7 @@ class SelectInterpreter(Task):
   def implementation_version(cls):
     # TODO(John Sirois): Fixup this task to use VTS results_dirs. Right now version bumps aren't
     # effective in dealing with workdir data format changes.
-    return super(SelectInterpreter, cls).implementation_version() + [('SelectInterpreter', 3)]
+    return super(SelectInterpreter, cls).implementation_version() + [('SelectInterpreter', 4)]
 
   @classmethod
   def subsystem_dependencies(cls):
@@ -54,6 +55,10 @@ def subsystem_dependencies(cls):
   def product_types(cls):
     return [PythonInterpreter]
 
+  @property
+  def _interpreter_cache(self):
+    return PythonInterpreterCache.global_instance()
+
   def execute(self):
     # NB: Downstream product consumers may need the selected interpreter for use with
     # any type of importable Python target, including `PythonRequirementLibrary` targets
@@ -65,23 +70,23 @@ def execute(self):
     if not python_tgts_and_reqs:
       return
     python_tgts = [tgt for tgt in python_tgts_and_reqs if isinstance(tgt, PythonTarget)]
-    fs = PythonInterpreterFingerprintStrategy()
+    fs = PythonInterpreterFingerprintStrategy(python_setup=self._interpreter_cache.python_setup)
     with self.invalidated(python_tgts, fingerprint_strategy=fs) as invalidation_check:
-      # If there are no relevant targets, we still go through the motions of selecting
-      # an interpreter, to prevent downstream tasks from having to check for this special case.
-      if invalidation_check.all_vts:
-        target_set_id = VersionedTargetSet.from_versioned_targets(
-            invalidation_check.all_vts).cache_key.hash
-      else:
-        target_set_id = 'no_targets'
+      # If there are no constraints, meaning no global constraints nor compatibility requirements on
+      # the targets, we still go through the motions of selecting an interpreter, to prevent
+      # downstream tasks from having to check for this special case.
+      target_set_id = (
+        'no_constraints'
+        if not invalidation_check.all_vts else
+        VersionedTargetSet.from_versioned_targets(invalidation_check.all_vts).cache_key.hash
+      )
       interpreter_path_file = self._interpreter_path_file(target_set_id)
       interpreter = self._get_interpreter(interpreter_path_file, python_tgts)
 
     self.context.products.register_data(PythonInterpreter, interpreter)
 
   def _select_interpreter(self, interpreter_path_file, targets):
-    interpreter_cache = PythonInterpreterCache.global_instance()
-    interpreter = interpreter_cache.select_interpreter_for_targets(targets)
+    interpreter = self._interpreter_cache.select_interpreter_for_targets(targets)
     safe_mkdir_for(interpreter_path_file)
     with open(interpreter_path_file, 'w') as outfile:
       outfile.write('{}\n'.format(interpreter.binary))

diff --git a/tests/python/pants_test/backend/python/tasks/test_select_interpreter.py b/tests/python/pants_test/backend/python/tasks/test_select_interpreter.py
@@ -123,7 +123,7 @@ def test_interpreter_selection(self):
     self.assertIn('Unable to detect a suitable interpreter for compatibilities: '
                   'IronPython<2.99.999 && IronPython>2.88.888', str(cm.exception))
 
-  def test_interpreter_selection_invalidation(self):
+  def test_invalidation_for_target_constraints(self):
     tgta = self._fake_target('tgta', compatibility=['IronPython>2.77.777'],
                              dependencies=[self.tgt3])
     self.assertEqual('IronPython-2.99.999',
@@ -136,6 +136,19 @@ def test_interpreter_selection_invalidation(self):
     self.assertEqual('IronPython-2.99.999',
                       self._select_interpreter_and_get_version([tgtb], should_invalidate=False))
 
+  def test_invalidation_for_global_constraints(self):
+    # Because the system is setup with interpreter constraints, the task should
+    # invalidate on the first run.
+    self._select_interpreter_and_get_version([self.tgt1], should_invalidate=True)
+    self.set_options_for_scope(
+      PythonSetup.options_scope,
+      interpreter_constraints=RankedValue(RankedValue.CONFIG, ['IronPython>2.77.777'])
+    )
+    # After changing the global interpreter constraints, the task should invalidate.
+    self._select_interpreter_and_get_version([self.tgt1], should_invalidate=True)
+    # If the global constraints don't change, the task should not invalidate.
+    self._select_interpreter_and_get_version([self.tgt1], should_invalidate=False)
+
   def test_compatibility_AND(self):
     tgt = self._fake_target('tgt5', compatibility=['IronPython>2.77.777,<2.99.999'])
     self.assertEqual('IronPython-2.88.888', self._select_interpreter_and_get_version([tgt]))

diff --git a/tests/python/pants_test/pants_run_integration_test.py b/tests/python/pants_test/pants_run_integration_test.py
@@ -345,11 +345,6 @@ def run_pants_with_workdir_without_waiting(self, command, workdir, config=None,
     if extra_env:
       env.update(extra_env)
 
-    # This tells the ./pants runner script to avoid trying to clean the workspace when changing
-    # python versions. Since integration tests are run in a new buildroot, we don't need to do the
-    # clean-all.
-    env['ONLY_USING_SINGLE_PYTHON_VERSION'] = 'true'
-
     # Don't overwrite the profile of this process in the called process.
     # Instead, write the profile into a sibling file.
     if env.get('PANTS_PROFILE'):