parse_specs: unify specs based on concretizer:unify (#44843)

Currently, the `concretizer:unify:` config option only affects environments.

With this PR, it now affects any group of specs given to a command using the `parse_specs(*, concretize=True)` interface.

- [x] implementation in `parse_specs`
- [x] tests
- [x] ensure all commands that accept multiple specs and concretize use `parse_specs` interface

---------

Co-authored-by: Todd Gamblin <tgamblin@llnl.gov>
Signed-off-by: Todd Gamblin <tgamblin@llnl.gov>

lib/spack/spack/cmd/__init__.py

@@ -173,11 +173,30 @@ def parse_specs(
     arg_string = " ".join([quote_kvp(arg) for arg in args])
 
     specs = spack.parser.parse(arg_string)
-    for spec in specs:
+    if not concretize:
-        if concretize:
-            spec.concretize(tests=tests)
         return specs
 
+    to_concretize = [(s, None) for s in specs]
+    return _concretize_spec_pairs(to_concretize, tests=tests)
+
+
+def _concretize_spec_pairs(to_concretize, tests=False):
+    """Helper method that concretizes abstract specs from a list of abstract,concrete pairs.
+
+    Any spec with a concrete spec associated with it will concretize to that spec. Any spec
+    with ``None`` for its concrete spec will be newly concretized. This method respects unification
+    rules from config."""
+    unify = spack.config.get("concretizer:unify", False)
+
+    concretize_method = spack.concretize.concretize_separately  # unify: false
+    if unify is True:
+        concretize_method = spack.concretize.concretize_together
+    elif unify == "when_possible":
+        concretize_method = spack.concretize.concretize_together_when_possible
+
+    concretized = concretize_method(*to_concretize, tests=tests)
+    return [concrete for _, concrete in concretized]
+
 
 def matching_spec_from_env(spec):
     """
@@ -192,6 +211,22 @@ def matching_spec_from_env(spec):
         return spec.concretized()
 
 
+def matching_specs_from_env(specs):
+    """
+    Same as ``matching_spec_from_env`` but respects spec unification rules.
+
+    For each spec, if there is a matching spec in the environment it is used. If no
+    matching spec is found, this will return the given spec but concretized in the
+    context of the active environment and other given specs, with unification rules applied.
+    """
+    env = ev.active_environment()
+    spec_pairs = [(spec, env.matching_spec(spec) if env else None) for spec in specs]
+    additional_concrete_specs = (
+        [(concrete, concrete) for _, concrete in env.concretized_specs()] if env else []
+    )
+    return _concretize_spec_pairs(spec_pairs + additional_concrete_specs)[: len(spec_pairs)]
+
+
 def disambiguate_spec(spec, env, local=False, installed=True, first=False):
     """Given a spec, figure out which installed package it refers to.
 

lib/spack/spack/cmd/clean.py

@@ -105,7 +105,8 @@ def clean(parser, args):
     # Then do the cleaning falling through the cases
     if args.specs:
         specs = spack.cmd.parse_specs(args.specs, concretize=False)
-        specs = list(spack.cmd.matching_spec_from_env(x) for x in specs)
+        specs = spack.cmd.matching_specs_from_env(specs)
+
         for spec in specs:
             msg = "Cleaning build stage [{0}]"
             tty.msg(msg.format(spec.short_spec))

lib/spack/spack/cmd/patch.py

@@ -33,8 +33,9 @@ def patch(parser, args):
         spack.config.set("config:checksum", False, scope="command_line")
 
     specs = spack.cmd.parse_specs(args.specs, concretize=False)
+    specs = spack.cmd.matching_specs_from_env(specs)
     for spec in specs:
-        _patch(spack.cmd.matching_spec_from_env(spec).package)
+        _patch(spec.package)
 
 
 def _patch_env(env: ev.Environment):

lib/spack/spack/cmd/stage.py

@@ -47,8 +47,8 @@ def stage(parser, args):
     if len(specs) > 1 and custom_path:
         tty.die("`--path` requires a single spec, but multiple were provided")
 
+    specs = spack.cmd.matching_specs_from_env(specs)
     for spec in specs:
-        spec = spack.cmd.matching_spec_from_env(spec)
         pkg = spec.package
 
         if custom_path:

lib/spack/spack/concretize.py

@@ -5,11 +5,17 @@
 """
 (DEPRECATED) Used to contain the code for the original concretizer
 """
+import sys
+import time
 from contextlib import contextmanager
 from itertools import chain
+from typing import Tuple
+
+import llnl.util.tty as tty
 
 import spack.config
 import spack.error
+from spack.spec import Spec
 
 CHECK_COMPILER_EXISTENCE = True
 
@@ -83,6 +89,140 @@ def concretize_specs_together(*abstract_specs, **kwargs):
     return [s.copy() for s in result.specs]
 
 
+def concretize_together(*spec_list, **kwargs):
+    """Given a number of specs as input, tries to concretize them together.
+
+    Args:
+        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
+        *spec_list: list of tuples to concretize. First entry is abstract spec, second entry is
+            already concrete spec or None if not yet concretized
+
+    Returns:
+        List of tuples of abstract and concretized specs
+    """
+    to_concretize = [concrete if concrete else abstract for abstract, concrete in spec_list]
+    abstract_specs = [abstract for abstract, _ in spec_list]
+    concrete_specs = concretize_specs_together(*to_concretize, **kwargs)
+    return list(zip(abstract_specs, concrete_specs))
+
+
+def concretize_together_when_possible(*spec_list, **kwargs):
+    """Given a number of specs as input, tries to concretize them together to the extent possible.
+
+    See documentation for ``unify: when_possible`` concretization for the precise definition of
+    "to the extent possible".
+
+    Args:
+        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
+        *spec_list: list of tuples to concretize. First entry is abstract spec, second entry is
+            already concrete spec or None if not yet concretized
+
+    Returns:
+        List of tuples of abstract and concretized specs
+    """
+    to_concretize = [concrete if concrete else abstract for abstract, concrete in spec_list]
+    old_concrete_to_abstract = {
+        concrete: abstract for (abstract, concrete) in spec_list if concrete
+    }
+
+    result_by_user_spec = {}
+    solver = spack.solver.asp.Solver()
+    allow_deprecated = spack.config.get("config:deprecated", False)
+    for result in solver.solve_in_rounds(
+        to_concretize, tests=kwargs.get("tests", False), allow_deprecated=allow_deprecated
+    ):
+        result_by_user_spec.update(result.specs_by_input)
+
+    # If the "abstract" spec is a concrete spec from the previous concretization
+    # translate it back to an abstract spec. Otherwise, keep the abstract spec
+    return [
+        (old_concrete_to_abstract.get(abstract, abstract), concrete)
+        for abstract, concrete in sorted(result_by_user_spec.items())
+    ]
+
+
+def concretize_separately(*spec_list, **kwargs):
+    """Given a number of specs as input, tries to concretize them together.
+
+    Args:
+        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
+        *spec_list: list of tuples to concretize. First entry is abstract spec, second entry is
+            already concrete spec or None if not yet concretized
+
+    Returns:
+        List of tuples of abstract and concretized specs
+    """
+    tests = kwargs.get("tests", False)
+    to_concretize = [abstract for abstract, concrete in spec_list if not concrete]
+    args = [
+        (i, str(abstract), tests)
+        for i, abstract in enumerate(to_concretize)
+        if not abstract.concrete
+    ]
+    ret = [(i, abstract) for i, abstract in enumerate(to_concretize) if abstract.concrete]
+    # Ensure we don't try to bootstrap clingo in parallel
+    with spack.bootstrap.ensure_bootstrap_configuration():
+        spack.bootstrap.ensure_clingo_importable_or_raise()
+
+    # Ensure all the indexes have been built or updated, since
+    # otherwise the processes in the pool may timeout on waiting
+    # for a write lock. We do this indirectly by retrieving the
+    # provider index, which should in turn trigger the update of
+    # all the indexes if there's any need for that.
+    _ = spack.repo.PATH.provider_index
+
+    # Ensure we have compilers in compilers.yaml to avoid that
+    # processes try to write the config file in parallel
+    _ = spack.compilers.all_compilers_config(spack.config.CONFIG)
+
+    # Early return if there is nothing to do
+    if len(args) == 0:
+        # Still have to combine the things that were passed in as abstract with the things
+        # that were passed in as pairs
+        return [(abstract, concrete) for abstract, (_, concrete) in zip(to_concretize, ret)] + [
+            (abstract, concrete) for abstract, concrete in spec_list if concrete
+        ]
+
+    # Solve the environment in parallel on Linux
+    # TODO: support parallel concretization on macOS and Windows
+    num_procs = min(len(args), spack.config.determine_number_of_jobs(parallel=True))
+
+    for j, (i, concrete, duration) in enumerate(
+        spack.util.parallel.imap_unordered(
+            spack.concretize._concretize_task,
+            args,
+            processes=num_procs,
+            debug=tty.is_debug(),
+            maxtaskperchild=1,
+        )
+    ):
+        ret.append((i, concrete))
+        percentage = (j + 1) / len(args) * 100
+        tty.verbose(
+            f"{duration:6.1f}s [{percentage:3.0f}%] {concrete.cformat('{hash:7}')} "
+            f"{to_concretize[i].colored_str}"
+        )
+        sys.stdout.flush()
+
+    # Add specs in original order
+    ret.sort(key=lambda x: x[0])
+
+    return [(abstract, concrete) for abstract, (_, concrete) in zip(to_concretize, ret)] + [
+        (abstract, concrete) for abstract, concrete in spec_list if concrete
+    ]
+
+
+def _concretize_task(packed_arguments) -> Tuple[int, Spec, float]:
+    index, spec_str, tests = packed_arguments
+    with tty.SuppressOutput(msg_enabled=False):
+        start = time.time()
+        spec = Spec(spec_str).concretized(tests=tests)
+        return index, spec, time.time() - start
+
+
 class UnavailableCompilerVersionError(spack.error.SpackError):
     """Raised when there is no available compiler that satisfies a
     compiler spec."""

lib/spack/spack/environment/environment.py

@@ -11,12 +11,10 @@
 import re
 import shutil
 import stat
-import sys
-import time
 import urllib.parse
 import urllib.request
 import warnings
-from typing import Any, Dict, Iterable, List, Optional, Set, Tuple, Union
+from typing import Any, Dict, Iterable, List, Optional, Tuple, Union
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
@@ -57,6 +55,8 @@
 from spack.spec_list import SpecList
 from spack.util.path import substitute_path_variables
 
+SpecPair = Tuple[spack.spec.Spec, spack.spec.Spec]
+
 #: environment variable used to indicate the active environment
 spack_env_var = "SPACK_ENV"
 
@@ -1510,7 +1510,7 @@ def deconcretize(self, spec: spack.spec.Spec, concrete: bool = True):
 
     def _get_specs_to_concretize(
         self,
-    ) -> Tuple[Set[spack.spec.Spec], Set[spack.spec.Spec], List[spack.spec.Spec]]:
+    ) -> Tuple[List[spack.spec.Spec], List[spack.spec.Spec], List[SpecPair]]:
         """Compute specs to concretize for unify:true and unify:when_possible.
 
         This includes new user specs and any already concretized specs.
@@ -1520,19 +1520,17 @@ def _get_specs_to_concretize(
 
         """
         # Exit early if the set of concretized specs is the set of user specs
-        new_user_specs = set(self.user_specs) - set(self.concretized_user_specs)
+        new_user_specs = list(set(self.user_specs) - set(self.concretized_user_specs))
-        kept_user_specs = set(self.user_specs) & set(self.concretized_user_specs)
+        kept_user_specs = list(set(self.user_specs) & set(self.concretized_user_specs))
-        kept_user_specs |= set(self.included_user_specs)
+        kept_user_specs += self.included_user_specs
         if not new_user_specs:
             return new_user_specs, kept_user_specs, []
 
-        concrete_specs_to_keep = [
+        specs_to_concretize = [(s, None) for s in new_user_specs] + [
-            concrete
+            (abstract, concrete)
             for abstract, concrete in self.concretized_specs()
             if abstract in kept_user_specs
         ]
-
-        specs_to_concretize = list(new_user_specs) + concrete_specs_to_keep
         return new_user_specs, kept_user_specs, specs_to_concretize
 
     def _concretize_together_where_possible(
@@ -1546,39 +1544,26 @@ def _concretize_together_where_possible(
         if not new_user_specs:
             return []
 
-        old_concrete_to_abstract = {
-            concrete: abstract for (abstract, concrete) in self.concretized_specs()
-        }
-
         self.concretized_user_specs = []
         self.concretized_order = []
         self.specs_by_hash = {}
 
-        result_by_user_spec = {}
+        ret = []
-        solver = spack.solver.asp.Solver()
+        result = spack.concretize.concretize_together_when_possible(
-        allow_deprecated = spack.config.get("config:deprecated", False)
+            *specs_to_concretize, tests=tests
-        for result in solver.solve_in_rounds(
+        )
-            specs_to_concretize, tests=tests, allow_deprecated=allow_deprecated
+        for abstract, concrete in result:
-        ):
+            # Only add to the environment if it's from this environment (not included in)
-            result_by_user_spec.update(result.specs_by_input)
-
-        result = []
-        for abstract, concrete in sorted(result_by_user_spec.items()):
-            # If the "abstract" spec is a concrete spec from the previous concretization
-            # translate it back to an abstract spec. Otherwise, keep the abstract spec
-            abstract = old_concrete_to_abstract.get(abstract, abstract)
-            if abstract in new_user_specs:
-                result.append((abstract, concrete))
-
-            # Only add to the environment if it's from this environment (not just included)
             if abstract in self.user_specs:
                 self._add_concrete_spec(abstract, concrete)
 
-        return result
+            # Return only the new specs
+            if abstract in new_user_specs:
+                ret.append((abstract, concrete))
 
-    def _concretize_together(
+        return ret
-        self, tests: bool = False
+
-    ) -> List[Tuple[spack.spec.Spec, spack.spec.Spec]]:
+    def _concretize_together(self, tests: bool = False) -> List[SpecPair]:
         """Concretization strategy that concretizes all the specs
         in the same DAG.
         """
@@ -1592,7 +1577,7 @@ def _concretize_together(
         self.specs_by_hash = {}
 
         try:
-            concrete_specs: List[spack.spec.Spec] = spack.concretize.concretize_specs_together(
+            concretized_specs: List[SpecPair] = spack.concretize.concretize_together(
                 *specs_to_concretize, tests=tests
             )
         except spack.error.UnsatisfiableSpecError as e:
@@ -1611,16 +1596,13 @@ def _concretize_together(
                 )
             raise
 
-        # set() | set() does not preserve ordering, even though sets are ordered
-        ordered_user_specs = list(new_user_specs) + list(kept_user_specs)
-        concretized_specs = [x for x in zip(ordered_user_specs, concrete_specs)]
         for abstract, concrete in concretized_specs:
             # Don't add if it's just included
             if abstract in self.user_specs:
                 self._add_concrete_spec(abstract, concrete)
 
-        # zip truncates the longer list, which is exactly what we want here
+        # Return the portion of the return value that is new
-        return list(zip(new_user_specs, concrete_specs))
+        return concretized_specs[: len(new_user_specs)]
 
     def _concretize_separately(self, tests=False):
         """Concretization strategy that concretizes separately one
@@ -1642,71 +1624,16 @@ def _concretize_separately(self, tests=False):
                 concrete = old_specs_by_hash[h]
                 self._add_concrete_spec(s, concrete, new=False)
 
-        # Concretize any new user specs that we haven't concretized yet
+        to_concretize = [
-        args, root_specs, i = [], [], 0
+            (root, None) for root in self.user_specs if root not in old_concretized_user_specs
-        for uspec in self.user_specs:
+        ]
-            if uspec not in old_concretized_user_specs:
+        concretized_specs = spack.concretize.concretize_separately(*to_concretize, tests=tests)
-                root_specs.append(uspec)
-                args.append((i, str(uspec), tests))
-                i += 1
 
-        # Ensure we don't try to bootstrap clingo in parallel
+        by_hash = {}
-        with spack.bootstrap.ensure_bootstrap_configuration():
+        for abstract, concrete in concretized_specs:
-            spack.bootstrap.ensure_clingo_importable_or_raise()
+            self._add_concrete_spec(abstract, concrete)
-
-        # Ensure all the indexes have been built or updated, since
-        # otherwise the processes in the pool may timeout on waiting
-        # for a write lock. We do this indirectly by retrieving the
-        # provider index, which should in turn trigger the update of
-        # all the indexes if there's any need for that.
-        _ = spack.repo.PATH.provider_index
-
-        # Ensure we have compilers in compilers.yaml to avoid that
-        # processes try to write the config file in parallel
-        _ = spack.compilers.all_compilers_config(spack.config.CONFIG)
-
-        # Early return if there is nothing to do
-        if len(args) == 0:
-            return []
-
-        # Solve the environment in parallel on Linux
-        start = time.time()
-        num_procs = min(len(args), spack.config.determine_number_of_jobs(parallel=True))
-
-        # TODO: support parallel concretization on macOS and Windows
-        msg = "Starting concretization"
-        if sys.platform not in ("darwin", "win32") and num_procs > 1:
-            msg += f" pool with {num_procs} processes"
-        tty.msg(msg)
-
-        batch = []
-        for j, (i, concrete, duration) in enumerate(
-            spack.util.parallel.imap_unordered(
-                _concretize_task,
-                args,
-                processes=num_procs,
-                debug=tty.is_debug(),
-                maxtaskperchild=1,
-            )
-        ):
-            batch.append((i, concrete))
-            percentage = (j + 1) / len(args) * 100
-            tty.verbose(
-                f"{duration:6.1f}s [{percentage:3.0f}%] {concrete.cformat('{hash:7}')} "
-                f"{root_specs[i].colored_str}"
-            )
-            sys.stdout.flush()
-
-        # Add specs in original order
-        batch.sort(key=lambda x: x[0])
-        by_hash = {}  # for attaching information on test dependencies
-        for root, (_, concrete) in zip(root_specs, batch):
-            self._add_concrete_spec(root, concrete)
             by_hash[concrete.dag_hash()] = concrete
 
-        finish = time.time()
-        tty.msg(f"Environment concretized in {finish - start:.2f} seconds")
-
         # Unify the specs objects, so we get correct references to all parents
         self._read_lockfile_dict(self._to_lockfile_dict())
 
@@ -1726,11 +1653,7 @@ def _concretize_separately(self, tests=False):
                             test_dependency.copy(), depflag=dt.TEST, virtuals=current_edge.virtuals
                         )
 
-        results = [
+        return concretized_specs
-            (abstract, self.specs_by_hash[h])
-            for abstract, h in zip(self.concretized_user_specs, self.concretized_order)
-        ]
-        return results
 
     @property
     def default_view(self):
@@ -2537,14 +2460,6 @@ def display_specs(specs):
     print(tree_string)
 
 
-def _concretize_task(packed_arguments) -> Tuple[int, Spec, float]:
-    index, spec_str, tests = packed_arguments
-    with tty.SuppressOutput(msg_enabled=False):
-        start = time.time()
-        spec = Spec(spec_str).concretized(tests=tests)
-        return index, spec, time.time() - start
-
-
 def make_repo_path(root):
     """Make a RepoPath from the repo subdirectories in an environment."""
     path = spack.repo.RepoPath(cache=spack.caches.MISC_CACHE)

lib/spack/spack/solver/asp.py

@@ -515,6 +515,8 @@ def _compute_specs_from_answer_set(self):
         best = min(self.answers)
         opt, _, answer = best
         for input_spec in self.abstract_specs:
+            # The specs must be unified to get here, so it is safe to associate any satisfying spec
+            # with the input. Multiple inputs may be matched to the same concrete spec
             node = SpecBuilder.make_node(pkg=input_spec.name)
             if input_spec.virtual:
                 providers = [

lib/spack/spack/test/cmd/install.py

@@ -906,7 +906,7 @@ def test_cdash_configure_warning(tmpdir, mock_fetch, install_mockery, capfd):
         specfile = "./spec.json"
         with open(specfile, "w") as f:
             f.write(spec.to_json())
-
+        print(spec.to_json())
         install("--log-file=cdash_reports", "--log-format=cdash", specfile)
         # Verify Configure.xml exists with expected contents.
         report_dir = tmpdir.join("cdash_reports")

lib/spack/spack/test/concretize.py

@@ -14,6 +14,7 @@
 import llnl.util.lang
 
 import spack.binary_distribution
+import spack.cmd
 import spack.compiler
 import spack.compilers
 import spack.concretize
@@ -3106,3 +3107,20 @@ def test_reuse_prefers_standard_over_git_versions(
         test_spec = spack.spec.Spec("git-ref-package@2").concretized()
         assert git_spec.dag_hash() != test_spec.dag_hash()
         assert standard_spec.dag_hash() == test_spec.dag_hash()
+
+
+@pytest.mark.parametrize("unify", [True, "when_possible", False])
+def test_spec_unification(unify, mutable_config, mock_packages):
+    spack.config.set("concretizer:unify", unify)
+    a = "pkg-a"
+    a_restricted = "pkg-a^pkg-b foo=baz"
+    b = "pkg-b foo=none"
+
+    unrestricted = spack.cmd.parse_specs([a, b], concretize=True)
+    a_concrete_unrestricted = [s for s in unrestricted if s.name == "pkg-a"][0]
+    b_concrete_unrestricted = [s for s in unrestricted if s.name == "pkg-b"][0]
+    assert (a_concrete_unrestricted["pkg-b"] == b_concrete_unrestricted) == (unify is not False)
+
+    maybe_fails = pytest.raises if unify is True else llnl.util.lang.nullcontext
+    with maybe_fails(spack.solver.asp.UnsatisfiableSpecError):
+        _ = spack.cmd.parse_specs([a_restricted, b], concretize=True)