concretizer: use only attr() for Spec attributes (#31202)

All Spec attributes are now represented as `attr(attribute_name, ... args ...)`, e.g.
`attr(node, "hdf5")` instead of `node("hdf5")`, as we *have* to maintain the `attr()`
form anyway, and it simplifies the encoding to just maintain one form of the Spec
information.

Background
----------

In #20644, we unified the way conditionals are done in the concretizer, but this
introduced a nasty aspect to the encoding: we have to maintain everything we want in
general conditions in two forms: `predicate(...)` and `attr("predicate", ...)`. For
example, here's the start of the table of spec attributes we had to maintain:

```prolog
node(Package)                      :- attr("node", Package).
virtual_node(Virtual)              :- attr("virtual_node", Virtual).
hash(Package, Hash)                :- attr("hash", Package, Hash).
version(Package, Version)          :- attr("version", Package, Version).
...
```

```prolog
attr("node", Package)              :- node(Package).
attr("virtual_node", Virtual)      :- virtual_node(Virtual).
attr("hash", Package, Hash)        :- hash(Package, Hash).
attr("version", Package, Version)  :- version(Package, Version).
...
```

This adds cognitive load to understanding how the concretizer works, as you have to
understand the equivalence between the two forms of spec attributes. It also makes the
general condition logic in #20644 hard to explain, and it's easy to forget to add a new
equivalence to this list when adding new spec attributes (at least two people have been
bitten by this).

Solution
--------

- [x] remove the equivalence list from `concretize.lp`
- [x] simplify `spec_clauses()`, `condition()`, and other functions in `asp.py` that need
      to deal with `Spec` attributes.
- [x] Convert all old-form spec attributes in `concretize.lp` to the `attr()` form
- [x] Simplify `display.lp`, where we also had to maintain a list of spec attributes. Now
      we only need to show `attr/2`, `attr/3`, and `attr/4`.
- [x] Simplify model extraction logic in `asp.py`.

Performance
-----------

This seems to result in a smaller grounded problem (as there are no longer duplicated
`attr("foo", ...)` / `foo(...)` predicates in the program), but it also adds a slight
performance overhead vs. develop. Ultimately, simplifying the encoding will be a win,
particularly for improving error messages.

Notes
-----

This will simplify future node refactors in `concretize.lp` (e.g., not identifying nodes
by package name, which we need for separate build dependencies).

I'm still not entirely used to reading `attr()` notation, but I thnk it's ultimately
clearer than what we did before. We need more uniform naming, and it's now clear what is
part of a solution. We should probably continue making the encoding of `concretize.lp`
simpler and more self-explanatory. It may make sense to rename `attr` to something like
`node_attr` and to simplify the names of node attributes. It also might make sense to do
something similar for other types of predicates in `concretize.lp`.

lib/spack/spack/cmd/diff.py

@@ -46,6 +46,14 @@ def setup_parser(subparser):
     )
 
 
+def shift(asp_function):
+    """Transforms ``attr("foo", "bar")`` into ``foo("bar")``."""
+    if not asp_function.args:
+        raise ValueError(f"Can't shift ASP function with no arguments: {str(asp_function)}")
+    first, *rest = asp_function.args
+    return asp.AspFunction(first, rest)
+
+
 def compare_specs(a, b, to_string=False, color=None):
     """
     Generate a comparison, including diffs (for each side) and an intersection.
@@ -71,22 +79,24 @@ def compare_specs(a, b, to_string=False, color=None):
     # get facts for specs, making sure to include build dependencies of concrete
     # specs and to descend into dependency hashes so we include all facts.
     a_facts = set(
-        t
+        shift(func)
-        for t in setup.spec_clauses(
+        for func in setup.spec_clauses(
             a,
             body=True,
             expand_hashes=True,
             concrete_build_deps=True,
         )
+        if func.name == "attr"
     )
     b_facts = set(
-        t
+        shift(func)
-        for t in setup.spec_clauses(
+        for func in setup.spec_clauses(
             b,
             body=True,
             expand_hashes=True,
             concrete_build_deps=True,
         )
+        if func.name == "attr"
     )
 
     # We want to present them to the user as simple key: values

lib/spack/spack/solver/asp.py

@@ -145,17 +145,14 @@ def getter(node):
 fixed_priority_offset = 100
 
 
-def build_criteria_names(costs, tuples):
+def build_criteria_names(costs, arg_tuples):
     """Construct an ordered mapping from criteria names to costs."""
     # pull optimization criteria names out of the solution
     priorities_names = []
 
     num_fixed = 0
     num_high_fixed = 0
-    for pred, args in tuples:
+    for args in arg_tuples:
-        if pred != "opt_criterion":
-            continue
-
         priority, name = args[:2]
         priority = int(priority)
 
@@ -255,13 +252,33 @@ def _id(thing):
 class AspFunction(AspObject):
     def __init__(self, name, args=None):
         self.name = name
-        self.args = () if args is None else args
+        self.args = () if args is None else tuple(args)
 
     def _cmp_key(self):
         return (self.name, self.args)
 
     def __call__(self, *args):
-        return AspFunction(self.name, args)
+        """Return a new instance of this function with added arguments.
+
+        Note that calls are additive, so you can do things like::
+
+            >>> attr = AspFunction("attr")
+            attr()
+
+            >>> attr("version")
+            attr("version")
+
+            >>> attr("version")("foo")
+            attr("version", "foo")
+
+            >>> v = AspFunction("attr", "version")
+            attr("version")
+
+            >>> v("foo", "bar")
+            attr("version", "foo", "bar")
+
+        """
+        return AspFunction(self.name, self.args + args)
 
     def symbol(self, positive=True):
         def argify(arg):
@@ -537,6 +554,36 @@ def bootstrap_clingo():
         from clingo import parse_files
 
 
+def stringify(sym):
+    """Stringify symbols from clingo models.
+
+    This will turn a ``clingo.Symbol`` into a string, or a sequence of ``clingo.Symbol``
+    objects into a tuple of strings.
+
+    """
+    # TODO: simplify this when we no longer have to support older clingo versions.
+    if isinstance(sym, (list, tuple)):
+        return tuple(stringify(a) for a in sym)
+
+    if clingo_cffi:
+        # Clingo w/ CFFI will throw an exception on failure
+        try:
+            return sym.string
+        except RuntimeError:
+            return str(sym)
+    else:
+        return sym.string or str(sym)
+
+
+def extract_args(model, predicate_name):
+    """Extract the arguments to predicates with the provided name from a model.
+
+    Pull out all the predicates with name ``predicate_name`` from the model, and return
+    their stringified arguments as tuples.
+    """
+    return [stringify(sym.arguments) for sym in model if sym.name == predicate_name]
+
+
 class PyclingoDriver(object):
     def __init__(self, cores=True):
         """Driver for the Python clingo interface.
@@ -592,6 +639,20 @@ def fact(self, head):
         if choice:
             self.assumptions.append(atom)
 
+    def handle_error(self, msg, *args):
+        """Handle an error state derived by the solver."""
+        msg = msg.format(*args)
+
+        # For variant formatting, we sometimes have to construct specs
+        # to format values properly. Find/replace all occurances of
+        # Spec(...) with the string representation of the spec mentioned
+        specs_to_construct = re.findall(r"Spec\(([^)]*)\)", msg)
+        for spec_str in specs_to_construct:
+            msg = msg.replace("Spec(%s)" % spec_str, str(spack.spec.Spec(spec_str)))
+
+        # TODO: this raises early -- we should handle multiple errors if there are any.
+        raise UnsatisfiableSpecError(msg)
+
     def solve(self, setup, specs, reuse=None, output=None, control=None):
         """Set up the input and solve for dependencies of ``specs``.
 
@@ -687,26 +748,27 @@ def on_model(model):
         # once done, construct the solve result
         result.satisfiable = solve_result.satisfiable
 
-        def stringify(x):
-            if clingo_cffi:
-                # Clingo w/ CFFI will throw an exception on failure
-                try:
-                    return x.string
-                except RuntimeError:
-                    return str(x)
-            else:
-                return x.string or str(x)
-
         if result.satisfiable:
-            # build spec from the best model
+            # get the best model
             builder = SpecBuilder(specs, hash_lookup=setup.reusable_and_possible)
             min_cost, best_model = min(models)
-            tuples = [(sym.name, [stringify(a) for a in sym.arguments]) for sym in best_model]
+
-            answers = builder.build_specs(tuples)
+            # first check for errors
+            error_args = extract_args(best_model, "error")
+            errors = sorted((int(priority), msg, args) for priority, msg, *args in error_args)
+            for _, msg, args in errors:
+                self.handle_error(msg, *args)
+
+            # build specs from spec attributes in the model
+            spec_attrs = [(name, tuple(rest)) for name, *rest in extract_args(best_model, "attr")]
+            answers = builder.build_specs(spec_attrs)
 
             # add best spec to the results
             result.answers.append((list(min_cost), 0, answers))
-            result.criteria = build_criteria_names(min_cost, tuples)
+
+            # get optimization criteria
+            criteria_args = extract_args(best_model, "opt_criterion")
+            result.criteria = build_criteria_names(min_cost, criteria_args)
 
             # record the number of models the solver considered
             result.nmodels = len(models)
@@ -714,6 +776,13 @@ def stringify(x):
             # record the possible dependencies in the solve
             result.possible_dependencies = setup.pkgs
 
+            # print any unknown functions in the model
+            for sym in best_model:
+                if sym.name not in ("attr", "error", "opt_criterion"):
+                    tty.debug(
+                        "UNKNOWN SYMBOL: %s(%s)" % (sym.name, ", ".join(stringify(sym.arguments)))
+                    )
+
         elif cores:
             result.control = self.control
             result.cores.extend(cores)
@@ -836,14 +905,14 @@ def spec_versions(self, spec):
         assert spec.name, msg
 
         if spec.concrete:
-            return [fn.version(spec.name, spec.version)]
+            return [fn.attr("version", spec.name, spec.version)]
 
         if spec.versions == spack.version.ver(":"):
             return []
 
         # record all version constraints for later
         self.version_constraints.add((spec.name, spec.versions))
-        return [fn.node_version_satisfies(spec.name, spec.versions)]
+        return [fn.attr("node_version_satisfies", spec.name, spec.versions)]
 
     def target_ranges(self, spec, single_target_fn):
         target = spec.architecture.target
@@ -853,7 +922,7 @@ def target_ranges(self, spec, single_target_fn):
             return [single_target_fn(spec.name, target)]
 
         self.target_constraints.add(target)
-        return [fn.node_target_satisfies(spec.name, target)]
+        return [fn.attr("node_target_satisfies", spec.name, target)]
 
     def conflict_rules(self, pkg):
         default_msg = "{0} '{1}' conflicts with '{2}'"
@@ -1097,7 +1166,7 @@ def condition(self, required_spec, imposed_spec=None, name=None, msg=None, node=
         # requirements trigger the condition
         requirements = self.spec_clauses(named_cond, body=True, required_from=name)
         for pred in requirements:
-            self.gen.fact(fn.condition_requirement(condition_id, pred.name, *pred.args))
+            self.gen.fact(fn.condition_requirement(condition_id, *pred.args))
 
         if imposed_spec:
             self.impose(condition_id, imposed_spec, node=node, name=name)
@@ -1108,9 +1177,9 @@ def impose(self, condition_id, imposed_spec, node=True, name=None, body=False):
         imposed_constraints = self.spec_clauses(imposed_spec, body=body, required_from=name)
         for pred in imposed_constraints:
             # imposed "node"-like conditions are no-ops
-            if not node and pred.name in ("node", "virtual_node"):
+            if not node and pred.args[0] in ("node", "virtual_node"):
                 continue
-            self.gen.fact(fn.imposed_constraint(condition_id, pred.name, *pred.args))
+            self.gen.fact(fn.imposed_constraint(condition_id, *pred.args))
 
     def package_provider_rules(self, pkg):
         for provider_name in sorted(set(s.name for s in pkg.provided.keys())):
@@ -1367,30 +1436,30 @@ def _spec_clauses(
 
         # TODO: do this with consistent suffixes.
         class Head(object):
-            node = fn.node
+            node = fn.attr("node")
-            virtual_node = fn.virtual_node
+            virtual_node = fn.attr("virtual_node")
-            node_platform = fn.node_platform_set
+            node_platform = fn.attr("node_platform_set")
-            node_os = fn.node_os_set
+            node_os = fn.attr("node_os_set")
-            node_target = fn.node_target_set
+            node_target = fn.attr("node_target_set")
-            variant_value = fn.variant_set
+            variant_value = fn.attr("variant_set")
-            node_compiler = fn.node_compiler_set
+            node_compiler = fn.attr("node_compiler_set")
-            node_compiler_version = fn.node_compiler_version_set
+            node_compiler_version = fn.attr("node_compiler_version_set")
-            node_flag = fn.node_flag_set
+            node_flag = fn.attr("node_flag_set")
-            node_flag_propagate = fn.node_flag_propagate
+            node_flag_propagate = fn.attr("node_flag_propagate")
-            variant_propagate = fn.variant_propagate
+            variant_propagate = fn.attr("variant_propagate")
 
         class Body(object):
-            node = fn.node
+            node = fn.attr("node")
-            virtual_node = fn.virtual_node
+            virtual_node = fn.attr("virtual_node")
-            node_platform = fn.node_platform
+            node_platform = fn.attr("node_platform")
-            node_os = fn.node_os
+            node_os = fn.attr("node_os")
-            node_target = fn.node_target
+            node_target = fn.attr("node_target")
-            variant_value = fn.variant_value
+            variant_value = fn.attr("variant_value")
-            node_compiler = fn.node_compiler
+            node_compiler = fn.attr("node_compiler")
-            node_compiler_version = fn.node_compiler_version
+            node_compiler_version = fn.attr("node_compiler_version")
-            node_flag = fn.node_flag
+            node_flag = fn.attr("node_flag")
-            node_flag_propagate = fn.node_flag_propagate
+            node_flag_propagate = fn.attr("node_flag_propagate")
-            variant_propagate = fn.variant_propagate
+            variant_propagate = fn.attr("variant_propagate")
 
         f = Body if body else Head
 
@@ -1457,8 +1526,11 @@ class Body(object):
 
             elif spec.compiler.versions:
                 clauses.append(
-                    fn.node_compiler_version_satisfies(
+                    fn.attr(
-                        spec.name, spec.compiler.name, spec.compiler.versions
+                        "node_compiler_version_satisfies",
+                        spec.name,
+                        spec.compiler.name,
+                        spec.compiler.versions,
                     )
                 )
                 self.compiler_version_constraints.add(spec.compiler)
@@ -1474,8 +1546,8 @@ class Body(object):
         if spec.concrete:
             # older specs do not have package hashes, so we have to do this carefully
             if getattr(spec, "_package_hash", None):
-                clauses.append(fn.package_hash(spec.name, spec._package_hash))
+                clauses.append(fn.attr("package_hash", spec.name, spec._package_hash))
-            clauses.append(fn.hash(spec.name, spec.dag_hash()))
+            clauses.append(fn.attr("hash", spec.name, spec.dag_hash()))
 
         # add all clauses from dependencies
         if transitive:
@@ -1489,18 +1561,18 @@ class Body(object):
                     for dtype in dspec.deptypes:
                         # skip build dependencies of already-installed specs
                         if concrete_build_deps or dtype != "build":
-                            clauses.append(fn.depends_on(spec.name, dep.name, dtype))
+                            clauses.append(fn.attr("depends_on", spec.name, dep.name, dtype))
 
                             # Ensure Spack will not coconcretize this with another provider
                             # for the same virtual
                             for virtual in dep.package.virtuals_provided:
-                                clauses.append(fn.virtual_node(virtual.name))
+                                clauses.append(fn.attr("virtual_node", virtual.name))
                                 clauses.append(fn.provider(dep.name, virtual.name))
 
                     # imposing hash constraints for all but pure build deps of
                     # already-installed concrete specs.
                     if concrete_build_deps or dspec.deptypes != ("build",):
-                        clauses.append(fn.hash(dep.name, dep.dag_hash()))
+                        clauses.append(fn.attr("hash", dep.name, dep.dag_hash()))
 
                 # if the spec is abstract, descend into dependencies.
                 # if it's concrete, then the hashes above take care of dependency
@@ -2057,12 +2129,13 @@ def literal_specs(self, specs):
             self.gen.h2("Spec: %s" % str(spec))
             self.gen.fact(fn.literal(idx))
 
-            root_fn = fn.virtual_root(spec.name) if spec.virtual else fn.root(spec.name)
+            self.gen.fact(fn.literal(idx, "virtual_root" if spec.virtual else "root", spec.name))
-            self.gen.fact(fn.literal(idx, root_fn.name, *root_fn.args))
             for clause in self.spec_clauses(spec):
-                self.gen.fact(fn.literal(idx, clause.name, *clause.args))
+                self.gen.fact(fn.literal(idx, *clause.args))
-                if clause.name == "variant_set":
+                if clause.args[0] == "variant_set":
-                    self.gen.fact(fn.literal(idx, "variant_default_value_from_cli", *clause.args))
+                    self.gen.fact(
+                        fn.literal(idx, "variant_default_value_from_cli", *clause.args[1:])
+                    )
 
         if self.concretize_everything:
             self.gen.fact(fn.concretize_everything())
@@ -2071,8 +2144,20 @@ def literal_specs(self, specs):
 class SpecBuilder(object):
     """Class with actions to rebuild a spec from ASP results."""
 
-    #: Attributes that don't need actions
+    #: Regex for attributes that don't need actions b/c they aren't used to construct specs.
-    ignored_attributes = ["opt_criterion"]
+    ignored_attributes = re.compile(
+        "|".join(
+            [
+                r"^.*_propagate$",
+                r"^.*_satisfies$",
+                r"^.*_set$",
+                r"^package_hash$",
+                r"^root$",
+                r"^virtual_node$",
+                r"^virtual_root$",
+            ]
+        )
+    )
 
     def __init__(self, specs, hash_lookup=None):
         self._specs = {}
@@ -2109,17 +2194,6 @@ def node_os(self, pkg, os):
     def node_target(self, pkg, target):
         self._arch(pkg).target = target
 
-    def error(self, priority, msg, *args):
-        msg = msg.format(*args)
-
-        # For variant formatting, we sometimes have to construct specs
-        # to format values properly. Find/replace all occurances of
-        # Spec(...) with the string representation of the spec mentioned
-        specs_to_construct = re.findall(r"Spec\(([^)]*)\)", msg)
-        for spec_str in specs_to_construct:
-            msg = msg.replace("Spec(%s)" % spec_str, str(spack.spec.Spec(spec_str)))
-        raise UnsatisfiableSpecError(msg)
-
     def variant_value(self, pkg, name, value):
         # FIXME: is there a way not to special case 'dev_path' everywhere?
         if name == "dev_path":
@@ -2244,10 +2318,7 @@ def deprecated(self, pkg, version):
     @staticmethod
     def sort_fn(function_tuple):
         name = function_tuple[0]
-        if name == "error":
+        if name == "hash":
-            priority = function_tuple[1][0]
-            return (-5, priority)
-        elif name == "hash":
             return (-4, 0)
         elif name == "node":
             return (-3, 0)
@@ -2262,18 +2333,18 @@ def build_specs(self, function_tuples):
         # Functions don't seem to be in particular order in output.  Sort
         # them here so that directives that build objects (like node and
         # node_compiler) are called in the right order.
-        self.function_tuples = function_tuples
+        self.function_tuples = sorted(set(function_tuples), key=self.sort_fn)
-        self.function_tuples.sort(key=self.sort_fn)
 
         self._specs = {}
-        for name, args in function_tuples:
+        for name, args in self.function_tuples:
-            if name in SpecBuilder.ignored_attributes:
+            if SpecBuilder.ignored_attributes.match(name):
                 continue
 
             action = getattr(self, name, None)
+
             # print out unknown actions so we can display them for debugging
             if not action:
-                msg = "%s(%s)" % (name, ", ".join(str(a) for a in args))
+                msg = 'UNKNOWN SYMBOL: attr("%s", %s)' % (name, ", ".join(str(a) for a in args))
                 tty.debug(msg)
                 continue
 

lib/spack/spack/solver/display.lp

@@ -9,33 +9,14 @@
 % This section determines what parts of the model are printed at the end
 %==============================================================================
 
-% Spec-related functions.
+% Spec attributes
-% Used to build the result of the solve.
+#show attr/2.
-#show node/1.
+#show attr/3.
-#show hash/2.
+#show attr/4.
-#show depends_on/3.
-#show version/2.
-#show variant_value/3.
-#show node_platform/2.
-#show node_os/2.
-#show node_target/2.
-#show node_compiler/2.
-#show node_compiler_version/3.
-#show node_flag/3.
-#show node_flag_compiler_default/1.
-#show node_flag_source/3.
-#show no_flags/2.
-#show external_spec_selected/2.
-#show version_equivalent/3.
-
-#show build/1.
 
 % names of optimization criteria
 #show opt_criterion/2.
 
-% deprecated packages
-#show deprecated/2.
-
 % error types
 #show error/2.
 #show error/3.