spack.concretize: add type-hints, remove kwargs (#47382)

Also remove find_spec, which was used by the old concretizer.
Currently, it seems to be used only in tests.

lib/spack/docs/conf.py

@@ -214,6 +214,7 @@ def setup(sphinx):
     # Spack classes that intersphinx is unable to resolve
     ("py:class", "spack.version.StandardVersion"),
     ("py:class", "spack.spec.DependencySpec"),
+    ("py:class", "spack.spec.ArchSpec"),
     ("py:class", "spack.spec.InstallStatus"),
     ("py:class", "spack.spec.SpecfileReaderBase"),
     ("py:class", "spack.install_test.Pb"),

lib/spack/spack/cmd/__init__.py

@@ -194,7 +194,7 @@ def _concretize_spec_pairs(to_concretize, tests=False):
     elif unify == "when_possible":
         concretize_method = spack.concretize.concretize_together_when_possible
 
-    concretized = concretize_method(*to_concretize, tests=tests)
+    concretized = concretize_method(to_concretize, tests=tests)
     return [concrete for _, concrete in concretized]
 
 

lib/spack/spack/concretize.py

@@ -2,20 +2,17 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-"""
+"""High-level functions to concretize list of specs"""
-(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 Iterable, Optional, Sequence, Tuple, Union
-from typing import Tuple
 
 import llnl.util.tty as tty
 
 import spack.config
 import spack.error
-from spack.spec import Spec
+from spack.spec import ArchSpec, CompilerSpec, Spec
 
 CHECK_COMPILER_EXISTENCE = True
 
@@ -36,91 +33,59 @@ def enable_compiler_existence_check():
     CHECK_COMPILER_EXISTENCE = saved
 
 
-def find_spec(spec, condition, default=None):
+SpecPair = Tuple[Spec, Spec]
-    """Searches the dag from spec in an intelligent order and looks
+SpecLike = Union[Spec, str]
-    for a spec that matches a condition"""
+TestsType = Union[bool, Iterable[str]]
-    # First search parents, then search children
-    deptype = ("build", "link")
-    dagiter = chain(
-        spec.traverse(direction="parents", deptype=deptype, root=False),
-        spec.traverse(direction="children", deptype=deptype, root=False),
-    )
-    visited = set()
-    for relative in dagiter:
-        if condition(relative):
-            return relative
-        visited.add(id(relative))
-
-    # Then search all other relatives in the DAG *except* spec
-    for relative in spec.root.traverse(deptype="all"):
-        if relative is spec:
-            continue
-        if id(relative) in visited:
-            continue
-        if condition(relative):
-            return relative
-
-    # Finally search spec itself.
-    if condition(spec):
-        return spec
-
-    return default  # Nothing matched the condition; return default.
 
 
-def concretize_specs_together(*abstract_specs, **kwargs):
+def concretize_specs_together(
+    abstract_specs: Sequence[SpecLike], tests: TestsType = False
+) -> Sequence[Spec]:
     """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
+        abstract_specs: abstract specs to be concretized
-            all packages, or a list of package names to run tests for some
+        tests: list of package names for which to consider tests dependencies. If True, all nodes
-        *abstract_specs: abstract specs to be concretized, given either
+            will have test dependencies. If False, test dependencies will be disregarded.
-            as Specs or strings
-
-    Returns:
-        List of concretized specs
     """
     import spack.solver.asp
 
     allow_deprecated = spack.config.get("config:deprecated", False)
     solver = spack.solver.asp.Solver()
-    result = solver.solve(
+    result = solver.solve(abstract_specs, tests=tests, allow_deprecated=allow_deprecated)
-        abstract_specs, tests=kwargs.get("tests", False), allow_deprecated=allow_deprecated
-    )
     return [s.copy() for s in result.specs]
 
 
-def concretize_together(*spec_list, **kwargs):
+def concretize_together(
+    spec_list: Sequence[SpecPair], tests: TestsType = False
+) -> Sequence[SpecPair]:
     """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
+        spec_list: list of tuples to concretize. First entry is abstract spec, second entry is
-            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
-
+        tests: list of package names for which to consider tests dependencies. If True, all nodes
-    Returns:
+            will have test dependencies. If False, test dependencies will be disregarded.
-        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)
+    concrete_specs = concretize_specs_together(to_concretize, tests=tests)
     return list(zip(abstract_specs, concrete_specs))
 
 
-def concretize_together_when_possible(*spec_list, **kwargs):
+def concretize_together_when_possible(
+    spec_list: Sequence[SpecPair], tests: TestsType = False
+) -> Sequence[SpecPair]:
     """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
+        spec_list: list of tuples to concretize. First entry is abstract spec, second entry is
-            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
-
+        tests: list of package names for which to consider tests dependencies. If True, all nodes
-    Returns:
+            will have test dependencies. If False, test dependencies will be disregarded.
-        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 = {
@@ -131,7 +96,7 @@ def concretize_together_when_possible(*spec_list, **kwargs):
     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
+        to_concretize, tests=tests, allow_deprecated=allow_deprecated
     ):
         result_by_user_spec.update(result.specs_by_input)
 
@@ -143,19 +108,17 @@ def concretize_together_when_possible(*spec_list, **kwargs):
     ]
 
 
-def concretize_separately(*spec_list, **kwargs):
+def concretize_separately(
-    """Given a number of specs as input, tries to concretize them together.
+    spec_list: Sequence[SpecPair], tests: TestsType = False
+) -> Sequence[SpecPair]:
+    """Concretizes the input specs separately from each other.
 
     Args:
-        tests (bool or list or set): False to run no tests, True to test
+        spec_list: list of tuples to concretize. First entry is abstract spec, second entry is
-            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
-
+        tests: list of package names for which to consider tests dependencies. If True, all nodes
-    Returns:
+            will have test dependencies. If False, test dependencies will be disregarded.
-        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)
@@ -215,7 +178,7 @@ def concretize_separately(*spec_list, **kwargs):
     ]
 
 
-def _concretize_task(packed_arguments) -> Tuple[int, Spec, float]:
+def _concretize_task(packed_arguments: Tuple[int, str, TestsType]) -> Tuple[int, Spec, float]:
     index, spec_str, tests = packed_arguments
     with tty.SuppressOutput(msg_enabled=False):
         start = time.time()
@@ -227,10 +190,10 @@ class UnavailableCompilerVersionError(spack.error.SpackError):
     """Raised when there is no available compiler that satisfies a
     compiler spec."""
 
-    def __init__(self, compiler_spec, arch=None):
+    def __init__(self, compiler_spec: CompilerSpec, arch: Optional[ArchSpec] = None) -> None:
-        err_msg = "No compilers with spec {0} found".format(compiler_spec)
+        err_msg = f"No compilers with spec {compiler_spec} found"
         if arch:
-            err_msg += " for operating system {0} and target {1}.".format(arch.os, arch.target)
+            err_msg += f" for operating system {arch.os} and target {arch.target}."
 
         super().__init__(
             err_msg,

lib/spack/spack/environment/environment.py

@@ -14,7 +14,7 @@
 import urllib.parse
 import urllib.request
 import warnings
-from typing import Any, Dict, Iterable, List, Optional, Tuple, Union
+from typing import Any, Dict, Iterable, List, Optional, Sequence, Tuple, Union
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
@@ -55,7 +55,7 @@
 from spack.spec_list import SpecList
 from spack.util.path import substitute_path_variables
 
-SpecPair = Tuple[spack.spec.Spec, spack.spec.Spec]
+SpecPair = spack.concretize.SpecPair
 
 #: environment variable used to indicate the active environment
 spack_env_var = "SPACK_ENV"
@@ -1533,9 +1533,7 @@ def _get_specs_to_concretize(
         ]
         return new_user_specs, kept_user_specs, specs_to_concretize
 
-    def _concretize_together_where_possible(
+    def _concretize_together_where_possible(self, tests: bool = False) -> Sequence[SpecPair]:
-        self, tests: bool = False
-    ) -> List[Tuple[spack.spec.Spec, spack.spec.Spec]]:
         # Avoid cyclic dependency
         import spack.solver.asp
 
@@ -1550,7 +1548,7 @@ def _concretize_together_where_possible(
 
         ret = []
         result = spack.concretize.concretize_together_when_possible(
-            *specs_to_concretize, tests=tests
+            specs_to_concretize, tests=tests
         )
         for abstract, concrete in result:
             # Only add to the environment if it's from this environment (not included in)
@@ -1563,7 +1561,7 @@ def _concretize_together_where_possible(
 
         return ret
 
-    def _concretize_together(self, tests: bool = False) -> List[SpecPair]:
+    def _concretize_together(self, tests: bool = False) -> Sequence[SpecPair]:
         """Concretization strategy that concretizes all the specs
         in the same DAG.
         """
@@ -1577,8 +1575,8 @@ def _concretize_together(self, tests: bool = False) -> List[SpecPair]:
         self.specs_by_hash = {}
 
         try:
-            concretized_specs: List[SpecPair] = spack.concretize.concretize_together(
+            concretized_specs = spack.concretize.concretize_together(
-                *specs_to_concretize, tests=tests
+                specs_to_concretize, tests=tests
             )
         except spack.error.UnsatisfiableSpecError as e:
             # "Enhance" the error message for multiple root specs, suggest a less strict
@@ -1627,7 +1625,7 @@ def _concretize_separately(self, tests=False):
         to_concretize = [
             (root, None) for root in self.user_specs if root not in old_concretized_user_specs
         ]
-        concretized_specs = spack.concretize.concretize_separately(*to_concretize, tests=tests)
+        concretized_specs = spack.concretize.concretize_separately(to_concretize, tests=tests)
 
         by_hash = {}
         for abstract, concrete in concretized_specs:

lib/spack/spack/test/concretize.py

@@ -33,7 +33,6 @@
 import spack.store
 import spack.util.file_cache
 import spack.variant as vt
-from spack.concretize import find_spec
 from spack.installer import PackageInstaller
 from spack.spec import CompilerSpec, Spec
 from spack.version import Version, VersionList, ver
@@ -674,39 +673,6 @@ def test_external_and_virtual(self, mutable_config):
         assert spec["externaltool"].compiler.satisfies("gcc")
         assert spec["stuff"].compiler.satisfies("gcc")
 
-    def test_find_spec_parents(self):
-        """Tests the spec finding logic used by concretization."""
-        s = Spec.from_literal({"a +foo": {"b +foo": {"c": None, "d+foo": None}, "e +foo": None}})
-
-        assert "a" == find_spec(s["b"], lambda s: "+foo" in s).name
-
-    def test_find_spec_children(self):
-        s = Spec.from_literal({"a": {"b +foo": {"c": None, "d+foo": None}, "e +foo": None}})
-
-        assert "d" == find_spec(s["b"], lambda s: "+foo" in s).name
-
-        s = Spec.from_literal({"a": {"b +foo": {"c+foo": None, "d": None}, "e +foo": None}})
-
-        assert "c" == find_spec(s["b"], lambda s: "+foo" in s).name
-
-    def test_find_spec_sibling(self):
-        s = Spec.from_literal({"a": {"b +foo": {"c": None, "d": None}, "e +foo": None}})
-
-        assert "e" == find_spec(s["b"], lambda s: "+foo" in s).name
-        assert "b" == find_spec(s["e"], lambda s: "+foo" in s).name
-
-        s = Spec.from_literal({"a": {"b +foo": {"c": None, "d": None}, "e": {"f +foo": None}}})
-
-        assert "f" == find_spec(s["b"], lambda s: "+foo" in s).name
-
-    def test_find_spec_self(self):
-        s = Spec.from_literal({"a": {"b +foo": {"c": None, "d": None}, "e": None}})
-        assert "b" == find_spec(s["b"], lambda s: "+foo" in s).name
-
-    def test_find_spec_none(self):
-        s = Spec.from_literal({"a": {"b": {"c": None, "d": None}, "e": None}})
-        assert find_spec(s["b"], lambda s: "+foo" in s) is None
-
     def test_compiler_child(self):
         s = Spec("mpileaks%clang target=x86_64 ^dyninst%gcc")
         s.concretize()
@@ -815,7 +781,7 @@ def test_regression_issue_7941(self):
     )
     def test_simultaneous_concretization_of_specs(self, abstract_specs):
         abstract_specs = [Spec(x) for x in abstract_specs]
-        concrete_specs = spack.concretize.concretize_specs_together(*abstract_specs)
+        concrete_specs = spack.concretize.concretize_specs_together(abstract_specs)
 
         # Check there's only one configuration of each package in the DAG
         names = set(dep.name for spec in concrete_specs for dep in spec.traverse())
@@ -2137,7 +2103,7 @@ def test_external_python_extension_find_unified_python(self):
         spack.config.set("packages", external_conf)
 
         abstract_specs = [Spec(s) for s in ["py-extension1", "python"]]
-        specs = spack.concretize.concretize_specs_together(*abstract_specs)
+        specs = spack.concretize.concretize_specs_together(abstract_specs)
         assert specs[0]["python"] == specs[1]["python"]
 
     @pytest.mark.regression("36190")