From 826e0c0405c32b5e9a9c37a5f12e9bdbd279ac59 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Fri, 5 Apr 2024 20:10:28 +0200 Subject: [PATCH] Improve hit-rate on buildcaches (#43272) * Relax compiler and target mismatches The mismatch occurs on an edge. Previously it was assigned the parent priority, now it is assigned the child priority. This should make reuse from buildcaches or store more likely, since most mismatches will be counted with "reused" priority. * Optimize version badness for runtimes at very low priority We don't want to e.g. switch other attributes because we cannot reuse an old installed runtime. * Optimize runtime attributes at very low priority This is such that the version of the runtime would not influence whether we should reuse a spec. Compiler mismatches are considered for runtimes, to avoid situations where compiling foo%gcc@9 brings in gcc-runtime%gcc@13 if gcc@13 is among the available compilers * Exclude specs without runtimes from reuse This should ensure that we do not reuse specs that could be broken, as they expect the compiler to be installed in a specific place. --- lib/spack/spack/solver/asp.py | 15 +++- lib/spack/spack/solver/concretize.lp | 78 ++++++++++++++----- lib/spack/spack/target.py | 2 +- lib/spack/spack/test/cmd/compiler.py | 12 +-- lib/spack/spack/test/cmd/spec.py | 2 +- lib/spack/spack/test/concretize.py | 10 +-- .../test/concretize_compiler_runtimes.py | 35 +++++++-- .../spack/test/concretize_preferences.py | 2 +- lib/spack/spack/test/conftest.py | 9 +++ .../spack/test/data/config/compilers.yaml | 6 +- .../spack/test/data/config/packages.yaml | 2 +- .../data/modules/lmod/complex_hierarchy.yaml | 2 +- .../data/modules/lmod/core_compilers.yaml | 2 +- .../modules/lmod/core_compilers_at_equal.yaml | 2 +- lib/spack/spack/test/modules/lmod.py | 6 +- 15 files changed, 134 insertions(+), 51 deletions(-) diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 5928886bdbe..3c55941a56a 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -3371,7 +3371,7 @@ def _is_reusable(spec: spack.spec.Spec, packages, local: bool) -> bool: return False if not spec.external: - return True + return _has_runtime_dependencies(spec) # Cray external manifest externals are always reusable if local: @@ -3396,6 +3396,19 @@ def _is_reusable(spec: spack.spec.Spec, packages, local: bool) -> bool: return False +def _has_runtime_dependencies(spec: spack.spec.Spec) -> bool: + if not WITH_RUNTIME: + return True + + if spec.compiler.name == "gcc" and not spec.dependencies("gcc-runtime"): + return False + + if spec.compiler.name == "oneapi" and not spec.dependencies("intel-oneapi-runtime"): + return False + + return True + + class Solver: """This is the main external interface class for solving. diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index dd2a5afe027..1c6709602dd 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/lib/spack/spack/solver/concretize.lp @@ -80,6 +80,7 @@ unification_set(SetID, VirtualNode) #defined multiple_unification_sets/1. +#defined runtime/1. %---- % Rules to break symmetry and speed-up searches @@ -1494,18 +1495,20 @@ opt_criterion(40, "compiler mismatches that are not from CLI"). #minimize{ 0@240: #true }. #minimize{ 0@40: #true }. #minimize{ - 1@40+Priority,PackageNode,DependencyNode - : compiler_mismatch(PackageNode, DependencyNode), - build_priority(PackageNode, Priority) + 1@40+Priority,PackageNode,node(ID, Dependency) + : compiler_mismatch(PackageNode, node(ID, Dependency)), + build_priority(node(ID, Dependency), Priority), + not runtime(Dependency) }. opt_criterion(39, "compiler mismatches that are not from CLI"). #minimize{ 0@239: #true }. #minimize{ 0@39: #true }. #minimize{ - 1@39+Priority,PackageNode,DependencyNode - : compiler_mismatch_required(PackageNode, DependencyNode), - build_priority(PackageNode, Priority) + 1@39+Priority,PackageNode,node(ID, Dependency) + : compiler_mismatch_required(PackageNode, node(ID, Dependency)), + build_priority(node(ID, Dependency), Priority), + not runtime(Dependency) }. opt_criterion(30, "non-preferred OS's"). @@ -1522,9 +1525,10 @@ opt_criterion(25, "version badness"). #minimize{ 0@225: #true }. #minimize{ 0@25: #true }. #minimize{ - Weight@25+Priority,PackageNode - : version_weight(PackageNode, Weight), - build_priority(PackageNode, Priority) + Weight@25+Priority,node(X, Package) + : version_weight(node(X, Package), Weight), + build_priority(node(X, Package), Priority), + not runtime(Package) }. % Try to use all the default values of variants @@ -1543,9 +1547,10 @@ opt_criterion(15, "non-preferred compilers"). #minimize{ 0@215: #true }. #minimize{ 0@15: #true }. #minimize{ - Weight@15+Priority,PackageNode - : node_compiler_weight(PackageNode, Weight), - build_priority(PackageNode, Priority) + Weight@15+Priority,node(X, Package) + : node_compiler_weight(node(X, Package), Weight), + build_priority(node(X, Package), Priority), + not runtime(Package) }. % Minimize the number of mismatches for targets in the DAG, try @@ -1554,18 +1559,55 @@ opt_criterion(10, "target mismatches"). #minimize{ 0@210: #true }. #minimize{ 0@10: #true }. #minimize{ - 1@10+Priority,PackageNode,Dependency - : node_target_mismatch(PackageNode, Dependency), - build_priority(PackageNode, Priority) + 1@10+Priority,PackageNode,node(ID, Dependency) + : node_target_mismatch(PackageNode, node(ID, Dependency)), + build_priority(node(ID, Dependency), Priority), + not runtime(Dependency) }. opt_criterion(5, "non-preferred targets"). #minimize{ 0@205: #true }. #minimize{ 0@5: #true }. #minimize{ - Weight@5+Priority,PackageNode - : node_target_weight(PackageNode, Weight), - build_priority(PackageNode, Priority) + Weight@5+Priority,node(X, Package) + : node_target_weight(node(X, Package), Weight), + build_priority(node(X, Package), Priority), + not runtime(Package) +}. + + +% Minimize the number of compiler mismatches for runtimes +opt_criterion(4, "compiler mismatches (runtimes)"). +#minimize{ 0@204: #true }. +#minimize{ 0@4: #true }. +#minimize{ + 1@4,PackageNode,node(ID, Dependency) + : compiler_mismatch(PackageNode, node(ID, Dependency)), runtime(Dependency) +}. +#minimize{ + 1@4,PackageNode,node(ID, Dependency) + : compiler_mismatch_required(PackageNode, node(ID, Dependency)), runtime(Dependency) +}. + + +% Choose more recent versions for runtimes +opt_criterion(3, "version badness (runtimes)"). +#minimize{ 0@203: #true }. +#minimize{ 0@3: #true }. +#minimize{ + Weight@3,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 }. +#minimize{ + Weight@2,node(X, Package) + : node_target_weight(node(X, Package), Weight), + runtime(Package) }. % Choose more recent versions for nodes diff --git a/lib/spack/spack/target.py b/lib/spack/spack/target.py index 8f9e6dcf511..8d0943c28ae 100644 --- a/lib/spack/spack/target.py +++ b/lib/spack/spack/target.py @@ -142,7 +142,7 @@ def optimization_flags(self, compiler): # custom spec. compiler_version = compiler.version version_number, suffix = archspec.cpu.version_components(compiler.version) - if not version_number or suffix not in ("", "apple"): + if not version_number or suffix: # Try to deduce the underlying version of the compiler, regardless # of its name in compilers.yaml. Depending on where this function # is called we might get either a CompilerSpec or a fully fledged diff --git a/lib/spack/spack/test/cmd/compiler.py b/lib/spack/spack/test/cmd/compiler.py index 3b78720f087..46613b41fb6 100644 --- a/lib/spack/spack/test/cmd/compiler.py +++ b/lib/spack/spack/test/cmd/compiler.py @@ -112,10 +112,10 @@ def test_compiler_find_no_apple_gcc(no_compilers_yaml, working_env, mock_executa @pytest.mark.regression("37996") def test_compiler_remove(mutable_config, mock_packages): """Tests that we can remove a compiler from configuration.""" - assert spack.spec.CompilerSpec("gcc@=4.8.0") in spack.compilers.all_compiler_specs() - args = spack.util.pattern.Bunch(all=True, compiler_spec="gcc@4.8.0", add_paths=[], scope=None) + assert spack.spec.CompilerSpec("gcc@=9.4.0") in spack.compilers.all_compiler_specs() + args = spack.util.pattern.Bunch(all=True, compiler_spec="gcc@9.4.0", add_paths=[], scope=None) spack.cmd.compiler.compiler_remove(args) - assert spack.spec.CompilerSpec("gcc@=4.8.0") not in spack.compilers.all_compiler_specs() + assert spack.spec.CompilerSpec("gcc@=9.4.0") not in spack.compilers.all_compiler_specs() @pytest.mark.regression("37996") @@ -124,10 +124,10 @@ def test_removing_compilers_from_multiple_scopes(mutable_config, mock_packages): site_config = spack.config.get("compilers", scope="site") spack.config.set("compilers", site_config, scope="user") - assert spack.spec.CompilerSpec("gcc@=4.8.0") in spack.compilers.all_compiler_specs() - args = spack.util.pattern.Bunch(all=True, compiler_spec="gcc@4.8.0", add_paths=[], scope=None) + assert spack.spec.CompilerSpec("gcc@=9.4.0") in spack.compilers.all_compiler_specs() + args = spack.util.pattern.Bunch(all=True, compiler_spec="gcc@9.4.0", add_paths=[], scope=None) spack.cmd.compiler.compiler_remove(args) - assert spack.spec.CompilerSpec("gcc@=4.8.0") not in spack.compilers.all_compiler_specs() + assert spack.spec.CompilerSpec("gcc@=9.4.0") not in spack.compilers.all_compiler_specs() @pytest.mark.not_on_windows("Cannot execute bash script on Windows") diff --git a/lib/spack/spack/test/cmd/spec.py b/lib/spack/spack/test/cmd/spec.py index 718a5e08d45..6dfc024f47b 100644 --- a/lib/spack/spack/test/cmd/spec.py +++ b/lib/spack/spack/test/cmd/spec.py @@ -31,7 +31,7 @@ def test_spec(): @pytest.mark.only_clingo("Known failure of the original concretizer") -def test_spec_concretizer_args(mutable_config, mutable_database): +def test_spec_concretizer_args(mutable_config, mutable_database, do_not_check_runtimes_on_reuse): """End-to-end test of CLI concretizer prefs. It's here to make sure that everything works from CLI diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index cd1b1899616..81d4f3190f2 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -254,7 +254,7 @@ def gcc11_with_flags(compiler_factory): # This must use the mutable_config fixture because the test # adjusting_default_target_based_on_compiler uses the current_host fixture, # which changes the config. -@pytest.mark.usefixtures("mutable_config", "mock_packages") +@pytest.mark.usefixtures("mutable_config", "mock_packages", "do_not_check_runtimes_on_reuse") class TestConcretize: def test_concretize(self, spec): check_concretize(spec) @@ -614,7 +614,7 @@ def test_my_dep_depends_on_provider_of_my_virtual_dep(self): spec.normalize() spec.concretize() - @pytest.mark.parametrize("compiler_str", ["clang", "gcc", "gcc@10.2.1", "clang@:12.0.0"]) + @pytest.mark.parametrize("compiler_str", ["clang", "gcc", "gcc@10.2.1", "clang@:15.0.0"]) def test_compiler_inheritance(self, compiler_str): spec_str = "mpileaks %{0}".format(compiler_str) spec = Spec(spec_str).concretized() @@ -877,7 +877,7 @@ def test_concretize_anonymous_dep(self, spec_str): # Unconstrained versions select default compiler (gcc@4.5.0) ("bowtie@1.4.0", "%gcc@10.2.1"), # Version with conflicts and no valid gcc select another compiler - ("bowtie@1.3.0", "%clang@12.0.0"), + ("bowtie@1.3.0", "%clang@15.0.0"), # If a higher gcc is available still prefer that ("bowtie@1.2.2 os=redhat6", "%gcc@11.1.0"), ], @@ -1439,7 +1439,7 @@ def test_os_selection_when_multiple_choices_are_possible( @pytest.mark.regression("22718") @pytest.mark.parametrize( "spec_str,expected_compiler", - [("mpileaks", "%gcc@10.2.1"), ("mpileaks ^mpich%clang@12.0.0", "%clang@12.0.0")], + [("mpileaks", "%gcc@10.2.1"), ("mpileaks ^mpich%clang@15.0.0", "%clang@15.0.0")], ) def test_compiler_is_unique(self, spec_str, expected_compiler): s = Spec(spec_str).concretized() @@ -1727,7 +1727,7 @@ def test_reuse_with_unknown_package_dont_raise(self, tmpdir, temporary_store, mo [ (["libelf", "libelf@0.8.10"], 1), (["libdwarf%gcc", "libelf%clang"], 2), - (["libdwarf%gcc", "libdwarf%clang"], 4), + (["libdwarf%gcc", "libdwarf%clang"], 3), (["libdwarf^libelf@0.8.12", "libdwarf^libelf@0.8.13"], 4), (["hdf5", "zmpi"], 3), (["hdf5", "mpich"], 2), diff --git a/lib/spack/spack/test/concretize_compiler_runtimes.py b/lib/spack/spack/test/concretize_compiler_runtimes.py index ed91c01df85..3e13fd8e567 100644 --- a/lib/spack/spack/test/concretize_compiler_runtimes.py +++ b/lib/spack/spack/test/concretize_compiler_runtimes.py @@ -7,6 +7,8 @@ import pytest +import archspec.cpu + import spack.paths import spack.repo import spack.solver.asp @@ -24,9 +26,7 @@ def _concretize_with_reuse(*, root_str, reused_str): reused_spec = spack.spec.Spec(reused_str).concretized() setup = spack.solver.asp.SpackSolverSetup(tests=False) driver = spack.solver.asp.PyclingoDriver() - result, _, _ = driver.solve( - setup, [spack.spec.Spec(f"{root_str} ^{reused_str}")], reuse=[reused_spec] - ) + result, _, _ = driver.solve(setup, [spack.spec.Spec(f"{root_str}")], reuse=[reused_spec]) root = result.specs[0] return root, reused_spec @@ -47,7 +47,7 @@ def enable_runtimes(): def test_correct_gcc_runtime_is_injected_as_dependency(runtime_repo): - s = spack.spec.Spec("a%gcc@10.2.1 ^b%gcc@4.8.0").concretized() + s = spack.spec.Spec("a%gcc@10.2.1 ^b%gcc@9.4.0").concretized() a, b = s["a"], s["b"] # Both a and b should depend on the same gcc-runtime directly @@ -78,9 +78,28 @@ def test_external_nodes_do_not_have_runtimes(runtime_repo, mutable_config, tmp_p "root_str,reused_str,expected,nruntime", [ # The reused runtime is older than we need, thus we'll add a more recent one for a - ("a%gcc@10.2.1", "b%gcc@4.8.0", {"a": "gcc-runtime@10.2.1", "b": "gcc-runtime@4.8.0"}, 2), + ("a%gcc@10.2.1", "b%gcc@9.4.0", {"a": "gcc-runtime@10.2.1", "b": "gcc-runtime@9.4.0"}, 2), # The root is compiled with an older compiler, thus we'll reuse the runtime from b - ("a%gcc@4.8.0", "b%gcc@10.2.1", {"a": "gcc-runtime@10.2.1", "b": "gcc-runtime@10.2.1"}, 1), + ("a%gcc@9.4.0", "b%gcc@10.2.1", {"a": "gcc-runtime@10.2.1", "b": "gcc-runtime@10.2.1"}, 1), + # Same as before, but tests that we can reuse from a more generic target + pytest.param( + "a%gcc@9.4.0", + "b%gcc@10.2.1 target=x86_64", + {"a": "gcc-runtime@10.2.1 target=x86_64", "b": "gcc-runtime@10.2.1 target=x86_64"}, + 1, + marks=pytest.mark.skipif( + str(archspec.cpu.host().family) != "x86_64", reason="test data is x86_64 specific" + ), + ), + pytest.param( + "a%gcc@10.2.1", + "b%gcc@9.4.0 target=x86_64", + {"a": "gcc-runtime@10.2.1 target=x86_64", "b": "gcc-runtime@9.4.0 target=x86_64"}, + 2, + marks=pytest.mark.skipif( + str(archspec.cpu.host().family) != "x86_64", reason="test data is x86_64 specific" + ), + ), ], ) def test_reusing_specs_with_gcc_runtime(root_str, reused_str, expected, nruntime, runtime_repo): @@ -104,8 +123,8 @@ def test_reusing_specs_with_gcc_runtime(root_str, reused_str, expected, nruntime [ # Ensure that, whether we have multiple runtimes in the DAG or not, # we always link only the latest version - ("a%gcc@10.2.1", "b%gcc@4.8.0", ["gcc-runtime@10.2.1"], ["gcc-runtime@4.8.0"]), - ("a%gcc@4.8.0", "b%gcc@10.2.1", ["gcc-runtime@10.2.1"], ["gcc-runtime@4.8.0"]), + ("a%gcc@10.2.1", "b%gcc@9.4.0", ["gcc-runtime@10.2.1"], ["gcc-runtime@9.4.0"]), + ("a%gcc@9.4.0", "b%gcc@10.2.1", ["gcc-runtime@10.2.1"], ["gcc-runtime@9.4.0"]), ], ) def test_views_can_handle_duplicate_runtime_nodes( diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py index fb53374be6c..a7683bf65fb 100644 --- a/lib/spack/spack/test/concretize_preferences.py +++ b/lib/spack/spack/test/concretize_preferences.py @@ -105,7 +105,7 @@ def test_preferred_variants_from_wildcard(self): @pytest.mark.parametrize( "compiler_str,spec_str", - [("gcc@=4.8.0", "mpileaks"), ("clang@=12.0.0", "mpileaks"), ("gcc@=4.8.0", "openmpi")], + [("gcc@=9.4.0", "mpileaks"), ("clang@=15.0.0", "mpileaks"), ("gcc@=9.4.0", "openmpi")], ) def test_preferred_compilers(self, compiler_str, spec_str): """Test preferred compilers are applied correctly""" diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index e491cb436ca..7a38d4ccba9 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -2013,3 +2013,12 @@ def _factory(*, spec, operating_system): def host_architecture_str(): """Returns the broad architecture family (x86_64, aarch64, etc.)""" return str(archspec.cpu.host().family) + + +def _true(x): + return True + + +@pytest.fixture() +def do_not_check_runtimes_on_reuse(monkeypatch): + monkeypatch.setattr(spack.solver.asp, "_has_runtime_dependencies", _true) diff --git a/lib/spack/spack/test/data/config/compilers.yaml b/lib/spack/spack/test/data/config/compilers.yaml index e8a709dc851..0d5345130ac 100644 --- a/lib/spack/spack/test/data/config/compilers.yaml +++ b/lib/spack/spack/test/data/config/compilers.yaml @@ -1,6 +1,6 @@ compilers: - compiler: - spec: gcc@=4.8.0 + spec: gcc@=9.4.0 operating_system: {linux_os.name}{linux_os.version} paths: cc: /path/to/gcc @@ -10,7 +10,7 @@ compilers: modules: [] target: {target} - compiler: - spec: gcc@=4.8.0 + spec: gcc@=9.4.0 operating_system: redhat6 paths: cc: /path/to/gcc @@ -20,7 +20,7 @@ compilers: modules: [] target: {target} - compiler: - spec: clang@=12.0.0 + spec: clang@=15.0.0 operating_system: {linux_os.name}{linux_os.version} paths: cc: /path/to/clang diff --git a/lib/spack/spack/test/data/config/packages.yaml b/lib/spack/spack/test/data/config/packages.yaml index c6ff731a7e5..3d5cac76647 100644 --- a/lib/spack/spack/test/data/config/packages.yaml +++ b/lib/spack/spack/test/data/config/packages.yaml @@ -16,7 +16,7 @@ packages: externalvirtual: buildable: False externals: - - spec: externalvirtual@2.0%clang@12.0.0 + - spec: externalvirtual@2.0%clang@15.0.0 prefix: /path/to/external_virtual_clang - spec: externalvirtual@1.0%gcc@10.2.1 prefix: /path/to/external_virtual_gcc diff --git a/lib/spack/spack/test/data/modules/lmod/complex_hierarchy.yaml b/lib/spack/spack/test/data/modules/lmod/complex_hierarchy.yaml index 39515b38560..91adfd92e63 100644 --- a/lib/spack/spack/test/data/modules/lmod/complex_hierarchy.yaml +++ b/lib/spack/spack/test/data/modules/lmod/complex_hierarchy.yaml @@ -4,7 +4,7 @@ lmod: hash_length: 0 core_compilers: - - 'clang@12.0.0' + - 'clang@15.0.0' core_specs: - 'mpich@3.0.1' diff --git a/lib/spack/spack/test/data/modules/lmod/core_compilers.yaml b/lib/spack/spack/test/data/modules/lmod/core_compilers.yaml index 60d461d10af..0070f42bec5 100644 --- a/lib/spack/spack/test/data/modules/lmod/core_compilers.yaml +++ b/lib/spack/spack/test/data/modules/lmod/core_compilers.yaml @@ -2,4 +2,4 @@ enable: - lmod lmod: core_compilers: - - 'clang@12.0.0' + - 'clang@15.0.0' diff --git a/lib/spack/spack/test/data/modules/lmod/core_compilers_at_equal.yaml b/lib/spack/spack/test/data/modules/lmod/core_compilers_at_equal.yaml index d4b22dccf13..5a69c98bb5e 100644 --- a/lib/spack/spack/test/data/modules/lmod/core_compilers_at_equal.yaml +++ b/lib/spack/spack/test/data/modules/lmod/core_compilers_at_equal.yaml @@ -2,4 +2,4 @@ enable: - lmod lmod: core_compilers: - - 'clang@=12.0.0' + - 'clang@=15.0.0' diff --git a/lib/spack/spack/test/modules/lmod.py b/lib/spack/spack/test/modules/lmod.py index 5b38931c069..9038d8c9f6a 100644 --- a/lib/spack/spack/test/modules/lmod.py +++ b/lib/spack/spack/test/modules/lmod.py @@ -29,7 +29,7 @@ ] -@pytest.fixture(params=["clang@=12.0.0", "gcc@=10.2.1"]) +@pytest.fixture(params=["clang@=15.0.0", "gcc@=10.2.1"]) def compiler(request): return request.param @@ -59,7 +59,7 @@ def test_layout_for_specs_compiled_with_core_compilers( we can use both ``compiler@version`` and ``compiler@=version`` to specify a core compiler. """ module_configuration(modules_config) - module, spec = factory("libelf%clang@12.0.0") + module, spec = factory("libelf%clang@15.0.0") assert "Core" in module.layout.available_path_parts def test_file_layout(self, compiler, provider, factory, module_configuration): @@ -78,7 +78,7 @@ def test_file_layout(self, compiler, provider, factory, module_configuration): # is transformed to r"Core" if the compiler is listed among core # compilers # Check that specs listed as core_specs are transformed to "Core" - if compiler == "clang@=12.0.0" or spec_string == "mpich@3.0.1": + if compiler == "clang@=15.0.0" or spec_string == "mpich@3.0.1": assert "Core" in layout.available_path_parts else: assert compiler.replace("@=", "/") in layout.available_path_parts