From 157d47fc5a20b4894f62e76928d643a047648b90 Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Tue, 5 Mar 2024 07:46:28 +0100 Subject: [PATCH] ASP-based solver: improve reusing nodes with gcc-runtime (#42408) * ASP-based solver: improve reusing nodes with gcc-runtime This PR skips emitting dependency constraints on "gcc-runtime", for concrete specs that are considered for reuse. Instead, an appropriate version of gcc-runtime is recomputed considering also the concrete nodes from reused specs. This ensures that root nodes in a DAG have always a runtime that is at a version greater or equal than their dependencies. * Add unit-test for view with multiple runtimes * Select latest version of runtimes in views * Construct result keeping track of latest * Keep ordering stable, just in case --- lib/spack/spack/environment/environment.py | 23 +++++-- lib/spack/spack/solver/asp.py | 5 ++ .../test/concretize_compiler_runtimes.py | 68 +++++++++++++++++++ 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 2ffa760ccd..727b46d048 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -626,14 +626,13 @@ def view(self, new: Optional[str] = None) -> SimpleFilesystemView: new: If a string, create a FilesystemView rooted at that path. Default None. This should only be used to regenerate the view, and cannot be used to access specs. """ - root = new if new else self._current_root - if not root: + path = new if new else self._current_root + if not path: # This can only be hit if we write a future bug raise SpackEnvironmentViewError( - "Attempting to get nonexistent view from environment. " - f"View root is at {self.root}" + f"Attempting to get nonexistent view from environment. View root is at {self.root}" ) - return self._view(root) + return self._view(path) def _view(self, root: str) -> SimpleFilesystemView: """Returns a view object for a given root dir.""" @@ -678,7 +677,9 @@ def specs_for_view(self, concrete_roots: List[Spec]) -> List[Spec]: # Filter selected, installed specs with spack.store.STORE.db.read_transaction(): - return [s for s in specs if s in self and s.installed] + result = [s for s in specs if s in self and s.installed] + + return self._exclude_duplicate_runtimes(result) def regenerate(self, concrete_roots: List[Spec]) -> None: specs = self.specs_for_view(concrete_roots) @@ -765,6 +766,16 @@ def regenerate(self, concrete_roots: List[Spec]) -> None: msg += str(e) tty.warn(msg) + def _exclude_duplicate_runtimes(self, nodes): + all_runtimes = spack.repo.PATH.packages_with_tags("runtime") + runtimes_by_name = {} + for s in nodes: + if s.name not in all_runtimes: + continue + current_runtime = runtimes_by_name.get(s.name, s) + runtimes_by_name[s.name] = max(current_runtime, s, key=lambda x: x.version) + return [x for x in nodes if x.name not in all_runtimes or runtimes_by_name[x.name] == x] + def _create_environment(path): return Environment(path) diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 8137daea63..0ae359c006 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -1786,6 +1786,11 @@ def _spec_clauses( dep = dspec.spec if spec.concrete: + # GCC runtime is solved again by clingo, even on concrete specs, to give + # the possibility to reuse specs built against a different runtime. + if dep.name == "gcc-runtime": + continue + # We know dependencies are real for concrete specs. For abstract # specs they just mean the dep is somehow in the DAG. for dtype in dt.ALL_FLAGS: diff --git a/lib/spack/spack/test/concretize_compiler_runtimes.py b/lib/spack/spack/test/concretize_compiler_runtimes.py index f331e9a7f8..12c9b2454d 100644 --- a/lib/spack/spack/test/concretize_compiler_runtimes.py +++ b/lib/spack/spack/test/concretize_compiler_runtimes.py @@ -11,6 +11,7 @@ import spack.repo import spack.solver.asp import spack.spec +from spack.environment.environment import ViewDescriptor from spack.version import Version pytestmark = [ @@ -19,6 +20,17 @@ ] +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] + ) + root = result.specs[0] + return root, reused_spec + + @pytest.fixture def runtime_repo(config): repo = os.path.join(spack.paths.repos_path, "compiler_runtime.test") @@ -60,3 +72,59 @@ def test_external_nodes_do_not_have_runtimes(runtime_repo, mutable_config, tmp_p assert a.dependencies("gcc-runtime") assert a.dependencies("b") assert not b.dependencies("gcc-runtime") + + +@pytest.mark.parametrize( + "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.5.0", {"a": "gcc-runtime@10.2.1", "b": "gcc-runtime@4.5.0"}, 2), + # The root is compiled with an older compiler, thus we'll reuse the runtime from b + ("a%gcc@4.5.0", "b%gcc@10.2.1", {"a": "gcc-runtime@10.2.1", "b": "gcc-runtime@10.2.1"}, 1), + ], +) +def test_reusing_specs_with_gcc_runtime(root_str, reused_str, expected, nruntime, runtime_repo): + """Tests that we can reuse specs with a "gcc-runtime" leaf node. In particular, checks + that the semantic for gcc-runtimes versions accounts for reused packages too. + """ + root, reused_spec = _concretize_with_reuse(root_str=root_str, reused_str=reused_str) + + assert f"{expected['b']}" in reused_spec + runtime_a = root.dependencies("gcc-runtime")[0] + assert runtime_a.satisfies(expected["a"]) + runtime_b = root["b"].dependencies("gcc-runtime")[0] + assert runtime_b.satisfies(expected["b"]) + + runtimes = [x for x in root.traverse() if x.name == "gcc-runtime"] + assert len(runtimes) == nruntime + + +@pytest.mark.parametrize( + "root_str,reused_str,expected,not_expected", + [ + # 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.5.0", ["gcc-runtime@10.2.1"], ["gcc-runtime@4.5.0"]), + ("a%gcc@4.5.0", "b%gcc@10.2.1", ["gcc-runtime@10.2.1"], ["gcc-runtime@4.5.0"]), + ], +) +def test_views_can_handle_duplicate_runtime_nodes( + root_str, reused_str, expected, not_expected, runtime_repo, tmp_path, monkeypatch +): + """Tests that an environment is able to select the latest version of a runtime node to be + linked in a view, in case more than one compatible version is in the DAG. + """ + root, reused_spec = _concretize_with_reuse(root_str=root_str, reused_str=reused_str) + + # Mock the installation status to allow selecting nodes for the view + monkeypatch.setattr(spack.spec.Spec, "installed", True) + nodes = list(root.traverse()) + + view = ViewDescriptor(str(tmp_path), str(tmp_path)) + candidate_specs = view.specs_for_view(nodes) + + for x in expected: + assert any(node.satisfies(x) for node in candidate_specs) + + for x in not_expected: + assert all(not node.satisfies(x) for node in candidate_specs)