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

lib/spack/spack/cmd/solve.py

@@ -88,11 +88,11 @@ def solve(parser, args):
         'hashes': args.long or args.very_long
     }
 
-    # process dump options
+    # process output options
-    dump = re.split(r'\s*,\s*', args.show)
+    show = re.split(r'\s*,\s*', args.show)
-    if 'all' in dump:
+    if 'all' in show:
-        dump = show_options
+        show = show_options
-    for d in dump:
+    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
+    output = sys.stdout if "asp" in show else None
-    solver.models = models
+    result = solver.solve(
-    solver.timers = args.timers
+        specs,
-    solver.stats = args.stats
+        out=output,
-
+        models=models,
-    result = solver.solve(specs)
+        timers=args.timers,
-    if 'solutions' not in dump:
+        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
+    # show the solutions as concretized specs
-    if 'solutions' in dump:
+    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:")
 

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 make_concretization_repository(abstract_specs):
     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

lib/spack/spack/environment/environment.py

@@ -1083,7 +1083,7 @@ def is_develop(self, spec):
         """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 @@ def concretize(self, force=False, tests=False, reuse=None):
                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 @@ def concretize(self, force=False, tests=False, reuse=None):
 
         # 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 @@ def _concretize_together(self, tests=False, reuse=None):
         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 @@ def _concretize_separately(self, tests=False, reuse=None):
         ):
             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 _tree_to_display(spec):
         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):

lib/spack/spack/solver/asp.py

@@ -9,7 +9,6 @@
 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 @@ def fact(self, head, assumption=False):
             self.assumptions.append(atom)
 
     def solve(
-            self, solver_setup, specs, dump=None, nmodels=0,
+            self,
-            timers=False, stats=False,
+            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 @@ def solve(
         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()
+        self.driver = PyclingoDriver()
 
-    def set_default_configuration(self):
+        # These properties are settable via spack configuration, and overridable
-        # These properties are settable via spack configuration. `None`
+        # by setting them directly as properties.
-        # means go with the configuration setting; user can override.
+        self.reuse = spack.config.get("concretizer:reuse", False)
-        self.reuse = None
-
-        # these are concretizer settings
-        self.dump = ()
-        self.models = 0
-        self.timers = False
-        self.stats = False
-        self.tests = False
-
-    def solve(self, specs):
-        driver = PyclingoDriver()
-        if "asp" in self.dump:
-            driver.out = sys.stdout
 
+    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 @@ def solve(self, specs):
                     continue
                 spack.spec.Spec.ensure_valid_variants(s)
 
-        if self.reuse is None:
+        setup = SpackSolverSetup(reuse=self.reuse, tests=tests)
-            self.reuse = spack.config.get("concretizer:reuse", False)
+        return self.driver.solve(
-
+            setup,
-        setup = SpackSolverSetup(reuse=self.reuse, tests=self.tests)
+            specs,
-        return driver.solve(
+            nmodels=models,
-            setup, specs, self.dump, self.models, self.timers, self.stats
+            timers=timers,
+            stats=stats,
+            out=out,
+            setup_only=setup_only,
         )
 
 

lib/spack/spack/spec.py

@@ -2605,7 +2605,7 @@ def ensure_no_deprecated(root):
             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 @@ def _new_concretize(self, tests=False, reuse=None):
             return
 
         solver = spack.solver.asp.Solver()
-        solver.reuse = reuse
+        result = solver.solve([self], tests=tests)
-        solver.tests = tests
-
-        result = solver.solve([self])
         result.raise_if_unsat()
 
         # take the best answer
@@ -2637,23 +2634,17 @@ def _new_concretize(self, tests=False, reuse=None):
         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 @@ def _mark_concrete(self, value=True):
                 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 @@ def concretized(self, tests=False, reuse=None):
             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):

lib/spack/spack/test/concretize.py

@@ -1345,7 +1345,7 @@ def test_non_default_provider_of_multiple_virtuals(self):
         ('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 @@ def test_concrete_specs_are_not_modified_on_reuse(
         # 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)