Add configs for reusing specs from environments and enable reuse from included concrete environments by default

[kwryankrattiger]

Add option to reuse specs from included concrete environments. The default is to reuse.

[kwryankrattiger]

Currently the reuse concrete test is failing. I am able to get a successful concretization using a local environment test and cmake version, but getting the test to work the versions from virtuals is proving to be more finicky.

[tldahlgren]

Very preliminary feedback...

[tldahlgren]

Including @tgamblin per his off-line request.

[tgamblin]

@kwryankrattiger can we get the tests passing on this one again?

[kwryankrattiger]

After talking with @becker33 I think the way reuse is defined for concrete includes is going to change. The current implementation applies reuse as a property of the incude-concrete. However, this is a bit confusing as it is unclear what should be expected when concretizing with concretizer:reuse:false.

To make this simpler to reason about the concretization reuse settings for reusing concrete specs from included environments will move under the concretizer:reuse section. TIL, this is a highly configurable section!

Here is an example of some use cases. The main additions here are adding the environment type which is either the key to the path/name of the environment or value. This idea could probably be extended to specify reuse from specific build caches as well. For this PR only the environment name/path will be used.

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

  include-concrete:
  - my_env
  - my_other_env
  ...

CC: @becker33 @tgamblin

[becker33]

I think we need another test (or maybe another parametrization) for testing reusing from an environment that is not included.

This is looking good in general, I think it's getting close

[kwryankrattiger]

@becker33 I am thinking about the case of spack concretize -f where I want to clear the concretized specs from the environment, but I don't want to re-read the included concrete. Currently there is no way to do that, you must re-read the included concrete environments or reuse the existing concrete specs in the environment.

Any ideas on how to express that? I was thinking about adding an option value force. Maybe something like -f full ie. force a re-read of includes and flush all concrete specs. Then the default here would be to preserver included concrete envs.

My thought on this is I maybe be consuming someone else's concrete environment and iterating, but they changed their lock and I didn't want to update yet. The idea of requiring full in that case over full being the default and not adding a --keep-included is I think keeping included concrete is the happy path in most cases.

[kwryankrattiger]

I think we need another test (or maybe another parametrization) for testing reusing from an environment that is not included.

I added tests for reuse of non-existent mirror (from_environment_raise) and listing an external mirror (from_environment_external_test).

I also noticed that we were not correctly distinguishing between included and external environments when reusing the environment by path/name. I added a check and a _specs_from_environment_included_concrete to handle that case correctly.

[becker33]

@becker33 I am thinking about the case of spack concretize -f where I want to clear the concretized specs from the environment, but I don't want to re-read the included concrete. Currently there is no way to do that, you must re-read the included concrete environments or reuse the existing concrete specs in the environment.

Any ideas on how to express that? I was thinking about adding an option value force. Maybe something like -f full ie. force a re-read of includes and flush all concrete specs. Then the default here would be to preserver included concrete envs.

My thought on this is I maybe be consuming someone else's concrete environment and iterating, but they changed their lock and I didn't want to update yet. The idea of requiring full in that case over full being the default and not adding a --keep-included is I think keeping included concrete is the happy path in most cases.

I don't think we should do anything to enable reusing without updating the concretizations from the upstream environment. Updating your included environments is part of the contract of concretize -f. If we want to change that later, that's an orthogonal PR.

[kwryankrattiger]

Updating your included environments is part of the contract of concretize -f

I agree with that. But there are cases where I want to keep by current includes, but I have modified other parts of my environment that I want to re-concretize against. I am thinking about a different option which does -f, but without overriding includes.

I don't think it needs to be in the PR, just a thought.

[alalazo]

Just saw this change on develop. Curious why this choice:

from:
- type:
     environment: my_env

instead of this choice:

from:
- type: environment
  name: my_env

which could have also applied to buildcaches:

from:
- type: buildcache
  name: developer-tools

Are we in time for a hotfix in case? 🙂

EDIT: The issue I see with this is the one we do frequently to encode specific information in a key, which makes the schema not easily extendable. The example above with name: was in the description of the original PR introducing the feature #42782

EDIT2: If name is also too specific, we could use ref, or something along those lines. Point here is we can avoid an extra level of dicts, and having to redefine another schema for buildcaches.

[tgamblin]

@kwryankrattiger or @becker33 can you please update the description here to be more descriptive?

[alalazo]

@kwryankrattiger I have two questions on this PR as I'm trying to implement #49707

1. This feature was never documented. Was that on purpose because it's "experimental" etc. or should we document it?
2. Why do we reuse a possibly outdated version of a lockfile if the environment is among the included environments?

[kwryankrattiger]

1. So as far as this goes it was more a bug fix than a new feature imo We could probably improve the documentation around this a bit more though, but it wasn't part of the scope of this change.
2. For the case of include concrete part of the feature was to include a "snapshot" of a concrete environment. It is up to the user to say when to update the included concrete lockfile. That should be handled via the -f flag.

[alalazo]

I just found out that these tests are buggy, since they don't reflect what user would see when working with an environment from cli.

The issue is that to compute reused specs we always rely on the active_environment, while here we concretize the combined environment outside of:

with combined:
    ...

so there is no active_environment. If there was, a few assertions on unify: false would fail.

Question is: would it be fine to change the expectations to match the current behavior (i.e. specs from an included environment are "reused" when unify: false)? I would say "yes" since it will be more consistent in general to behave like that, but wondering if there are use cases I missed.

[kwryankrattiger]

The distinction between reuse and unify are important here. unfiy:false will disable the strict unification, but if reuse:true then we will include existing environment specs (ie. the included concrete specs) as reusable.

It is also possible to configure unify:false and only reuse from some of the included concrete environments.

Here you include foo and bar, but only reuse from foo when solving. bar specs are just injected into the environment in addition, but they are not to be included in the solve.

include_concrete:
    - foo
    - bar
unify: false
reuse:
  from: [
    type: environment
    path: foo
  ]