From 03acf14d86a4f3771fcb82b69ee673f1cc35b3c1 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sat, 7 Jan 2023 01:37:30 -0800 Subject: [PATCH] tests: test consistency of solver and spec_clauses Previously we didn't check whether all facts on a specs were represented in the models returned by the solver. This is an important check as it ensures that we can have conditions (dependencies, conflicts, etc.) on any property modeled by spec_clauses. This caught the issue reported in #32497 (https://github.com/spack/spack/issues/32497), specifically that we don't currently model patch facts. We can fix this in a follow-on but it is marked XFAIL for now. --- lib/spack/spack/cmd/solve.py | 2 +- lib/spack/spack/solver/asp.py | 40 ++++++----- lib/spack/spack/spec.py | 2 +- lib/spack/spack/test/solver/asp.py | 72 +++++++++++++++++++ .../builtin.mock/packages/openmpi/package.py | 2 + 5 files changed, 99 insertions(+), 19 deletions(-) create mode 100644 lib/spack/spack/test/solver/asp.py diff --git a/lib/spack/spack/cmd/solve.py b/lib/spack/spack/cmd/solve.py index 1c92591c303..accdeb6d05d 100644 --- a/lib/spack/spack/cmd/solve.py +++ b/lib/spack/spack/cmd/solve.py @@ -97,7 +97,7 @@ def setup_parser(subparser): def _process_result(result, show, required_format, kwargs): result.raise_if_unsat() - opt, _, _ = min(result.answers) + opt, *_ = min(result.answers) if ("opt" in show) and (not required_format): tty.msg("Best of %d considered solutions." % result.nmodels) tty.msg("Optimization Criteria:") diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 1dc3a09c6f5..22ec984c501 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -13,7 +13,7 @@ import re import types import warnings -from typing import Tuple +from typing import Tuple, Union import archspec.cpu @@ -240,7 +240,11 @@ def _id(thing): class AspFunction(AspObject): def __init__(self, name, args=None): self.name = name - self.args = () if args is None else tuple(args) + + def simplify(arg): + return arg if isinstance(arg, (str, bool, int)) else str(arg) + + self.args = () if args is None else tuple(simplify(arg) for arg in args) def _cmp_key(self): return (self.name, self.args) @@ -275,7 +279,7 @@ def argify(arg): elif isinstance(arg, int): return clingo.Number(arg) else: - return clingo.String(str(arg)) + return clingo.String(arg) return clingo.Function(self.name, [argify(arg) for arg in self.args], positive=positive) @@ -502,7 +506,8 @@ def _compute_specs_from_answer_set(self): self._concrete_specs, self._unsolved_specs = [], [] self._concrete_specs_by_input = {} best = min(self.answers) - opt, _, answer = best + + opt, _, answer, _ = best for input_spec in self.abstract_specs: key = input_spec.name if input_spec.virtual: @@ -773,7 +778,7 @@ def on_model(model): answers = builder.build_specs(spec_attrs) # add best spec to the results - result.answers.append((list(min_cost), 0, answers)) + result.answers.append((list(min_cost), 0, answers, spec_attrs)) # get optimization criteria criteria = extract_functions(best_model, "opt_criterion") @@ -1417,23 +1422,23 @@ def spec_clauses(self, *args, **kwargs): def _spec_clauses( self, spec, - body=False, - transitive=True, - expand_hashes=False, - concrete_build_deps=False, + body: bool = False, + transitive: bool = True, + expand_hashes: bool = False, + concrete_build_deps: bool = False, + deptype: Union[str, Tuple[str, ...]] = "all", ): """Return a list of clauses for a spec mandates are true. Arguments: - spec (spack.spec.Spec): the spec to analyze - body (bool): if True, generate clauses to be used in rule bodies + spec: the spec to analyze + body: if True, generate clauses to be used in rule bodies (final values) instead of rule heads (setters). - transitive (bool): if False, don't generate clauses from - dependencies (default True) - expand_hashes (bool): if True, descend into hashes of concrete specs - (default False) - concrete_build_deps (bool): if False, do not include pure build deps + transitive: if False, don't generate clauses from dependencies. + expand_hashes: If transitive and True, descend into hashes of concrete specs. + concrete_build_deps: if False, do not include pure build deps of concrete specs (as they have no effect on runtime constraints) + deptype: dependency types to follow when transitive (default "all"). Normally, if called with ``transitive=True``, ``spec_clauses()`` just generates hashes for the dependency requirements of concrete specs. If ``expand_hashes`` @@ -1561,7 +1566,7 @@ class Body(object): # add all clauses from dependencies if transitive: # TODO: Eventually distinguish 2 deps on the same pkg (build and link) - for dspec in spec.edges_to_dependencies(): + for dspec in spec.edges_to_dependencies(deptype=deptype): dep = dspec.spec if spec.concrete: @@ -1593,6 +1598,7 @@ class Body(object): body=body, expand_hashes=expand_hashes, concrete_build_deps=concrete_build_deps, + deptype=deptype, ) ) diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index ebaefbcf27a..30c06269013 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -2928,7 +2928,7 @@ def _new_concretize(self, tests=False): result.raise_if_unsat() # take the best answer - opt, i, answer = min(result.answers) + opt, i, answer, _ = min(result.answers) name = self.name # TODO: Consolidate this code with similar code in solve.py if self.virtual: diff --git a/lib/spack/spack/test/solver/asp.py b/lib/spack/spack/test/solver/asp.py new file mode 100644 index 00000000000..623ca4f8587 --- /dev/null +++ b/lib/spack/spack/test/solver/asp.py @@ -0,0 +1,72 @@ +# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) + +import itertools +import pytest + +import spack.spec +import spack.solver.asp as asp +import spack.store + + +pytestmark = [ + pytest.mark.skipif( + spack.config.get("config:concretizer") == "original", reason="requires new concretizer" + ), + pytest.mark.usefixtures("mutable_config", "mock_packages"), +] + + +@pytest.fixture +def reusable_specs(mock_packages): + reusable_specs = [] + for spec in ["mpich", "openmpi", "zmpi"]: + reusable_specs.extend(s for s in spack.spec.Spec(spec).concretized().traverse(root=True)) + return list(sorted(set(reusable_specs))) + + +@pytest.mark.parametrize( + "root,reuse", + itertools.product( + ("mpileaks ^mpich", "mpileaks ^openmpi", "mpileaks ^zmpi", "patch"), + (True, False), + ), +) +def test_all_facts_in_solve(database, root, reuse, reusable_specs): + reusable_specs = reusable_specs if reuse else [] + + solver = spack.solver.asp.Solver() + setup = spack.solver.asp.SpackSolverSetup() + result, _, _ = solver.driver.solve(setup, [spack.spec.Spec(root)], reuse=reusable_specs) + + *_, result_attrs = result.answers[0] + result_attrs = set(result_attrs) + + def remove_hashes(attrs): + return [] + + for spec in result.specs: + # check only link and run deps if reusing. + deptype = ("link", "run") if reuse else "all" + + # get all facts about the spec and filter out just the "attr" ones. + attrs = setup.spec_clauses(spec, deptype=deptype, body=True, expand_hashes=True) + + # only consider attr() functions, not other displayed atoms + # don't consider any DAG/package hashes, as they are added after solving + attrs = set(attr for attr in attrs if attr.name == "attr" and "hash" not in attr.args[0]) + + # make sure all facts from the solver are in the actual solution. + diff = attrs - result_attrs + + # this is a current bug in the solver: we don't manage dependency patches + # properly, and with reuse it can grab something w/o the right patch. + # See https://github.com/spack/spack/issues/32497 + # TODO: Remove this XFAIL when #32497 is fixed. + patches = [a for a in diff if a.args[0] == "variant_value" and a.args[2] == "patches"] + if diff and not (diff - set(patches)): + pytest.xfail("Bug in new concretizer with patch constraints. See #32497.") + + assert not diff diff --git a/var/spack/repos/builtin.mock/packages/openmpi/package.py b/var/spack/repos/builtin.mock/packages/openmpi/package.py index ea4cb42f7a1..a2fb5fdc90c 100644 --- a/var/spack/repos/builtin.mock/packages/openmpi/package.py +++ b/var/spack/repos/builtin.mock/packages/openmpi/package.py @@ -12,4 +12,6 @@ class Openmpi(Package): variant("internal-hwloc", default=False) variant("fabrics", values=any_combination_of("psm", "mxm")) + provides("mpi") + depends_on("hwloc", when="~internal-hwloc")