From 745a0fac8a7bbfbfaf516af101f1f56f67c7e215 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Wed, 12 Feb 2025 11:27:54 +0100 Subject: [PATCH] Improve default selection of compilers This splits optimization on providers, and puts selection of default compilers at lower priority wrt selection of e.g. mpi or lapack provider. This is so that, on systems where 2 or more compilers are used (e.g. macOS), clingo would not make weird choices for mpi or lapack, in order to minimize compiler penalty due to using different compilers on the same node. --- lib/spack/spack/modules/lmod.py | 11 +++- lib/spack/spack/solver/concretize.lp | 62 +++++++++++++-------- lib/spack/spack/test/concretization/core.py | 49 ++++++++++++++-- lib/spack/spack/test/env.py | 28 ++++++++++ 4 files changed, 119 insertions(+), 31 deletions(-) diff --git a/lib/spack/spack/modules/lmod.py b/lib/spack/spack/modules/lmod.py index f8970cd1d2a..0f9475a14b1 100644 --- a/lib/spack/spack/modules/lmod.py +++ b/lib/spack/spack/modules/lmod.py @@ -6,6 +6,7 @@ import itertools import os import pathlib +import warnings from typing import Dict, List, Optional, Tuple import llnl.util.filesystem as fs @@ -102,15 +103,19 @@ class LmodConfiguration(BaseConfiguration): def __init__(self, spec: spack.spec.Spec, module_set_name: str, explicit: bool) -> None: super().__init__(spec, module_set_name, explicit) - # FIXME (compiler as nodes): make this a bit more robust candidates = collections.defaultdict(list) for node in spec.traverse(deptype=("link", "run")): candidates["c"].extend(node.dependencies(virtuals=("c",))) candidates["cxx"].extend(node.dependencies(virtuals=("c",))) - # FIXME (compiler as nodes): decide what to do when we have more than one C compiler - if candidates["c"] and len(set(candidates["c"])) == 1: + if candidates["c"]: self.compiler = candidates["c"][0] + if len(set(candidates["c"])) > 1: + warnings.warn( + f"{spec.short_spec} uses more than one compiler, and might not fit the " + f"LMod hierarchy. Using {self.compiler.short_spec} as the LMod compiler." + ) + elif not candidates["c"]: self.compiler = None diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index 3ee427c35c7..b9439724c72 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/lib/spack/spack/solver/concretize.lp @@ -1246,10 +1246,6 @@ error(100, "Cannot propagate the variant '{0}' from the package: {1} because pac % 1. The same flag type is not set on this node % 2. This node has the same compilers as the propagation source -compiler_penalty(PackageNode, C-1) :- - C = #count { CompilerNode : node_compiler(PackageNode, CompilerNode) }, - node_compiler(PackageNode, _). - node_compiler(node(X, Package), node(Y, Compiler)) :- attr("virtual_on_edge", node(X, Package), node(Y, Compiler), Language), attr("version", node(Y, Compiler), Version), @@ -1295,6 +1291,12 @@ compiler_used_as_a_library(node(X, Child), Hash) :- compiler_package(Child), % Used to restrict grounding for this rule attr("depends_on", _, node(X, Child), "link"). +% If a compiler is used for C on a package, it must provide C++ too, if need be, and vice-versa +:- attr("virtual_on_edge", PackageNode, CompilerNode1, "c"), + attr("virtual_on_edge", PackageNode, CompilerNode2, "cxx"), + CompilerNode1 != CompilerNode2. + + %----------------------------------------------------------------------------- % Runtimes %----------------------------------------------------------------------------- @@ -1660,9 +1662,9 @@ opt_criterion(60, "preferred providers for roots"). #minimize{ 0@260: #true }. #minimize{ 0@60: #true }. #minimize{ - Weight@60+Priority,ProviderNode,Virtual - : provider_weight(ProviderNode, Virtual, Weight), - attr("root", ProviderNode), + Weight@60+Priority,ProviderNode,X,Virtual + : provider_weight(ProviderNode, node(X, Virtual), Weight), + attr("root", ProviderNode), not language(Virtual), build_priority(ProviderNode, Priority) }. @@ -1687,36 +1689,50 @@ opt_criterion(50, "number of non-default variants (non-roots)"). build_priority(PackageNode, Priority) }. -% Minimize the ids of the providers, i.e. use as much as -% possible the first providers -opt_criterion(48, "number of duplicate virtuals needed"). +% Minimize the weights of the providers, i.e. use as much as +% possible the most preferred providers +opt_criterion(48, "preferred providers (non-roots)"). #minimize{ 0@248: #true }. #minimize{ 0@48: #true }. #minimize{ - Weight@48+Priority,ProviderNode,Virtual - : provider(ProviderNode, node(Weight, Virtual)), + Weight@48+Priority,ProviderNode,X,Virtual + : provider_weight(ProviderNode, node(X, Virtual), Weight), + not attr("root", ProviderNode), not language(Virtual), build_priority(ProviderNode, Priority) }. % Minimize the number of compilers used on nodes -opt_criterion(49, "minimize the number of compilers used on the same node"). -#minimize{ 0@249: #true }. -#minimize{ 0@49: #true }. + +compiler_penalty(PackageNode, C-1) :- + C = #count { CompilerNode : node_compiler(PackageNode, CompilerNode) }, + node_compiler(PackageNode, _). + +opt_criterion(46, "number of compilers used on the same node"). +#minimize{ 0@246: #true }. +#minimize{ 0@46: #true }. #minimize{ - Penalty@49+Priority,PackageNode - : compiler_penalty(PackageNode, Penalty), - build_priority(PackageNode, Priority) + Penalty@46+Priority,PackageNode + : compiler_penalty(PackageNode, Penalty), build_priority(PackageNode, Priority) }. -% Minimize the weights of the providers, i.e. use as much as -% possible the most preferred providers -opt_criterion(45, "preferred providers (non-roots)"). +% Minimize the ids of the providers, i.e. use as much as +% possible the first providers +opt_criterion(45, "number of duplicate virtuals needed"). #minimize{ 0@245: #true }. #minimize{ 0@45: #true }. #minimize{ Weight@45+Priority,ProviderNode,Virtual - : provider_weight(ProviderNode, Virtual, Weight), - not attr("root", ProviderNode), + : provider(ProviderNode, node(Weight, Virtual)), + build_priority(ProviderNode, Priority) +}. + +opt_criterion(40, "preferred compilers"). +#minimize{ 0@240: #true }. +#minimize{ 0@40: #true }. +#minimize{ + Weight@40+Priority,ProviderNode,X,Virtual + : provider_weight(ProviderNode, node(X, Virtual), Weight), + language(Virtual), build_priority(ProviderNode, Priority) }. diff --git a/lib/spack/spack/test/concretization/core.py b/lib/spack/spack/test/concretization/core.py index c2246131800..e88ba082093 100644 --- a/lib/spack/spack/test/concretization/core.py +++ b/lib/spack/spack/test/concretization/core.py @@ -741,12 +741,51 @@ def test_virtual_is_fully_expanded_for_mpileaks(self): assert all(not d.dependencies(name="mpi") for d in spec.traverse()) assert all(x in spec for x in ("zmpi", "mpi")) - @pytest.mark.parametrize("compiler_str", ["%clang", "%gcc", "%gcc@10.2.1", "%clang@:15.0.0"]) - def test_compiler_inheritance(self, compiler_str): - spec_str = f"mpileaks {compiler_str}" + @pytest.mark.parametrize( + "spec_str,expected,not_expected", + [ + # clang only provides C, and C++ compilers, while gcc has also fortran + # + # If we ask mpileaks%clang, then %gcc must be used for fortran, and since + # %gcc is preferred to clang in config, it will be used for most nodes + ( + "mpileaks %clang", + {"mpileaks": "%clang", "libdwarf": "%gcc", "libelf": "%gcc"}, + {"libdwarf": "%clang", "libelf": "%clang"}, + ), + ( + "mpileaks %clang@:15.0.0", + {"mpileaks": "%clang", "libdwarf": "%gcc", "libelf": "%gcc"}, + {"libdwarf": "%clang", "libelf": "%clang"}, + ), + ( + "mpileaks %gcc", + {"mpileaks": "%gcc", "libdwarf": "%gcc", "libelf": "%gcc"}, + {"mpileaks": "%clang", "libdwarf": "%clang", "libelf": "%clang"}, + ), + ( + "mpileaks %gcc@10.2.1", + {"mpileaks": "%gcc", "libdwarf": "%gcc", "libelf": "%gcc"}, + {"mpileaks": "%clang", "libdwarf": "%clang", "libelf": "%clang"}, + ), + # dyninst doesn't require fortran, so %clang is propagated + ( + "dyninst %clang", + {"dyninst": "%clang", "libdwarf": "%clang", "libelf": "%clang"}, + {"libdwarf": "%gcc", "libelf": "%gcc"}, + ), + ], + ) + def test_compiler_inheritance(self, spec_str, expected, not_expected): + """Spack tries to propagate compilers as much as possible, but prefers using a single + toolchain on a node, rather than mixing them. + """ spec = spack.concretize.concretize_one(spec_str) - assert spec["libdwarf"].satisfies(compiler_str) - assert spec["libelf"].satisfies(compiler_str) + for name, constraint in expected.items(): + assert spec[name].satisfies(constraint) + + for name, constraint in not_expected.items(): + assert not spec[name].satisfies(constraint) def test_external_package(self): """Tests that an external is preferred, if present, and that it does not diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py index 84c6046ae77..d56cd9e1fb1 100644 --- a/lib/spack/spack/test/env.py +++ b/lib/spack/spack/test/env.py @@ -921,3 +921,31 @@ def test_environment_from_name_or_dir(mock_packages, mutable_mock_env_path, tmp_ with pytest.raises(ev.SpackEnvironmentError, match="no such environment"): _ = ev.environment_from_name_or_dir("fake-env") + + +def test_using_multiple_compilers_on_a_node_is_discouraged( + tmp_path, mutable_config, mock_packages +): + """Tests that when we specify % Spack tries to use that compiler for all the + languages needed by that node. + """ + manifest = tmp_path / "spack.yaml" + manifest.write_text( + """\ +spack: + specs: + - mpileaks%clang ^mpich%gcc + concretizer: + unify: true +""" + ) + with ev.Environment(tmp_path) as e: + e.concretize() + mpileaks = e.concrete_roots()[0] + + assert not mpileaks.satisfies("%gcc") and mpileaks.satisfies("%clang") + assert len(mpileaks.dependencies(virtuals=("c", "cxx"))) == 1 + + mpich = mpileaks["mpich"] + assert mpich.satisfies("%gcc") and not mpich.satisfies("%clang") + assert len(mpich.dependencies(virtuals=("c", "cxx"))) == 1