From 842954f6a4d9efaba0f7495ac2b321f4d3b96477 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Fri, 9 May 2025 17:41:16 +0200 Subject: [PATCH] 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 --- lib/spack/spack/solver/asp.py | 4 +- lib/spack/spack/solver/concretize.lp | 62 ++++++----- lib/spack/spack/test/concretization/core.py | 100 ++++++++++++------ .../spack/test/concretization/requirements.py | 9 +- 4 files changed, 105 insertions(+), 70 deletions(-) diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 3a825929307..a3e04fcac03 100644 --- a/lib/spack/spack/solver/asp.py +++ b/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) diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index 697a6d4d08d..3ed05e81691 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/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"). -#minimize{ 0@205: #true }. -#minimize{ 0@5: #true }. +opt_criterion(7, "non-preferred targets"). +#minimize{ 0@207: #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)"). -#minimize{ 0@204: #true }. -#minimize{ 0@4: #true }. +opt_criterion(5, "preferred providers (language runtimes)"). +#minimize{ 0@205: #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)"). -#minimize{ 0@203: #true }. -#minimize{ 0@3: #true }. +opt_criterion(4, "version badness (runtimes)"). +#minimize{ 0@204: #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)"). -#minimize{ 0@202: #true }. -#minimize{ 0@2: #true }. +opt_criterion(3, "non-preferred targets (runtimes)"). +#minimize{ 0@203: #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 diff --git a/lib/spack/spack/test/concretization/core.py b/lib/spack/spack/test/concretization/core.py index 0399d8d7ef2..722174072ed 100644 --- a/lib/spack/spack/test/concretization/core.py +++ b/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 ( - bool(x.dependencies(name="llvm", deptype="build")) is not expected_gcc - ), x.long_spec + assert bool(x.dependencies(name="llvm", deptype="build")) is not expected_gcc, x.tree() 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 - ("bowtie@1.2.2", "%gcc@11.1.0"), + # If a higher gcc is available, with a worse os, still prefer that, + # 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') - # and it should be using pkg-b@1.0 with a version badness of 2 - # The provenance is: + # Version badness should be > 0 only for reused specs. For instance, for pkg-b + # the version 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"). - # - # Depending on the target, it may also use gnuconfig + v_weights = [x for x in result.criteria if x[2] == "version badness (non roots)"][0] + 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") - PackageInstaller([external1.package], fake=True, explicit=True).install() - assert external1.external + middle_pkg = spack.concretize.concretize_one("middle") + PackageInstaller([middle_pkg.package], fake=True, explicit=True).install() + 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") - assert external2.dag_hash() != external1.dag_hash() + root_no_reuse = spack.concretize.concretize_one("root") + 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") - assert external3.dag_hash() == external1.dag_hash() + root_with_reuse = spack.concretize.concretize_one("root") + 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") diff --git a/lib/spack/spack/test/concretization/requirements.py b/lib/spack/spack/test/concretization/requirements.py index c83a59d674e..cae5cb760a0 100644 --- a/lib/spack/spack/test/concretization/requirements.py +++ b/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" - pkg-a: - require: - - "%gcc@10" + - "%gcc@9" """, 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)