concretizer: refactor argument passing for reuse

Reuse previously was a very invasive change that required parameters to be added to all
the methods that called `concretize()` on a `Spec` object. With the addition of
concretizer configuration, we can use the config system to simplify this argument
passing and keep the code cleaner.

We decided that concretizer config options should be read at `Solver` instantiation
time, and if config changes between instnatiation of a particular solver and
`solve()` invocation, the `Solver` should use the settings from `__init__()`.

- [x] remove `reuse` keyword argument from most concretize functions
- [x] refactor usages to use `spack.config.override("concretizer:reuse", True)`
- [x] rework argument passing in `Solver` so that parameters are set from config
      at instantiation time

6 files changed, 106 insertions, 107 deletions

diff --git a/lib/spack/spack/cmd/solve.py b/lib/spack/spack/cmd/solve.py
index 81d6ed5268..b3b9519ea5 100644
--- a/lib/spack/spack/cmd/solve.py
+++ b/lib/spack/spack/cmd/solve.py
@@ -88,11 +88,11 @@ def solve(parser, args):
         'hashes': args.long or args.very_long
     }
 
-    # process dump options
-    dump = re.split(r'\s*,\s*', args.show)
-    if 'all' in dump:
-        dump = show_options
-    for d in dump:
+    # process output options
+    show = re.split(r'\s*,\s*', args.show)
+    if 'all' in show:
+        show = show_options
+    for d in show:
         if d not in show_options:
             raise ValueError(
                 "Invalid option for '--show': '%s'\nchoose from: (%s)"
@@ -105,23 +105,28 @@ def solve(parser, args):
     specs = spack.cmd.parse_specs(args.specs)
 
     # set up solver parameters
+    # Note: reuse and other concretizer prefs are passed as configuration
     solver = asp.Solver()
-    solver.dump = dump
-    solver.models = models
-    solver.timers = args.timers
-    solver.stats = args.stats
-
-    result = solver.solve(specs)
-    if 'solutions' not in dump:
+    output = sys.stdout if "asp" in show else None
+    result = solver.solve(
+        specs,
+        out=output,
+        models=models,
+        timers=args.timers,
+        stats=args.stats,
+        setup_only=(set(show) == {'asp'})
+    )
+    if 'solutions' not in show:
         return
 
     # die if no solution was found
     result.raise_if_unsat()
 
-    # dump the solutions as concretized specs
-    if 'solutions' in dump:
+    # show the solutions as concretized specs
+    if 'solutions' in show:
         opt, _, _ = min(result.answers)
-        if ("opt" in dump) and (not args.format):
+
+        if ("opt" in show) and (not args.format):
             tty.msg("Best of %d considered solutions." % result.nmodels)
             tty.msg("Optimization Criteria:")
 
diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index de04fecffc..62a180436b 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -751,7 +751,6 @@ def _concretize_specs_together_new(*abstract_specs, **kwargs):
 
     solver = spack.solver.asp.Solver()
     solver.tests = kwargs.get('tests', False)
-    solver.reuse = kwargs.get('reuse', False)
 
     result = solver.solve(abstract_specs)
     result.raise_if_unsat()
@@ -789,15 +788,10 @@ def _concretize_specs_together_original(*abstract_specs, **kwargs):
     abstract_specs = [spack.spec.Spec(s) for s in abstract_specs]
     concretization_repository = make_concretization_repository(abstract_specs)
 
-    concretization_kwargs = {
-        'tests': kwargs.get('tests', False),
-        'reuse': kwargs.get('reuse', False)
-    }
-
     with spack.repo.additional_repository(concretization_repository):
         # Spec from a helper package that depends on all the abstract_specs
         concretization_root = spack.spec.Spec('concretizationroot')
-        concretization_root.concretize(**concretization_kwargs)
+        concretization_root.concretize(tests=kwargs.get("tests", False))
         # Retrieve the direct dependencies
         concrete_specs = [
             concretization_root[spec.name].copy() for spec in abstract_specs
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 55c8af97f7..e7f51c750a 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -1083,7 +1083,7 @@ class Environment(object):
         """Returns true when the spec is built from local sources"""
         return spec.name in self.dev_specs
 
-    def concretize(self, force=False, tests=False, reuse=None):
+    def concretize(self, force=False, tests=False):
         """Concretize user_specs in this environment.
 
         Only concretizes specs that haven't been concretized yet unless
@@ -1097,8 +1097,6 @@ class Environment(object):
                already concretized
             tests (bool or list or set): False to run no tests, True to test
                 all packages, or a list of package names to run tests for some
-            reuse (bool): if True try to maximize reuse of already installed
-                specs, if False don't account for installation status.
 
         Returns:
             List of specs that have been concretized. Each entry is a tuple of
@@ -1112,15 +1110,15 @@ class Environment(object):
 
         # Pick the right concretization strategy
         if self.concretization == 'together':
-            return self._concretize_together(tests=tests, reuse=reuse)
+            return self._concretize_together(tests=tests)
 
         if self.concretization == 'separately':
-            return self._concretize_separately(tests=tests, reuse=reuse)
+            return self._concretize_separately(tests=tests)
 
         msg = 'concretization strategy not implemented [{0}]'
         raise SpackEnvironmentError(msg.format(self.concretization))
 
-    def _concretize_together(self, tests=False, reuse=None):
+    def _concretize_together(self, tests=False):
         """Concretization strategy that concretizes all the specs
         in the same DAG.
         """
@@ -1153,14 +1151,14 @@ class Environment(object):
         self.specs_by_hash = {}
 
         concrete_specs = spack.concretize.concretize_specs_together(
-            *self.user_specs, tests=tests, reuse=reuse
+            *self.user_specs, tests=tests
         )
         concretized_specs = [x for x in zip(self.user_specs, concrete_specs)]
         for abstract, concrete in concretized_specs:
             self._add_concrete_spec(abstract, concrete)
         return concretized_specs
 
-    def _concretize_separately(self, tests=False, reuse=None):
+    def _concretize_separately(self, tests=False):
         """Concretization strategy that concretizes separately one
         user spec after the other.
         """
@@ -1185,7 +1183,7 @@ class Environment(object):
         ):
             if uspec not in old_concretized_user_specs:
                 root_specs.append(uspec)
-                arguments.append((uspec_constraints, tests, reuse))
+                arguments.append((uspec_constraints, tests))
 
         # Ensure we don't try to bootstrap clingo in parallel
         if spack.config.get('config:concretizer') == 'clingo':
@@ -2009,7 +2007,7 @@ def display_specs(concretized_specs):
         print('')
 
 
-def _concretize_from_constraints(spec_constraints, tests=False, reuse=None):
+def _concretize_from_constraints(spec_constraints, tests=False):
     # Accept only valid constraints from list and concretize spec
     # Get the named spec even if out of order
     root_spec = [s for s in spec_constraints if s.name]
@@ -2028,7 +2026,7 @@ def _concretize_from_constraints(spec_constraints, tests=False, reuse=None):
             if c not in invalid_constraints:
                 s.constrain(c)
         try:
-            return s.concretized(tests=tests, reuse=reuse)
+            return s.concretized(tests=tests)
         except spack.spec.InvalidDependencyError as e:
             invalid_deps_string = ['^' + d for d in e.invalid_deps]
             invalid_deps = [c for c in spec_constraints
@@ -2048,9 +2046,9 @@ def _concretize_from_constraints(spec_constraints, tests=False, reuse=None):
 
 
 def _concretize_task(packed_arguments):
-    spec_constraints, tests, reuse = packed_arguments
+    spec_constraints, tests = packed_arguments
     with tty.SuppressOutput(msg_enabled=False):
-        return _concretize_from_constraints(spec_constraints, tests, reuse)
+        return _concretize_from_constraints(spec_constraints, tests)
 
 
 def make_repo_path(root):
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 1403077997..0df83191a4 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -9,7 +9,6 @@ import copy
 import itertools
 import os
 import pprint
-import sys
 import types
 import warnings
 
@@ -474,18 +473,16 @@ def bootstrap_clingo():
 
 
 class PyclingoDriver(object):
-    def __init__(self, cores=True, asp=None):
+    def __init__(self, cores=True):
         """Driver for the Python clingo interface.
 
         Arguments:
             cores (bool): whether to generate unsatisfiable cores for better
                 error reporting.
-            asp (file-like): optional stream to write a text-based ASP program
-                for debugging or verification.
         """
         bootstrap_clingo()
 
-        self.out = asp or llnl.util.lang.Devnull()
+        self.out = llnl.util.lang.Devnull()
         self.cores = cores
 
     def title(self, name, char):
@@ -528,9 +525,30 @@ class PyclingoDriver(object):
             self.assumptions.append(atom)
 
     def solve(
-            self, solver_setup, specs, dump=None, nmodels=0,
-            timers=False, stats=False,
+            self,
+            setup,
+            specs,
+            nmodels=0,
+            timers=False,
+            stats=False,
+            out=None,
+            setup_only=False
     ):
+        """Set up the input and solve for dependencies of ``specs``.
+
+        Arguments:
+          setup (SpackSolverSetup): An object to set up the ASP problem.
+          specs (list): List of ``Spec`` objects to solve for.
+          nmodels (list): Number of models to consider (default 0 for unlimited).
+          timers (bool):  Print out coarse timers for different solve phases.
+          stats (bool): Whether to output Clingo's internal solver statistics.
+          out: Optional output stream for the generated ASP program.
+          setup_only (bool): if True, stop after setup and don't solve (default False).
+        """
+        # allow solve method to override the output stream
+        if out is not None:
+            self.out = out
+
         timer = spack.util.timer.Timer()
 
         # Initialize the control object for the solver
@@ -545,9 +563,13 @@ class PyclingoDriver(object):
         self.assumptions = []
         with self.control.backend() as backend:
             self.backend = backend
-            solver_setup.setup(self, specs)
+            setup.setup(self, specs)
         timer.phase("setup")
 
+        # If we're only doing setup, just return an empty solve result
+        if setup_only:
+            return Result(specs)
+
         # read in the main ASP program and display logic -- these are
         # handwritten, not generated, so we load them as resources
         parent_dir = os.path.dirname(__file__)
@@ -2033,49 +2055,36 @@ class Solver(object):
       ``reuse (bool)``
         Whether to try to reuse existing installs/binaries
 
-      ``show (tuple)``
-        What information to print to the console while running. Options are:
-        * asp: asp program text
-        * opt: optimization criteria for best model
-        * output: raw clingo output
-        * solutions: models found by asp program
-        * all: all of the above
-
-      ``models (int)``
-        Number of models to search (default: 0 for unlimited)
-
-      ``timers (bool)``
-        Print out coarse fimers for different solve phases.
-
-      ``stats (bool)``
-        Print out detailed stats from clingo
-
-      ``tests (bool or tuple)``
-        If ``True``, concretize test dependencies for all packages. If
-        a tuple of package names, concretize test dependencies for named
-        packages. If ``False``, do not concretize test dependencies.
-
     """
     def __init__(self):
-        self.set_default_configuration()
-
-    def set_default_configuration(self):
-        # These properties are settable via spack configuration. `None`
-        # means go with the configuration setting; user can override.
-        self.reuse = None
-
-        # these are concretizer settings
-        self.dump = ()
-        self.models = 0
-        self.timers = False
-        self.stats = False
-        self.tests = False
+        self.driver = PyclingoDriver()
 
-    def solve(self, specs):
-        driver = PyclingoDriver()
-        if "asp" in self.dump:
-            driver.out = sys.stdout
+        # These properties are settable via spack configuration, and overridable
+        # by setting them directly as properties.
+        self.reuse = spack.config.get("concretizer:reuse", False)
 
+    def solve(
+            self,
+            specs,
+            out=None,
+            models=0,
+            timers=False,
+            stats=False,
+            tests=False,
+            setup_only=False,
+    ):
+        """
+        Arguments:
+          specs (list): List of ``Spec`` objects to solve for.
+          out: Optionally write the generate ASP program to a file-like object.
+          models (int): Number of models to search (default: 0 for unlimited).
+          timers (bool): Print out coarse fimers for different solve phases.
+          stats (bool): Print out detailed stats from clingo.
+          tests (bool or tuple): If True, concretize test dependencies for all packages.
+            If a tuple of package names, concretize test dependencies for named
+            packages (defaults to False: do not concretize test dependencies).
+          setup_only (bool): if True, stop after setup and don't solve (default False).
+        """
         # Check upfront that the variants are admissible
         for root in specs:
             for s in root.traverse():
@@ -2083,12 +2092,15 @@ class Solver(object):
                     continue
                 spack.spec.Spec.ensure_valid_variants(s)
 
-        if self.reuse is None:
-            self.reuse = spack.config.get("concretizer:reuse", False)
-
-        setup = SpackSolverSetup(reuse=self.reuse, tests=self.tests)
-        return driver.solve(
-            setup, specs, self.dump, self.models, self.timers, self.stats
+        setup = SpackSolverSetup(reuse=self.reuse, tests=tests)
+        return self.driver.solve(
+            setup,
+            specs,
+            nmodels=models,
+            timers=timers,
+            stats=stats,
+            out=out,
+            setup_only=setup_only,
         )
 
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 7a918cc0c5..bbcad6f746 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2605,7 +2605,7 @@ class Spec(object):
             msg += "    For each package listed, choose another spec\n"
             raise SpecDeprecatedError(msg)
 
-    def _new_concretize(self, tests=False, reuse=None):
+    def _new_concretize(self, tests=False):
         import spack.solver.asp
 
         if not self.name:
@@ -2616,10 +2616,7 @@ class Spec(object):
             return
 
         solver = spack.solver.asp.Solver()
-        solver.reuse = reuse
-        solver.tests = tests
-
-        result = solver.solve([self])
+        result = solver.solve([self], tests=tests)
         result.raise_if_unsat()
 
         # take the best answer
@@ -2637,23 +2634,17 @@ class Spec(object):
         self._dup(concretized)
         self._mark_concrete()
 
-    def concretize(self, tests=False, reuse=None):
+    def concretize(self, tests=False):
         """Concretize the current spec.
 
         Args:
             tests (bool or list): if False disregard 'test' dependencies,
                 if a list of names activate them for the packages in the list,
                 if True activate 'test' dependencies for all packages.
-            reuse (bool): if True try to maximize reuse of already installed
-                specs, if False don't account for installation status.
         """
         if spack.config.get('config:concretizer') == "clingo":
-            self._new_concretize(tests, reuse=reuse)
+            self._new_concretize(tests)
         else:
-            if reuse:
-                msg = ('maximizing reuse of installed specs is not '
-                       'possible with the original concretizer')
-                raise spack.error.SpecError(msg)
             self._old_concretize(tests)
 
     def _mark_root_concrete(self, value=True):
@@ -2678,7 +2669,7 @@ class Spec(object):
                 s.clear_cached_hashes()
             s._mark_root_concrete(value)
 
-    def concretized(self, tests=False, reuse=None):
+    def concretized(self, tests=False):
         """This is a non-destructive version of concretize().
 
         First clones, then returns a concrete version of this package
@@ -2688,11 +2679,9 @@ class Spec(object):
             tests (bool or list): if False disregard 'test' dependencies,
                 if a list of names activate them for the packages in the list,
                 if True activate 'test' dependencies for all packages.
-            reuse (bool): if True try to maximize reuse of already installed
-                specs, if False don't account for installation status.
         """
         clone = self.copy(caches=True)
-        clone.concretize(tests=tests, reuse=reuse)
+        clone.concretize(tests=tests)
         return clone
 
     def flat_dependencies(self, **kwargs):
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index fae852edf8..e08ef6b6aa 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -1345,7 +1345,7 @@ class TestConcretize(object):
         ('mpich~debug', True)
     ])
     def test_concrete_specs_are_not_modified_on_reuse(
-            self, mutable_database, spec_str, expect_installed
+            self, mutable_database, spec_str, expect_installed, config
     ):
         if spack.config.get('config:concretizer') == 'original':
             pytest.skip('Original concretizer cannot reuse specs')
@@ -1354,7 +1354,8 @@ class TestConcretize(object):
         # when reused specs are added to the mix. This prevents things
         # like additional constraints being added to concrete specs in
         # the answer set produced by clingo.
-        s = spack.spec.Spec(spec_str).concretized(reuse=True)
+        with spack.config.override("concretizer:reuse", True):
+            s = spack.spec.Spec(spec_str).concretized()
         assert s.package.installed is expect_installed
         assert s.satisfies(spec_str, strict=True)