Make PythonInterpreterCache into a subsystem. (pantsbuild#6765)

It effectively depended on other subsystems (expecting instances to be passed into it), so it's a natural fit.

Switches it to use a local logger instead of one passed in from task context. The logger was only used for debug output anyway, and we want all such output to go through standard logging in the future anyway.

Also simplifies the interface of PythonInterpreterCache.setup(). Previously you could pass a list of paths to it, overriding its internal logic. However this was only used in tests, which can easily be converted to use the appropriate option instead.

This is the first step in simplifying the path-selection logic to be entirely option-based, instead of the current chain of ors that can't be fully overridden in options.

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

@@ -8,8 +8,6 @@
 from builtins import filter, open, str
 
 from pants.backend.python.interpreter_cache import PythonInterpreterCache
-from pants.backend.python.subsystems.python_repos import PythonRepos
-from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_binary import PythonBinary
 from pants.backend.python.targets.python_library import PythonLibrary
 from pants.backend.python.targets.python_target import PythonTarget
@@ -46,6 +44,10 @@ def register_options(cls, register):
   def supports_passthru_args(cls):
     return True
 
+  @classmethod
+  def subsystem_dependencies(cls):
+    return super(MypyTask, cls).subsystem_dependencies() + (PythonInterpreterCache,)
+
   def find_py3_interpreter(self):
     interpreters = self._interpreter_cache.setup(filters=['>=3'])
     return min(interpreters) if interpreters else None
@@ -82,9 +84,7 @@ def _collect_source_roots(self):
 
   @memoized_property
   def _interpreter_cache(self):
-    return PythonInterpreterCache(PythonSetup.global_instance(),
-                                  PythonRepos.global_instance(),
-                                  logger=self.context.log.debug)
+    return PythonInterpreterCache.global_instance()
 
   def _run_mypy(self, py3_interpreter, mypy_args, **kwargs):
     pex_info = PexInfo.default()

contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/checkstyle.py

@@ -58,8 +58,8 @@ def plugin_subsystems(cls):
   @classmethod
   def subsystem_dependencies(cls):
     return super(Task, cls).subsystem_dependencies() + cls.plugin_subsystems + (
-      # Needed implicitly by the pex_build_util functions we use.
-      PythonSetup, PythonRepos)
+      PythonSetup, PythonRepos, PythonInterpreterCache
+    )
 
   @classmethod
   def implementation_version(cls):
@@ -220,9 +220,7 @@ def execute(self):
     )
     if not python_tgts:
       return 0
-    interpreter_cache = PythonInterpreterCache(PythonSetup.global_instance(),
-                                               PythonRepos.global_instance(),
-                                               logger=self.context.log.debug)
+    interpreter_cache = PythonInterpreterCache.global_instance()
     with self.invalidated(self.get_targets(self._is_checked)) as invalidation_check:
       failure_count = 0
       tgts_by_compatibility, _ = interpreter_cache.partition_targets_by_compatibility(

contrib/python/src/python/pants/contrib/python/checks/tasks/python_eval.py

@@ -45,7 +45,9 @@ def __init__(self, *args, **kwargs):
 
   @classmethod
   def subsystem_dependencies(cls):
-    return super(PythonEval, cls).subsystem_dependencies() + (PythonRepos, PythonSetup)
+    return super(PythonEval, cls).subsystem_dependencies() + (
+      PythonRepos, PythonSetup, PythonInterpreterCache
+    )
 
   @classmethod
   def prepare(cls, options, round_manager):
@@ -72,9 +74,7 @@ def execute(self):
 
   @memoized_property
   def _interpreter_cache(self):
-    return PythonInterpreterCache(PythonSetup.global_instance(),
-                                  PythonRepos.global_instance(),
-                                  logger=self.context.log.debug)
+    return PythonInterpreterCache.global_instance()
 
   def _compile_targets(self, invalid_vts):
     with self.context.new_workunit(name='eval-targets', labels=[WorkUnitLabel.MULTITOOL]):

src/python/pants/backend/project_info/tasks/export.py

@@ -23,8 +23,6 @@
 from pants.backend.jvm.tasks.coursier_resolve import CoursierMixin
 from pants.backend.jvm.tasks.ivy_task_mixin import IvyTaskMixin
 from pants.backend.python.interpreter_cache import PythonInterpreterCache
-from pants.backend.python.subsystems.python_repos import PythonRepos
-from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
 from pants.backend.python.targets.python_target import PythonTarget
 from pants.backend.python.targets.python_tests import PythonTests
@@ -67,7 +65,7 @@ class ExportTask(ResolveRequirementsTaskBase, IvyTaskMixin, CoursierMixin):
   @classmethod
   def subsystem_dependencies(cls):
     return super(ExportTask, cls).subsystem_dependencies() + (
-      DistributionLocator, JvmPlatform, PythonSetup, PythonRepos
+      DistributionLocator, JvmPlatform, PythonInterpreterCache
     )
 
   class SourceRootTypes(object):
@@ -125,9 +123,7 @@ def prepare(cls, options, round_manager):
 
   @memoized_property
   def _interpreter_cache(self):
-    return PythonInterpreterCache(PythonSetup.global_instance(),
-                                  PythonRepos.global_instance(),
-                                  logger=self.context.log.debug)
+    return PythonInterpreterCache.global_instance()
 
   def check_artifact_cache_for(self, invalidation_check):
     # Export is an output dependent on the entire target set, and is not divisible

src/python/pants/backend/python/BUILD

@@ -45,6 +45,7 @@ python_library(
     'src/python/pants/backend/python/targets',
     'src/python/pants/base:exceptions',
     'src/python/pants/process',
+    'src/python/pants/subsystem',
     'src/python/pants/util:dirutil',
     'src/python/pants/util:memo',
   ]

src/python/pants/backend/python/interpreter_cache.py

@@ -4,23 +4,30 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
+import logging
 import os
 import shutil
-from builtins import map, object, str
+from builtins import map, str
 from collections import defaultdict
 
 from pex.interpreter import PythonInterpreter
 from pex.package import EggPackage, Package, SourcePackage
 from pex.resolver import resolve
 from pex.variables import Variables
 
+from pants.backend.python.subsystems.python_repos import PythonRepos
+from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_target import PythonTarget
 from pants.base.exceptions import TaskError
 from pants.process.lock import OwnerPrintingInterProcessFileLock
+from pants.subsystem.subsystem import Subsystem
 from pants.util.dirutil import safe_concurrent_creation, safe_mkdir
 from pants.util.memo import memoized_property
 
 
+logger = logging.getLogger(__name__)
+
+
 # TODO(wickman) Create a safer version of this and add to twitter.common.dirutil
 def _safe_link(src, dst):
   try:
@@ -30,7 +37,15 @@ def _safe_link(src, dst):
   os.symlink(src, dst)
 
 
-class PythonInterpreterCache(object):
+# TODO: Move under subsystems/ .
+class PythonInterpreterCache(Subsystem):
+  """Finds python interpreters on the local system."""
+  options_scope = 'python-interpreter-cache'
+
+  @classmethod
+  def subsystem_dependencies(cls):
+    return (super(PythonInterpreterCache, cls).subsystem_dependencies() +
+            (PythonSetup, PythonRepos))
 
   class UnsatisfiableInterpreterConstraintsError(TaskError):
     """Indicates a python interpreter matching given constraints could not be located."""
@@ -49,7 +64,7 @@ def _matching(cls, interpreters, filters=()):
   def pex_python_paths(cls):
     """A list of paths to Python interpreter binaries as defined by a
     PEX_PYTHON_PATH defined in either in '/etc/pexrc', '~/.pexrc'.
-    PEX_PYTHON_PATH defines a colon-seperated list of paths to interpreters
+    PEX_PYTHON_PATH defines a colon-separated list of paths to interpreters
     that a pex can be built and ran against.
 
     :return: paths to interpreters as specified by PEX_PYTHON_PATH
@@ -61,10 +76,13 @@ def pex_python_paths(cls):
     else:
       return []
 
-  def __init__(self, python_setup, python_repos, logger=None):
-    self._python_setup = python_setup
-    self._python_repos = python_repos
-    self._logger = logger or (lambda msg: True)
+  @memoized_property
+  def _python_setup(self):
+    return PythonSetup.global_instance()
+
+  @memoized_property
+  def _python_repos(self):
+    return PythonRepos.global_instance()
 
   @memoized_property
   def _cache_dir(self):
@@ -137,7 +155,7 @@ def _setup_cached(self, filters=()):
       if os.path.isdir(path):
         pi = self._interpreter_from_path(path, filters=filters)
         if pi:
-          self._logger('Detected interpreter {}: {}'.format(pi.binary, str(pi.identity)))
+          logger.debug('Detected interpreter {}: {}'.format(pi.binary, str(pi.identity)))
           yield pi
 
   def _setup_paths(self, paths, filters=()):
@@ -152,10 +170,9 @@ def _setup_paths(self, paths, filters=()):
       if pi:
         yield pi
 
-  def setup(self, paths=(), filters=()):
+  def setup(self, filters=()):
     """Sets up a cache of python interpreters.
 
-    :param paths: The paths to search for a python interpreter; the system ``PATH`` by default.
     :param filters: A sequence of strings that constrain the interpreter compatibility for this
       cache, using the Requirement-style format, e.g. ``'CPython>=3', or just ['>=2.7','<3']``
       for requirements agnostic to interpreter class.
@@ -166,11 +183,10 @@ def setup(self, paths=(), filters=()):
     # 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 = (paths
-                   or self.pex_python_paths()
+    setup_paths = (self.pex_python_paths()
                    or self._python_setup.interpreter_search_paths
                    or os.getenv('PATH').split(os.pathsep))
-    self._logger(
+    logger.debug(
       'Initializing Python interpreter cache matching filters `{}` from paths `{}`'.format(
         ':'.join(filters), ':'.join(setup_paths)))
 
@@ -184,13 +200,13 @@ def unsatisfied_filters(interpreters):
         interpreters.extend(self._setup_paths(setup_paths, filters=filters))
 
     for filt in unsatisfied_filters(interpreters):
-      self._logger('No valid interpreters found for {}!'.format(filt))
+      logger.debug('No valid interpreters found for {}!'.format(filt))
 
     matches = list(self._matching(interpreters, filters=filters))
     if len(matches) == 0:
-      self._logger('Found no valid interpreters!')
+      logger.debug('Found no valid interpreters!')
 
-    self._logger(
+    logger.debug(
       'Initialized Python interpreter cache with {}'.format(', '.join([x.binary for x in matches])))
     return matches
 
@@ -220,7 +236,7 @@ def _resolve_interpreter(self, interpreter, interpreter_dir, requirement):
     if bdist:
       return interpreter.with_extra(bdist.name, bdist.raw_version, bdist.path)
     else:
-      self._logger('Failed to resolve requirement {} for {}'.format(requirement, interpreter))
+      logger.debug('Failed to resolve requirement {} for {}'.format(requirement, interpreter))
 
   def _resolve_and_link(self, interpreter, requirement, target_link):
     # Short-circuit if there is a local copy.
@@ -252,5 +268,5 @@ def _resolve_and_link(self, interpreter, requirement, target_link):
     target_location = os.path.join(os.path.dirname(target_link), os.path.basename(dist_location))
     shutil.move(dist_location, target_location)
     _safe_link(target_location, target_link)
-    self._logger('    installed {}'.format(target_location))
+    logger.debug('    installed {}'.format(target_location))
     return Package.from_href(target_location)

src/python/pants/backend/python/tasks/select_interpreter.py

@@ -12,7 +12,6 @@
 from pex.interpreter import PythonInterpreter
 
 from pants.backend.python.interpreter_cache import PythonInterpreterCache
-from pants.backend.python.subsystems.python_repos import PythonRepos
 from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
 from pants.backend.python.targets.python_target import PythonTarget
@@ -49,7 +48,8 @@ def implementation_version(cls):
 
   @classmethod
   def subsystem_dependencies(cls):
-    return super(SelectInterpreter, cls).subsystem_dependencies() + (PythonSetup, PythonRepos)
+    return super(SelectInterpreter, cls).subsystem_dependencies() + (
+      PythonSetup, PythonInterpreterCache)
 
   @classmethod
   def product_types(cls):
@@ -88,9 +88,7 @@ def execute(self):
     self.context.products.register_data(PythonInterpreter, interpreter)
 
   def _create_interpreter_path_file(self, interpreter_path_file, targets):
-    interpreter_cache = PythonInterpreterCache(PythonSetup.global_instance(),
-                                               PythonRepos.global_instance(),
-                                               logger=self.context.log.debug)
+    interpreter_cache = PythonInterpreterCache.global_instance()
     interpreter = interpreter_cache.select_interpreter_for_targets(targets)
     safe_mkdir_for(interpreter_path_file)
     with open(interpreter_path_file, 'w') as outfile:

tests/python/pants_test/backend/codegen/thrift/python/test_apache_thrift_py_gen.py

@@ -16,7 +16,6 @@
 from pants.backend.codegen.thrift.python.python_thrift_library import PythonThriftLibrary
 from pants.backend.python.interpreter_cache import PythonInterpreterCache
 from pants.backend.python.subsystems.python_repos import PythonRepos
-from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_library import PythonLibrary
 from pants.base.build_environment import get_buildroot
 from pants.util.process_handler import subprocess
@@ -143,10 +142,9 @@ def test_namespace_effective(self):
     self.assertNotEqual(synthetic_target_one.target_base, synthetic_target_two.target_base)
 
     targets = (synthetic_target_one, synthetic_target_two)
-
-    python_repos = global_subsystem_instance(PythonRepos)
-    python_setup = global_subsystem_instance(PythonSetup)
-    interpreter_cache = PythonInterpreterCache(python_setup, python_repos)
+    self.context(for_subsystems=[PythonInterpreterCache, PythonRepos])
+    interpreter_cache = PythonInterpreterCache.global_instance()
+    python_repos = PythonRepos.global_instance()
     interpreter = interpreter_cache.select_interpreter_for_targets(targets)
 
     # We need setuptools to import namespace packages (via pkg_resources), so we prime the

tests/python/pants_test/backend/python/tasks/test_gather_sources.py

@@ -10,7 +10,6 @@
 from pex.interpreter import PythonInterpreter
 
 from pants.backend.python.interpreter_cache import PythonInterpreterCache
-from pants.backend.python.subsystems.python_repos import PythonRepos
 from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_library import PythonLibrary
 from pants.backend.python.tasks.gather_sources import GatherSources
@@ -108,18 +107,20 @@ def test_gather_files(self):
 
   def _gather_sources(self, target_roots):
     with temporary_dir() as cache_dir:
-      context = self.context(target_roots=target_roots,
-                             for_subsystems=[PythonSetup, PythonRepos],
-                             options={PythonSetup.options_scope: {'interpreter_cache_dir': cache_dir}})
-
-      # We must get an interpreter via the cache, instead of using PythonInterpreter.get() directly,
-      # to ensure that the interpreter has setuptools and wheel support.
       interpreter = PythonInterpreter.get()
-      interpreter_cache = PythonInterpreterCache(PythonSetup.global_instance(),
-                                                PythonRepos.global_instance(),
-                                                logger=context.log.debug)
-      interpreters = interpreter_cache.setup(paths=[os.path.dirname(interpreter.binary)],
-                                            filters=[str(interpreter.identity.requirement)])
+      context = self.context(target_roots=target_roots,
+                             for_subsystems=[PythonInterpreterCache],
+                             options={
+                               PythonSetup.options_scope: {
+                                 'interpreter_cache_dir': cache_dir,
+                                 'interpreter_search_paths': [os.path.dirname(interpreter.binary)],
+                               }})
+
+      # We must get an interpreter via the cache, instead of using the value of
+      # PythonInterpreter.get() directly, to ensure that the interpreter has setuptools and
+      # wheel support.
+      interpreter_cache = PythonInterpreterCache.global_instance()
+      interpreters = interpreter_cache.setup(filters=[str(interpreter.identity.requirement)])
       context.products.get_data(PythonInterpreter, lambda: interpreters[0])
 
       task = self.create_task(context)

tests/python/pants_test/backend/python/tasks/test_resolve_requirements.py

@@ -13,7 +13,6 @@
 
 from pants.backend.python.interpreter_cache import PythonInterpreterCache
 from pants.backend.python.python_requirement import PythonRequirement
-from pants.backend.python.subsystems.python_repos import PythonRepos
 from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
 from pants.backend.python.tasks.resolve_requirements import ResolveRequirements
@@ -130,18 +129,18 @@ def _fake_target(self, spec, requirement_strs):
   def _resolve_requirements(self, target_roots, options=None):
     with temporary_dir() as cache_dir:
       options = options or {}
-      options.setdefault(PythonSetup.options_scope, {})['interpreter_cache_dir'] = cache_dir
+      python_setup_opts = options.setdefault(PythonSetup.options_scope, {})
+      python_setup_opts['interpreter_cache_dir'] = cache_dir
+      interpreter = PythonInterpreter.get()
+      python_setup_opts['interpreter_search_paths'] = [os.path.dirname(interpreter.binary)]
       context = self.context(target_roots=target_roots, options=options,
-                             for_subsystems=[PythonSetup, PythonRepos])
+                             for_subsystems=[PythonInterpreterCache])
 
-      # We must get an interpreter via the cache, instead of using PythonInterpreter.get() directly,
-      # to ensure that the interpreter has setuptools and wheel support.
-      interpreter = PythonInterpreter.get()
-      interpreter_cache = PythonInterpreterCache(PythonSetup.global_instance(),
-                                                PythonRepos.global_instance(),
-                                                logger=context.log.debug)
-      interpreters = interpreter_cache.setup(paths=[os.path.dirname(interpreter.binary)],
-                                            filters=[str(interpreter.identity.requirement)])
+      # We must get an interpreter via the cache, instead of using the value of
+      # PythonInterpreter.get() directly, to ensure that the interpreter has setuptools and
+      # wheel support.
+      interpreter_cache = PythonInterpreterCache.global_instance()
+      interpreters = interpreter_cache.setup(filters=[str(interpreter.identity.requirement)])
       context.products.get_data(PythonInterpreter, lambda: interpreters[0])
 
       task = self.create_task(context)