Concretize reuse: reuse specs from environment (#45139)

The already concrete specs in an environment are now among the reusable specs for the concretizer.

This includes concrete specs from all include_concrete environments.

In addition to this change to the default reuse, `environment` is added as a reuse type for 
the concretizer config. This allows users to specify:

spack:
  concretizer:
    # Reuse from this environment (including included concrete) but not elsewhere
    reuse:
      from:
      - type: environment
    # or reuse from only my_env included environment
    reuse:
      from:
      - type:
          environment: my_env
    # or reuse from everywhere
    reuse: true

If reuse is specified from a specific environment, only specs from that environment will be reused.
If the reused environment is not specified via include_concrete, the concrete specs will be retried
at concretization time to be reused.

Signed-off-by: Ryan Krattiger <ryan.krattiger@kitware.com>
Co-authored-by: Gregory Becker <becker33@llnl.gov>

lib/spack/spack/environment/__init__.py

@@ -473,6 +473,7 @@
     active_environment,
     all_environment_names,
     all_environments,
+    as_env_dir,
     create,
     create_in_dir,
     deactivate,
@@ -480,6 +481,7 @@
     default_view_name,
     display_specs,
     environment_dir_from_name,
+    environment_from_name_or_dir,
     exists,
     initialize_environment_dir,
     installed_specs,
@@ -507,6 +509,7 @@
     "active_environment",
     "all_environment_names",
     "all_environments",
+    "as_env_dir",
     "create",
     "create_in_dir",
     "deactivate",
@@ -514,6 +517,7 @@
     "default_view_name",
     "display_specs",
     "environment_dir_from_name",
+    "environment_from_name_or_dir",
     "exists",
     "initialize_environment_dir",
     "installed_specs",

lib/spack/spack/environment/environment.py

@@ -277,6 +277,22 @@ def is_env_dir(path):
     return os.path.isdir(path) and os.path.exists(os.path.join(path, manifest_name))
 
 
+def as_env_dir(name_or_dir):
+    """Translate an environment name or directory to the environment directory"""
+    if is_env_dir(name_or_dir):
+        return name_or_dir
+    else:
+        validate_env_name(name_or_dir)
+        if not exists(name_or_dir):
+            raise SpackEnvironmentError("no such environment '%s'" % name_or_dir)
+        return root(name_or_dir)
+
+
+def environment_from_name_or_dir(name_or_dir):
+    """Get an environment with the supplied name."""
+    return Environment(as_env_dir(name_or_dir))
+
+
 def read(name):
     """Get an environment with the supplied name."""
     validate_env_name(name)
@@ -1506,6 +1522,7 @@ def _get_specs_to_concretize(
         # Exit early if the set of concretized specs is the set of user specs
         new_user_specs = set(self.user_specs) - set(self.concretized_user_specs)
         kept_user_specs = set(self.user_specs) & set(self.concretized_user_specs)
+        kept_user_specs |= set(self.included_user_specs)
         if not new_user_specs:
             return new_user_specs, kept_user_specs, []
 
@@ -1552,7 +1569,10 @@ def _concretize_together_where_possible(
             abstract = old_concrete_to_abstract.get(abstract, abstract)
             if abstract in new_user_specs:
                 result.append((abstract, concrete))
-            self._add_concrete_spec(abstract, concrete)
+
+            # Only add to the environment if it's from this environment (not just included)
+            if abstract in self.user_specs:
+                self._add_concrete_spec(abstract, concrete)
 
         return result
 
@@ -1595,7 +1615,9 @@ def _concretize_together(
         ordered_user_specs = list(new_user_specs) + list(kept_user_specs)
         concretized_specs = [x for x in zip(ordered_user_specs, concrete_specs)]
         for abstract, concrete in concretized_specs:
-            self._add_concrete_spec(abstract, concrete)
+            # Don't add if it's just included
+            if abstract in self.user_specs:
+                self._add_concrete_spec(abstract, concrete)
 
         # zip truncates the longer list, which is exactly what we want here
         return list(zip(new_user_specs, concrete_specs))

lib/spack/spack/schema/concretizer.py

@@ -32,8 +32,23 @@
                                     "type": "object",
                                     "properties": {
                                         "type": {
-                                            "type": "string",
+                                            "oneOf": [
-                                            "enum": ["local", "buildcache", "external"],
+                                                {
+                                                    "type": "string",
+                                                    "enum": [
+                                                        "local",
+                                                        "buildcache",
+                                                        "environment",
+                                                        "external",
+                                                    ],
+                                                },
+                                                {
+                                                    "type": "object",
+                                                    "properties": {
+                                                        "environment": {"type": "string"}
+                                                    },
+                                                },
+                                            ]
                                         },
                                         "include": LIST_OF_SPECS,
                                         "exclude": LIST_OF_SPECS,

lib/spack/spack/schema/env.py

@@ -19,6 +19,8 @@
 #: Top level key in a manifest file
 TOP_LEVEL_KEY = "spack"
 
+include_concrete = {"type": "array", "default": [], "items": {"type": "string"}}
+
 properties: Dict[str, Any] = {
     "spack": {
         "type": "object",
@@ -31,7 +33,7 @@
             {
                 "include": {"type": "array", "default": [], "items": {"type": "string"}},
                 "specs": spec_list_schema,
-                "include_concrete": {"type": "array", "default": [], "items": {"type": "string"}},
+                "include_concrete": include_concrete,
             },
         ),
     }

lib/spack/spack/solver/asp.py

@@ -2616,6 +2616,7 @@ def setup(
                 )
                 for name, info in env.dev_specs.items()
             )
+
         specs = tuple(specs)  # ensure compatible types to add
 
         self.gen.h1("Reusable concrete specs")
@@ -3978,7 +3979,7 @@ def selected_specs(self) -> List[spack.spec.Spec]:
         return [s for s in self.factory() if self.is_selected(s)]
 
     @staticmethod
-    def from_store(configuration, include, exclude) -> "SpecFilter":
+    def from_store(configuration, *, include, exclude) -> "SpecFilter":
         """Constructs a filter that takes the specs from the current store."""
         packages = _external_config_with_implicit_externals(configuration)
         is_reusable = functools.partial(_is_reusable, packages=packages, local=True)
@@ -3986,7 +3987,7 @@ def from_store(configuration, include, exclude) -> "SpecFilter":
         return SpecFilter(factory=factory, is_usable=is_reusable, include=include, exclude=exclude)
 
     @staticmethod
-    def from_buildcache(configuration, include, exclude) -> "SpecFilter":
+    def from_buildcache(configuration, *, include, exclude) -> "SpecFilter":
         """Constructs a filter that takes the specs from the configured buildcaches."""
         packages = _external_config_with_implicit_externals(configuration)
         is_reusable = functools.partial(_is_reusable, packages=packages, local=False)
@@ -3994,6 +3995,29 @@ def from_buildcache(configuration, include, exclude) -> "SpecFilter":
             factory=_specs_from_mirror, is_usable=is_reusable, include=include, exclude=exclude
         )
 
+    @staticmethod
+    def from_environment(configuration, *, include, exclude, env) -> "SpecFilter":
+        packages = _external_config_with_implicit_externals(configuration)
+        is_reusable = functools.partial(_is_reusable, packages=packages, local=True)
+        factory = functools.partial(_specs_from_environment, env=env)
+        return SpecFilter(factory=factory, is_usable=is_reusable, include=include, exclude=exclude)
+
+    @staticmethod
+    def from_environment_included_concrete(
+        configuration,
+        *,
+        include: List[str],
+        exclude: List[str],
+        env: ev.Environment,
+        included_concrete: str,
+    ) -> "SpecFilter":
+        packages = _external_config_with_implicit_externals(configuration)
+        is_reusable = functools.partial(_is_reusable, packages=packages, local=True)
+        factory = functools.partial(
+            _specs_from_environment_included_concrete, env=env, included_concrete=included_concrete
+        )
+        return SpecFilter(factory=factory, is_usable=is_reusable, include=include, exclude=exclude)
+
 
 def _specs_from_store(configuration):
     store = spack.store.create(configuration)
@@ -4011,6 +4035,23 @@ def _specs_from_mirror():
         return []
 
 
+def _specs_from_environment(env):
+    """Return all concrete specs from the environment. This includes all included concrete"""
+    if env:
+        return [concrete for _, concrete in env.concretized_specs()]
+    else:
+        return []
+
+
+def _specs_from_environment_included_concrete(env, included_concrete):
+    """Return only concrete specs from the environment included from the included_concrete"""
+    if env:
+        assert included_concrete in env.included_concrete_envs
+        return [concrete for concrete in env.included_specs_by_hash[included_concrete].values()]
+    else:
+        return []
+
+
 class ReuseStrategy(enum.Enum):
     ROOTS = enum.auto()
     DEPENDENCIES = enum.auto()
@@ -4040,6 +4081,12 @@ def __init__(self, configuration: spack.config.Configuration) -> None:
                     SpecFilter.from_buildcache(
                         configuration=self.configuration, include=[], exclude=[]
                     ),
+                    SpecFilter.from_environment(
+                        configuration=self.configuration,
+                        include=[],
+                        exclude=[],
+                        env=ev.active_environment(),  # includes all concrete includes
+                    ),
                 ]
             )
         else:
@@ -4054,7 +4101,46 @@ def __init__(self, configuration: spack.config.Configuration) -> None:
             for source in reuse_yaml.get("from", default_sources):
                 include = source.get("include", default_include)
                 exclude = source.get("exclude", default_exclude)
-                if source["type"] == "local":
+                if isinstance(source["type"], dict):
+                    env_dir = ev.as_env_dir(source["type"].get("environment"))
+                    active_env = ev.active_environment()
+                    if active_env and env_dir in active_env.included_concrete_envs:
+                        # If environment is included as a concrete environment, use the local copy
+                        # of specs in the active environment.
+                        # note: included concrete environments are only updated at concretization
+                        #       time, and reuse needs to matchthe included specs.
+                        self.reuse_sources.append(
+                            SpecFilter.from_environment_included_concrete(
+                                self.configuration,
+                                include=include,
+                                exclude=exclude,
+                                env=active_env,
+                                included_concrete=env_dir,
+                            )
+                        )
+                    else:
+                        # If the environment is not included as a concrete environment, use the
+                        # current specs from its lockfile.
+                        self.reuse_sources.append(
+                            SpecFilter.from_environment(
+                                self.configuration,
+                                include=include,
+                                exclude=exclude,
+                                env=ev.environment_from_name_or_dir(env_dir),
+                            )
+                        )
+                elif source["type"] == "environment":
+                    # reusing from the current environment implicitly reuses from all of the
+                    # included concrete environments
+                    self.reuse_sources.append(
+                        SpecFilter.from_environment(
+                            self.configuration,
+                            include=include,
+                            exclude=exclude,
+                            env=ev.active_environment(),
+                        )
+                    )
+                elif source["type"] == "local":
                     self.reuse_sources.append(
                         SpecFilter.from_store(self.configuration, include=include, exclude=exclude)
                     )

lib/spack/spack/test/cmd/env.py

@@ -9,6 +9,7 @@
 import pathlib
 import shutil
 from argparse import Namespace
+from typing import Any, Dict, Optional
 
 import pytest
 
@@ -74,7 +75,7 @@ def setup_combined_multiple_env():
     env("create", "test1")
     test1 = ev.read("test1")
     with test1:
-        add("zlib")
+        add("mpich@1.0")
     test1.concretize()
     test1.write()
 
@@ -401,14 +402,17 @@ def test_env_install_single_spec(install_mockery, mock_fetch):
 
 
 @pytest.mark.parametrize("unify", [True, False, "when_possible"])
-def test_env_install_include_concrete_env(unify, install_mockery, mock_fetch):
+def test_env_install_include_concrete_env(unify, install_mockery, mock_fetch, mutable_config):
     test1, test2, combined = setup_combined_multiple_env()
 
+    combined.unify = unify
+    if not unify:
+        combined.manifest.set_default_view(False)
+
+    combined.add("mpileaks")
     combined.concretize()
     combined.write()
 
-    combined.unify = unify
-
     with combined:
         install()
 
@@ -422,6 +426,14 @@ def test_env_install_include_concrete_env(unify, install_mockery, mock_fetch):
     assert test1_roots == combined_included_roots[test1.path]
     assert test2_roots == combined_included_roots[test2.path]
 
+    mpileaks = combined.specs_by_hash[combined.concretized_order[0]]
+    if unify:
+        assert mpileaks["mpi"].dag_hash() in test1_roots
+        assert mpileaks["libelf"].dag_hash() in test2_roots
+    else:
+        # check that unification is not by accident
+        assert mpileaks["mpi"].dag_hash() not in test1_roots
+
 
 def test_env_roots_marked_explicit(install_mockery, mock_fetch):
     install = SpackCommand("install")
@@ -1869,7 +1881,7 @@ def test_env_include_concrete_envs_lockfile():
 def test_env_include_concrete_add_env():
     test1, test2, combined = setup_combined_multiple_env()
 
-    # crete new env & crecretize
+    # create new env & concretize
     env("create", "new")
     new_env = ev.read("new")
     with new_env:
@@ -1921,6 +1933,116 @@ def test_env_include_concrete_remove_env():
     assert test2.path not in lockfile_as_dict["include_concrete"].keys()
 
 
+def configure_reuse(reuse_mode, combined_env) -> Optional[ev.Environment]:
+    override_env = None
+    _config: Dict[Any, Any] = {}
+    if reuse_mode == "true":
+        _config = {"concretizer": {"reuse": True}}
+    elif reuse_mode == "from_environment":
+        _config = {"concretizer": {"reuse": {"from": [{"type": "environment"}]}}}
+    elif reuse_mode == "from_environment_test1":
+        _config = {"concretizer": {"reuse": {"from": [{"type": {"environment": "test1"}}]}}}
+    elif reuse_mode == "from_environment_external_test":
+        # Create a new environment called external_test that enables the "debug"
+        # The default is "~debug"
+        env("create", "external_test")
+        override_env = ev.read("external_test")
+        with override_env:
+            add("mpich@1.0 +debug")
+        override_env.concretize()
+        override_env.write()
+
+        # Reuse from the environment that is not included.
+        # Specify the requirement for the debug variant. By default this would concretize to use
+        # mpich@3.0 but with include concrete the mpich@1.0 +debug version from the
+        # "external_test" environment will be used.
+        _config = {
+            "concretizer": {"reuse": {"from": [{"type": {"environment": "external_test"}}]}},
+            "packages": {"mpich": {"require": ["+debug"]}},
+        }
+    elif reuse_mode == "from_environment_raise":
+        _config = {
+            "concretizer": {"reuse": {"from": [{"type": {"environment": "not-a-real-env"}}]}}
+        }
+    # Disable unification in these tests to avoid confusing reuse due to unification using an
+    # include concrete spec vs reuse due to the reuse configuration
+    _config["concretizer"].update({"unify": False})
+
+    combined_env.manifest.configuration.update(_config)
+    combined_env.manifest.changed = True
+    combined_env.write()
+
+    return override_env
+
+
+@pytest.mark.parametrize(
+    "reuse_mode",
+    [
+        "true",
+        "from_environment",
+        "from_environment_test1",
+        "from_environment_external_test",
+        "from_environment_raise",
+    ],
+)
+def test_env_include_concrete_reuse(monkeypatch, reuse_mode):
+
+    # The mock packages do not use the gcc-runtime
+    def mock_has_runtime_dependencies(*args, **kwargs):
+        return True
+
+    monkeypatch.setattr(
+        spack.solver.asp, "_has_runtime_dependencies", mock_has_runtime_dependencies
+    )
+    # The default mpi version is 3.x provided by mpich in the mock repo.
+    # This test verifies that concretizing with an included concrete
+    # environment with "concretizer:reuse:true" the included
+    # concrete spec overrides the default with mpi@1.0.
+    test1, _, combined = setup_combined_multiple_env()
+
+    # Set the reuse mode for the environment
+    override_env = configure_reuse(reuse_mode, combined)
+    if override_env:
+        # If there is an override environment (ie. testing reuse with
+        # an external environment) update it here.
+        test1 = override_env
+
+    # Capture the test1 specs included by combined
+    test1_specs_by_hash = test1.specs_by_hash
+
+    try:
+        # Add mpileaks to the combined environment
+        with combined:
+            add("mpileaks")
+            combined.concretize()
+        comb_specs_by_hash = combined.specs_by_hash
+
+        # create reference env with mpileaks that does not use reuse
+        # This should concretize to the default version of mpich (3.0)
+        env("create", "new")
+        ref_env = ev.read("new")
+        with ref_env:
+            add("mpileaks")
+        ref_env.concretize()
+        ref_specs_by_hash = ref_env.specs_by_hash
+
+        # Ensure that the mpich used by the mpileaks is the mpich from the reused test environment
+        comb_mpileaks_spec = [s for s in comb_specs_by_hash.values() if s.name == "mpileaks"]
+        test1_mpich_spec = [s for s in test1_specs_by_hash.values() if s.name == "mpich"]
+        assert len(comb_mpileaks_spec) == 1
+        assert len(test1_mpich_spec) == 1
+        assert comb_mpileaks_spec[0]["mpich"].dag_hash() == test1_mpich_spec[0].dag_hash()
+
+        # None of the references specs (using mpich@3) reuse specs from test1.
+        # This tests that the reuse is not happening coincidently
+        assert not any([s in test1_specs_by_hash for s in ref_specs_by_hash])
+
+        # Make sure the raise tests raises
+        assert "raise" not in reuse_mode
+    except ev.SpackEnvironmentError:
+        assert "raise" in reuse_mode
+
+
 @pytest.mark.parametrize("unify", [True, False, "when_possible"])
 def test_env_include_concrete_env_reconcretized(unify):
     """Double check to make sure that concrete_specs for the local specs is empty

lib/spack/spack/test/env.py

@@ -906,3 +906,18 @@ def test_only_roots_are_explicitly_installed(tmp_path, mock_packages, config, te
     assert callpath in temporary_store.db.query(explicit=False)
     env.install_specs([mpileaks], fake=True)
     assert temporary_store.db.query(explicit=True) == [mpileaks]
+
+
+def test_environment_from_name_or_dir(mock_packages, mutable_mock_env_path, tmp_path):
+    test_env = ev.create("test")
+
+    name_env = ev.environment_from_name_or_dir(test_env.name)
+    assert name_env.name == test_env.name
+    assert name_env.path == test_env.path
+
+    dir_env = ev.environment_from_name_or_dir(test_env.path)
+    assert dir_env.name == test_env.name
+    assert dir_env.path == test_env.path
+
+    with pytest.raises(ev.SpackEnvironmentError, match="no such environment"):
+        _ = ev.environment_from_name_or_dir("fake-env")