solver: treat external nodes as concrete for optimization purposes (#50165)

This PR is a step towards treating externals as concrete specs. Specifically, it moves the optimization weights of external nodes into the group of "reused" specs, and doesn't count externals as specs to be built. 

It still keeps the one to many mapping between an external spec in `packages.yaml` and the corresponding specs in the DB. To make it such that an hashed external is preferred to a non-hashed one, the version weights of externals are demoted at lowest priority.

**Change in behavior**:
- Having the possibility, Spack will now prefer to mix compilers in a DAG, and use the latest version possible for each node, rather than using a single compiler and employ old versions of some nodes because of conflicts
- In general, using externals by default is now triggered by putting their weights in the "reused" group. This means that any penalty they might induce will never have the same priority as something that needs to be built. This behavior reflects reality, but changes some default choices from the previous state.

Modifications:
- External nodes weights are now moved to the group of "reused" specs
- Runtimes are treated as concrete as well (to avoid that e.g.`gcc` is not selected because it "builds" the runtime package)
- Shuffle version weights, so that externals are least preferred (counterbalanced by the fact that they are in the "reused" groups)
- Split provider weights on edges from version badness on edges

Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>

lib/spack/spack/solver/asp.py

@@ -128,8 +128,6 @@ class Provenance(enum.IntEnum):
     SPEC = enum.auto()
     # A dev spec literal
     DEV_SPEC = enum.auto()
-    # An external spec declaration
-    EXTERNAL = enum.auto()
     # The 'packages' section of the configuration
     PACKAGES_YAML = enum.auto()
     # A package requirement
@@ -138,6 +136,8 @@ class Provenance(enum.IntEnum):
     PACKAGE_PY = enum.auto()
     # An installed spec
     INSTALLED = enum.auto()
+    # An external spec declaration
+    EXTERNAL = enum.auto()
     # lower provenance for installed git refs so concretizer prefers StandardVersion installs
     INSTALLED_GIT_VERSION = enum.auto()
     # A runtime injected from another package (e.g. a compiler)

lib/spack/spack/solver/concretize.lp

@@ -1638,7 +1638,12 @@ build(PackageNode) :- attr("node", PackageNode), not concrete(PackageNode).
 %   200 - 299   Shifted priorities for build nodes; correspond to priorities 0 - 99.
 %   100 - 199   Unshifted priorities. Currently only includes minimizing #builds and minimizing dupes.
 %   0   -  99   Priorities for non-built nodes.
-build_priority(PackageNode, 200) :- build(PackageNode), attr("node", PackageNode).
+
+treat_node_as_concrete(node(X, Package)) :- external(node(X, Package)).
+treat_node_as_concrete(node(X, Package)) :- attr("node", node(X, Package)), runtime(Package).
+
+build_priority(PackageNode, 200) :- build(PackageNode), attr("node", PackageNode), not treat_node_as_concrete(PackageNode).
+build_priority(PackageNode, 0)   :- build(PackageNode), attr("node", PackageNode), treat_node_as_concrete(PackageNode).
 build_priority(PackageNode, 0)   :- not build(PackageNode), attr("node", PackageNode).
 
 % don't assign versions from installed packages unless reuse is enabled
@@ -1695,7 +1700,7 @@ opt_criterion(310, "requirement weight").
 % Try hard to reuse installed packages (i.e., minimize the number built)
 opt_criterion(110, "number of packages to build (vs. reuse)").
 #minimize { 0@110: #true }.
-#minimize { 1@110,PackageNode : build(PackageNode) }.
+#minimize { 1@110,PackageNode : build(PackageNode), not treat_node_as_concrete(PackageNode) }.
 
 opt_criterion(100, "number of nodes from the same package").
 #minimize { 0@100: #true }.
@@ -1860,48 +1865,59 @@ opt_criterion(10, "target mismatches").
       not runtime(Dependency)
 }.
 
-opt_criterion(5, "non-preferred targets").
+opt_criterion(7, "non-preferred targets").
-#minimize{ 0@205: #true }.
+#minimize{ 0@207: #true }.
-#minimize{ 0@5: #true }.
+#minimize{ 0@7: #true }.
 #minimize{
-    Weight@5+Priority,node(X, Package)
+    Weight@7+Priority,node(X, Package)
     : node_target_weight(node(X, Package), Weight),
       build_priority(node(X, Package), Priority),
       not runtime(Package)
 }.
 
-opt_criterion(4, "preferred providers (language runtimes)").
+opt_criterion(5, "preferred providers (language runtimes)").
-#minimize{ 0@204: #true }.
+#minimize{ 0@205: #true }.
-#minimize{ 0@4: #true }.
+#minimize{ 0@5: #true }.
 #minimize{
-    Weight@4+Priority,ProviderNode,X,Virtual
+    Weight@5+Priority,ProviderNode,X,Virtual
     : provider_weight(ProviderNode, node(X, Virtual), Weight),
       language_runtime(Virtual),
       build_priority(ProviderNode, Priority)
 }.
 
 % Choose more recent versions for runtimes
-opt_criterion(3, "version badness (runtimes)").
+opt_criterion(4, "version badness (runtimes)").
-#minimize{ 0@203: #true }.
+#minimize{ 0@204: #true }.
-#minimize{ 0@3: #true }.
+#minimize{ 0@4: #true }.
 #minimize{
-    Weight@3,node(X, Package)
+    Weight@4,node(X, Package)
     : version_weight(node(X, Package), Weight),
       runtime(Package)
 }.
 
 % Choose best target for runtimes
-opt_criterion(2, "non-preferred targets (runtimes)").
+opt_criterion(3, "non-preferred targets (runtimes)").
-#minimize{ 0@202: #true }.
+#minimize{ 0@203: #true }.
-#minimize{ 0@2: #true }.
+#minimize{ 0@3: #true }.
 #minimize{
-    Weight@2,node(X, Package)
+    Weight@3,node(X, Package)
     : node_target_weight(node(X, Package), Weight),
       runtime(Package)
 }.
 
 % Choose more recent versions for nodes
-opt_criterion(1, "edge wiring").
+opt_criterion(2, "providers on edges").
+#minimize{ 0@202: #true }.
+#minimize{ 0@2: #true }.
+#minimize{
+    Weight@2,ParentNode,ProviderNode,Virtual
+    : provider_weight(ProviderNode, Virtual, Weight),
+      not attr("root", ProviderNode),
+      depends_on(ParentNode, ProviderNode)
+}.
+
+% Choose more recent versions for nodes
+opt_criterion(1, "version badness on edges").
 #minimize{ 0@201: #true }.
 #minimize{ 0@1: #true }.
 #minimize{
@@ -1912,14 +1928,6 @@ opt_criterion(1, "edge wiring").
 }.
 
 
-#minimize{ 0@201: #true }.
-#minimize{ 0@1: #true }.
-#minimize{
-    Weight@1,ParentNode,ProviderNode,Virtual
-    : provider_weight(ProviderNode, Virtual, Weight),
-      not attr("root", ProviderNode),
-      depends_on(ParentNode, ProviderNode)
-}.
 
 %-----------
 % Notes

lib/spack/spack/test/concretization/core.py

@@ -188,6 +188,7 @@ class Root(Package):
     url      = "http://www.example.com/root-1.0.tar.gz"
 
     version("1.0", sha256="abcde")
+    depends_on("middle")
     depends_on("changing")
 
     conflicts("^changing~foo")
@@ -196,6 +197,20 @@ class Root(Package):
     package_py.parent.mkdir(parents=True)
     package_py.write_text(root_pkg_str)
 
+    middle_pkg_str = """
+from spack.package import *
+
+class Middle(Package):
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/root-1.0.tar.gz"
+
+    version("1.0", sha256="abcde")
+    depends_on("changing")
+"""
+    package_py = packages_dir / "middle" / "package.py"
+    package_py.parent.mkdir(parents=True)
+    package_py.write_text(middle_pkg_str)
+
     changing_template = """
 from spack.package import *
 
@@ -445,9 +460,7 @@ def test_mixing_compilers_only_affects_subdag(self):
             if "c" not in x or not x.name.startswith("dt-diamond"):
                 continue
             expected_gcc = x.name != "dt-diamond"
-            assert (
+            assert bool(x.dependencies(name="llvm", deptype="build")) is not expected_gcc, x.tree()
-                bool(x.dependencies(name="llvm", deptype="build")) is not expected_gcc
-            ), x.long_spec
             assert bool(x.dependencies(name="gcc", deptype="build")) is expected_gcc
             assert x.satisfies("%clang") is not expected_gcc
             assert x.satisfies("%gcc") is expected_gcc
@@ -1007,11 +1020,17 @@ def test_concretize_anonymous_dep(self, spec_str):
             ("bowtie@1.4.0", "%gcc@10.2.1"),
             # Version with conflicts and no valid gcc select another compiler
             ("bowtie@1.3.0", "%clang@15.0.0"),
-            # If a higher gcc is available, with a worse os, still prefer that
+            # If a higher gcc is available, with a worse os, still prefer that,
-            ("bowtie@1.2.2", "%gcc@11.1.0"),
+            # assuming the two operating systems are compatible
+            ("bowtie@1.2.2 %gcc", "%gcc@11.1.0"),
         ],
     )
-    def test_compiler_conflicts_in_package_py(self, spec_str, expected_str, gcc11_with_flags):
+    def test_compiler_conflicts_in_package_py(
+        self, spec_str, expected_str, gcc11_with_flags, mutable_config
+    ):
+        mutable_config.set(
+            "concretizer:os_compatible", {"debian6": ["redhat6"], "redhat6": ["debian6"]}
+        )
         with spack.config.override("packages", {"gcc": {"externals": [gcc11_with_flags]}}):
             s = spack.concretize.concretize_one(spec_str)
             assert s.satisfies(expected_str)
@@ -1224,20 +1243,6 @@ def test_activating_test_dependencies(self, spec_str, tests_arg, with_dep, witho
             node = s[pkg_name]
             assert not node.dependencies(deptype="test"), msg.format(pkg_name)
 
-    @pytest.mark.regression("20019")
-    def test_compiler_match_is_preferred_to_newer_version(self, compiler_factory):
-        # This spec depends on openblas. Openblas has a conflict
-        # that doesn't allow newer versions with gcc@4.4.0. Check
-        # that an old version of openblas is selected, rather than
-        # a different compiler for just that node.
-        with spack.config.override(
-            "packages", {"gcc": {"externals": [compiler_factory(spec="gcc@10.1.0 os=redhat6")]}}
-        ):
-            spec_str = "simple-inheritance+openblas os=redhat6 %gcc@10.1.0"
-            s = spack.concretize.concretize_one(spec_str)
-            assert "openblas@0.2.15" in s
-            assert s["openblas"].satisfies("%gcc@10.1.0")
-
     @pytest.mark.regression("19981")
     def test_target_ranges_in_conflicts(self):
         with pytest.raises(spack.error.SpackError):
@@ -1863,7 +1868,7 @@ def test_misleading_error_message_on_version(self, mutable_database):
 
     @pytest.mark.regression("31148")
     def test_version_weight_and_provenance(self):
-        """Test package preferences during coconcretization."""
+        """Test package preferences during concretization."""
         reusable_specs = [
             spack.concretize.concretize_one(spec_str) for spec_str in ("pkg-b@0.9", "pkg-b@1.0")
         ]
@@ -1873,17 +1878,18 @@ def test_version_weight_and_provenance(self):
             solver = spack.solver.asp.Solver()
             setup = spack.solver.asp.SpackSolverSetup()
             result, _, _ = solver.driver.solve(setup, [root_spec], reuse=reusable_specs)
-            # The result here should have a single spec to build ('pkg-a')
+            # Version badness should be > 0 only for reused specs. For instance, for pkg-b
-            # and it should be using pkg-b@1.0 with a version badness of 2
+            # the version provenance is:
-            # The provenance is:
+            #
             # version_declared("pkg-b","1.0",0,"package_py").
             # version_declared("pkg-b","0.9",1,"package_py").
             # version_declared("pkg-b","1.0",2,"installed").
             # version_declared("pkg-b","0.9",3,"installed").
-            #
+            v_weights = [x for x in result.criteria if x[2] == "version badness (non roots)"][0]
-            # Depending on the target, it may also use gnuconfig
+            reused_weights, built_weights, _ = v_weights
+            assert reused_weights > 2 and built_weights == 0
+
             result_spec = result.specs[0]
-            assert (2, 0, "version badness (non roots)") in result.criteria
             assert result_spec.satisfies("^pkg-b@1.0")
             assert result_spec["pkg-b"].dag_hash() == reusable_specs[1].dag_hash()
 
@@ -1930,7 +1936,9 @@ def test_git_ref_version_succeeds_with_unknown_version(self, git_ref):
     def test_installed_externals_are_reused(
         self, mutable_database, repo_with_changing_recipe, tmp_path
     ):
-        """Test that external specs that are in the DB can be reused."""
+        """Tests that external specs that are in the DB can be reused, if they result in a
+        better optimization score.
+        """
         external_conf = {
             "changing": {
                 "buildable": False,
@@ -1940,22 +1948,23 @@ def test_installed_externals_are_reused(
         spack.config.set("packages", external_conf)
 
         # Install the external spec
-        external1 = spack.concretize.concretize_one("changing@1.0")
+        middle_pkg = spack.concretize.concretize_one("middle")
-        PackageInstaller([external1.package], fake=True, explicit=True).install()
+        PackageInstaller([middle_pkg.package], fake=True, explicit=True).install()
-        assert external1.external
+        assert middle_pkg["changing"].external
+        changing_external = middle_pkg["changing"]
 
         # Modify the package.py file
         repo_with_changing_recipe.change({"delete_variant": True})
 
         # Try to concretize the external without reuse and confirm the hash changed
         with spack.config.override("concretizer:reuse", False):
-            external2 = spack.concretize.concretize_one("changing@1.0")
+            root_no_reuse = spack.concretize.concretize_one("root")
-        assert external2.dag_hash() != external1.dag_hash()
+        assert root_no_reuse["changing"].dag_hash() != changing_external.dag_hash()
 
         # ... while with reuse we have the same hash
         with spack.config.override("concretizer:reuse", True):
-            external3 = spack.concretize.concretize_one("changing@1.0")
+            root_with_reuse = spack.concretize.concretize_one("root")
-        assert external3.dag_hash() == external1.dag_hash()
+        assert root_with_reuse["changing"].dag_hash() == changing_external.dag_hash()
 
     @pytest.mark.regression("31484")
     def test_user_can_select_externals_with_require(self, mutable_database, tmp_path):
@@ -3440,3 +3449,24 @@ def test_using_externals_with_compilers(mutable_config, mock_packages, tmp_path)
 
     libelf = s["libelf"]
     assert libelf.external and libelf.satisfies("%gcc")
+
+
+@pytest.mark.regression("50161")
+def test_installed_compiler_and_better_external(
+    install_mockery, do_not_check_runtimes_on_reuse, mutable_config
+):
+    """Tests that we always prefer a higher-priority external compiler, when we have a
+    lower-priority compiler installed, and we try to concretize a spec without specifying
+    the compiler dependency.
+    """
+    pkg_b = spack.concretize.concretize_one(spack.spec.Spec("pkg-b %clang"))
+    PackageInstaller([pkg_b.package], fake=True, explicit=True).install()
+
+    with spack.config.override("concretizer:reuse", False):
+        pkg_a = spack.concretize.concretize_one("pkg-a")
+        assert pkg_a["c"].satisfies("gcc@10"), pkg_a.tree()
+        assert pkg_a["pkg-b"]["c"].satisfies("gcc@10")
+
+    with spack.config.override("concretizer:reuse", False):
+        mpileaks = spack.concretize.concretize_one("mpileaks")
+        assert mpileaks.satisfies("%gcc@10")

lib/spack/spack/test/concretization/requirements.py

@@ -1270,13 +1270,10 @@ def test_virtual_requirement_respects_any_of(concretize_scope, mock_packages):
 packages:
   all:
     require:
-    - "%gcc"
+    - "%gcc@9"
-  pkg-a:
-    require:
-    - "%gcc@10"
     """,
             True,
-            ["%gcc@10"],
+            ["%gcc@9"],
         ),
     ],
 )
@@ -1303,4 +1300,4 @@ def test_requirements_on_compilers_and_reuse(
 
     assert is_pkgb_reused == expected_reuse
     for c in expected_contraints:
-        assert pkga.satisfies(c), print(pkga.tree())
+        assert pkga.satisfies(c)