Support conditional and optional includes by scheibelp · Pull Request #46792 · spack/spack

scheibelp · 2024-10-04T20:30:56Z

UPDATE: Instead of the original idea of supporting include: as a subsection of config:, this PR has changed to support include: as an independent schema while retaining the new capabilities of supporting conditional, unconditional, and optional includes. The features are supported through spack:include: with essentially the same include syntax, i.e.:

include:
- path: "/path/to/a/config-dir-or-file"
  when: os == "ventura"
- "/another/path/added/unconditionally"
- path: "/base/dir/with/spack/variables/$os/$target/"
  optional: true

This PR also supports conditional and optional includes for environment include paths.

TODO

replace config: include: with include: support
Update docs
support URLs for global includes (as has been supported by environment includes)
support relative paths for global includes (relative to the source path)
resolve issues with unit tests
Change spack commands (fish scopes) completion message to note that user's includes need to be left off
resolve circular import issue introduced with above change(s)
Revise docs as needed
Move the commits to a separate PR as requested (Support independent includes with conditional, optional, and remote entries #48784)

@greenc-FNAL @tgamblin

This adds an include: subsection to config:. The include: subsection specifies a list of config paths (either directories or single files) that should be added as scopes to the configuration. For example:

config:
  include:
  - path: "/path/to/a/config-dir-or-file"
    when: os == "ventura"
  - "/another/path/added/unconditionally"
  - path: "/base/dir/with/spack/variables/$os/$target/"
    optional: true

If this were part of the site config scope, or added with spack -C, then all Spack commands would run with the config from /path/to/a/config-dir-or-file (e.g. spack config blame) when on Mac Ventura.

Inclusion is currently not recursive: if you include another config.yaml that itself defines includes, you will not get those includes.

This includes support for conditional activation of configs, using when, which functions like when: for spec list definitions (see: https://spack.readthedocs.io/en/latest/environments.html#spec-list-references). The when string is evaled, so can support something like os == "ventura" and target == ....

Altogether, this is intended as a more general alternative to #40018: you could conditionally activate a config for each os and/or target.

tgamblin

This looks awesome to me.

I think the PR description is not quite specific enough:

If this were part of the site config scope, or added with spack -C, then all Spack commands would run with the config from /path/to/a/config-dir-or-file (e.g. spack config blame).

I think it needs "when on macOS Ventura".

Other than that a few cosmetic requests below.

lib/spack/spack/schema/config.py

greenc-FNAL · 2024-10-08T21:53:28Z

Thanks for this, @scheibelp. I've asked the rest of the team for their comments, but my $0.02:

At first glance, this looks great, thank you! My one thought is that a Spack config for a CVMFS area supporting multiple OSes and/or targets could get pretty verbose unless we were able to have evaluable paths (e.g. /path/to/config/$os). That feature would also probably need to come with the ability to specify optional includes that inform rather than raise an exception if the file or directory is not present as calculated for the executing system (assuming that's not already the default behavior).

scheibelp · 2024-10-09T17:47:33Z

My one thought is that a Spack config for a CVMFS area supporting multiple OSes and/or targets could get pretty verbose unless we were able to have evaluable paths (e.g. /path/to/config/$os).

That sounds like a good idea: for this PR in its current form you would need a long list of include: to address your use case, but something like:

- include:
  - path: /a/b/c/$os/$target
    skip_if_nonexistent: true

would allow for a compact include file. ~~That will take me a couple days to address here.~~ (done now)

scheibelp · 2024-10-09T22:44:24Z

My one thought is that a Spack config for a CVMFS area supporting multiple OSes and/or targets could get pretty verbose unless we were able to have evaluable paths (e.g. /path/to/config/$os).

That sounds like a good idea: for this PR in its current form you would need a long list of include: to address your use case, but something like:

That is now supported (and tested) - that took less time than I originally assumed.

tgamblin · 2024-10-10T06:44:20Z

Requesting a review from @tldahlgren and @becker33 as I think they should give this a glance to see if it introduces any tricky config layering issues. They dealt with these from working on #33768.

lib/spack/spack/config.py

lib/spack/spack/environment/environment.py

greenc-FNAL · 2024-10-22T15:36:38Z

@scheibelp Does this only allow include for config items, or also for e.g. packages, compilers, et al?

tgamblin · 2024-10-22T16:23:31Z

@greenc-FNAL the plan is to make include: its own top-level thing, not config:include:

scheibelp · 2024-10-22T16:35:13Z

Does this only allow include for config items, or also for e.g. packages, compilers, et al?

@greenc-FNAL the plan is to make include: its own top-level thing, not config:include:

It its current form, you can include any config section you want: an include:d path can for example refer to a directory with a packages.yaml, repos.yaml, etc. It can also refer to a merged configuration that is stored in a single file (which can have a packages: section, a repos: section, and so on).

That will also be true when it is updated as Todd mentions.

…aths

…eError

…lude scopes

tldahlgren · 2025-01-30T02:07:53Z

Closing since superseded by #48784 .

…ntries (#48784) Supersedes #46792. Closes #40018. Closes #31026. Closes #2700. There were a number of feature requests for os-specific config. This enables os-specific config without adding a lot of special sub-scopes. Support `include:` as an independent configuration schema, allowing users to include configuration scopes from files or directories. Includes can be: * conditional (similar to definitions in environments), and/or * optional (i.e., the include will be skipped if it does not exist). Includes can be paths or URLs (`ftp`, `https`, `http` or `file`). Paths can be absolute or relative . Environments can include configuration files using the same schema. Remote includes must be checked by `sha256`. Includes can also be recursive, and this modifies the config system accordingly so that we push included configuration scopes on the stack *before* their including scopes, and we remove configuration scopes from the stack when their including scopes are removed. For example, you could have an `include.yaml` file (e.g., under `$HOME/.spack`) to specify global includes: ``` include: - ./enable_debug.yaml - path: https://github.com/spack/spack-configs/blob/main/NREL/configs/mac/config.yaml sha256: 37f982915b03de18cc4e722c42c5267bf04e46b6a6d6e0ef3a67871fcb1d258b ``` Or an environment `spack.yaml`: ``` spack: include: - path: "/path/to/a/config-dir-or-file" when: os == "ventura" - ./path/relative/to/containing/file/that/is/required - path: "/path/with/spack/variables/$os/$target" optional: true - path: https://raw.githubusercontent.com/spack/spack-configs/refs/heads/main/path/to/required/raw/config.yaml sha256: 26e871804a92cd07bb3d611b31b4156ae93d35b6a6d6e0ef3a67871fcb1d258b ``` Updated TODO: - [x] Get existing unit tests to pass with Todd's changes - [x] Resolve new (or old) circular imports - [x] Ensure remote includes (global) work - [x] Ensure remote includes for environments work (note: caches remote files under user cache root) - [x] add sha256 field to include paths, validate, and require for remote includes - [x] add sha256 remote file unit tests - [x] revisit how diamond includes should work - [x] support recursive includes - [x] add recursive include unit tests - [x] update docs and unit test to indicate ordering of recursive includes with conflicting options is deferred to follow-on work --------- Signed-off-by: Todd Gamblin <[email protected]> Co-authored-by: Peter Scheibel <[email protected]> Co-authored-by: Todd Gamblin <[email protected]>

spackbot-app bot added core PR affects Spack core functionality environments tests General test capability(ies) labels Oct 4, 2024

tgamblin self-requested a review October 4, 2024 21:18

tgamblin requested changes Oct 6, 2024

View reviewed changes

lib/spack/spack/schema/config.py Outdated Show resolved Hide resolved

lib/spack/spack/schema/config.py Outdated Show resolved Hide resolved

lib/spack/spack/schema/config.py Outdated Show resolved Hide resolved

alalazo assigned tgamblin Oct 7, 2024

spackbot-app bot added the commands label Oct 7, 2024

scheibelp marked this pull request as draft October 9, 2024 17:48

scheibelp marked this pull request as ready for review October 9, 2024 22:40

tgamblin requested a review from tldahlgren October 10, 2024 06:40

tldahlgren self-assigned this Oct 10, 2024

tgamblin requested a review from becker33 October 11, 2024 17:08

alalazo added this to the v0.23.0 milestone Oct 14, 2024

tldahlgren mentioned this pull request Oct 14, 2024

Discussion: Environments: Support fully including other environments and standardize include syntax #46973

Open

1 task

tldahlgren force-pushed the features/includes-as-config branch from 73a1699 to 0e01a4e Compare October 22, 2024 01:08

tldahlgren requested changes Oct 22, 2024

View reviewed changes

lib/spack/spack/config.py Outdated Show resolved Hide resolved

lib/spack/spack/environment/environment.py Outdated Show resolved Hide resolved

tldahlgren force-pushed the features/includes-as-config branch 2 times, most recently from b54925b to 4e5e087 Compare October 22, 2024 01:20

tldahlgren force-pushed the features/includes-as-config branch from 4e5e087 to 5254868 Compare October 23, 2024 00:50

tldahlgren changed the title ~~Includes as config~~ Support conditional and optional includes Oct 31, 2024

tldahlgren force-pushed the features/includes-as-config branch 2 times, most recently from d21a886 to 0b257f1 Compare October 31, 2024 02:27

spackbot-app bot added the documentation Improvements or additions to documentation label Oct 31, 2024

tldahlgren added 19 commits January 27, 2025 18:51

Update fish completion

a2ec67d

config.scopes_from_paths: add docstring spacing for readthedocs

57b25c5

Update the copyright lines for the new files

527667e

schemas: restore literal lines since new copyright is one-liner

d8311b8

_concretize_together_where_possible: Remove commented out import

871be9c

Replace scopes_from_paths with include_path_scope

d2cde24

main: remove unused import

a1c6954

Moved remote config retrieval out of config.py

05a893f

Resolve style issues plus add include_path_scope return to docstring

34116e1

test_config_change_existing: do NOT use normal misc cache path

30f93b2

Snapshot: added test_include_duplicate_source; some config cleanup

9ae4e0b

config: remove unused remote_path

4947059

test_env_with_included_config_precedence: fix docstring typo

58605b8

Refactor retention and handling of sources associated with included p…

ac2d8dc

…aths

Cache web-specific config settings in util.web_config; move RemoteFil…

5242e16

…eError

Cleanup prior; add remote_file_cache.py

e1760c9

Ignore type hint for passing config.CONFIG into web_config.update()

c7f78a5

Move path processing from config to remote file utility

2246bdf

Fix readthedocs FetchError complaint

38fa694

tldahlgren force-pushed the features/includes-as-config branch from 30c32bc to 38fa694 Compare January 28, 2025 22:42

tldahlgren added 4 commits January 28, 2025 16:31

Restore environment include ordering lost in refactor

0c6f381

Add information on path options for global includes

a50c961

test_updated_completion_scripts: change message to exclude global inc…

fd13bb7

…lude scopes

Add missing colon to exception in docstring

80c18c6

tldahlgren mentioned this pull request Jan 30, 2025

Support independent includes with conditional, optional, and remote entries #48784

Merged

10 tasks

tldahlgren closed this Jan 30, 2025

tgamblin removed this from the v1.0.0 milestone Jan 30, 2025

alalazo mentioned this pull request Feb 24, 2025

Assign priorities to configuration scopes #48420

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support conditional and optional includes#46792

Support conditional and optional includes#46792
scheibelp wants to merge 48 commits intospack:developfrom
scheibelp:features/includes-as-config

scheibelp commented Oct 4, 2024 •

edited by tldahlgren

Loading

Uh oh!

tgamblin left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greenc-FNAL commented Oct 8, 2024 •

edited

Loading

Uh oh!

scheibelp commented Oct 9, 2024 •

edited

Loading

Uh oh!

scheibelp commented Oct 9, 2024

Uh oh!

tgamblin commented Oct 10, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

greenc-FNAL commented Oct 22, 2024 •

edited

Loading

Uh oh!

tgamblin commented Oct 22, 2024

Uh oh!

scheibelp commented Oct 22, 2024

Uh oh!

tldahlgren commented Jan 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

scheibelp commented Oct 4, 2024 • edited by tldahlgren Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greenc-FNAL commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scheibelp commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scheibelp commented Oct 9, 2024

Uh oh!

tgamblin commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greenc-FNAL commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Oct 22, 2024

Uh oh!

scheibelp commented Oct 22, 2024

Uh oh!

tldahlgren commented Jan 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scheibelp commented Oct 4, 2024 •

edited by tldahlgren

Loading

greenc-FNAL commented Oct 8, 2024 •

edited

Loading

scheibelp commented Oct 9, 2024 •

edited

Loading

tgamblin commented Oct 10, 2024 •

edited

Loading

greenc-FNAL commented Oct 22, 2024 •

edited

Loading