Per-os-config feature -- third time is the charm? #40018
Per-os-config feature -- third time is the charm? #40018marcmengel wants to merge 4 commits intospack:developfrom
Conversation
|
@scheibelp -- pinging you to include this in your more general scope cleanup/changes. |
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
I have a couple preliminary requests:
- No need for tests
- OS scope should live inside platform scope
Also, It might make sense to augment https://spack.readthedocs.io/en/latest/configuration.html#platform-specific-scopes with a mention of OS-specific scopes (the list itself doesn't need to expand, I think the text that follows can explain there is another level).
Other considerations (these aren't problems, I'm just making note):
- Correct behavior of
highest_precedence_non_platform_scopeseems to be preserved (I would say that OS scopes should be excluded as well as platform scopes, which this would seem to do) - Augmenting of command line scopes with per-OS scope (analogous to platform) seems reasonable
- Overall, I think this will improve behavior of Spack on shared filesystems (we have other plans that should also help, like removing the
~scope and just using a scope associated with the Spack instance, but for people who use the same spack instance on a shared FS across different system logins, this should be useful)
|
I'm having some ongoing discussions with folks about how we want to customize this:
My goal is to have a clear path forward on this by the end of October 9 (this is delayed in part because I plan to be out next week). |
|
Actually, yes, doing platform, platform/os, platform/os/target (mirroring the "spack arch" platform-os-target triple) has a |
|
Re-requesting review, as this now does all three of the platform/os/target subdirectories. |
|
@scheibelp Ping? |
greenc-FNAL
force-pushed
the
per_os_config_feature3
branch
from
beff7ed to
032c1f0
Compare
greenc-FNAL
force-pushed
the
per_os_config_feature3
branch
from
032c1f0 to
c7db03d
Compare
|
@spackbot style |
|
@scheibelp Ping? |
|
@spackbot help |
|
You can interact with me in many ways!
I'll also help to label your pull request and assign reviewers! |
|
@scheibelp Are we good to go here? I've fixed the style issue (whitespace only) and the only remaining failure is code coverage, but I believe you said tests weren't necessary? I'm not quite sure why GitHub keeps claiming that the branch can't be rebased, since I was able to rebase via |
|
@scheibelp Hello? |
|
I have been getting a weekly reminder for this and it continually is not at the front of my queue. The soonest I can guarantee looking at this is in two weeks (April 8th). Changing the config storage location can have unexpected side effects, so I need to put some effort into chasing down the possible issues. One thing that would need to change in this PR (in addition to a re-sync) is updating the docs at https://spack.readthedocs.io/en/latest/configuration.html#platform-specific-scopes |
greenc-FNAL
force-pushed
the
per_os_config_feature3
branch
from
db63bf1 to
ef20ebc
Compare
spackbot-app
bot
added
the
documentation
|
@scheibelp Understood, and thank you. PR has been re-synced, and documentation has been updated. |
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
One documentation request; also, I looked through the code base for references to platform scopes and see a couple:
- bootstrap.config._bootstrap_config_scopes appears to replicate some logic from spack.config.create/_add_platform_scope, I'm not sure if this should change: The issues that prompt the existence of this PR would also possibly benefit from per-target bootstrapping config, so I suggest refactoring it here.
- I also think we should update config.SCOPES_METAVAR - this is for help-display purposes, so this is more about what scope we should tell users to edit by default.
Other than that, I see no problems with adding these scopes
lib/spack/spack/config.py
Outdated
|
|
||
| scope_name = os.path.join(scope_name, host_target) | ||
| scope_path = os.path.join(scope_path, host_target) | ||
| cfg.push_scope(scope_type(scope_name, scope_path)) |
There was a problem hiding this comment.
(no change needed)
Originally I was thinking of reducing this from 3 scopes to two:
platform/(we keep this because it's what we had before)platform/os- get rid of this becuase...platform/os/target- this should handle the prior two (we only keep the first because there may be preexisting config in there)
However, users might want to manually edit the os/ scope (or the base scope) for config that should apply for different systems.
lib/spack/docs/configuration.rst
Outdated
| #. ``defaults`` | ||
| #. ``defaults/<platform>`` | ||
| #. ``defaults/<platform>/<os>`` | ||
| #. ``defaults/<platform>/<os>/<target>`` |
There was a problem hiding this comment.
Of these 3, I think we can just document the last (defaults/<platform>/<os>/<target>): any automatic modifications will occur to this scope vs. the other two.
We can briefly mention defaults/<platform> (and os/) as an available scope after this list
|
@scheibelp Could you confirm what action is needed to finalize this, please? I'm seeing:
Is that it, or am I missing and/or inventing anything? |
greenc-FNAL
force-pushed
the
per_os_config_feature3
branch
from
00cda1e to
c217c91
Compare
|
@scheibelp I believe we have satisfied all outstanding comments; a final review would be appreciated. |
greenc-FNAL
force-pushed
the
per_os_config_feature3
branch
from
c217c91 to
0023ab9
Compare
|
Rebased only. |
cast to string add unit test to try to improve coverage flake8 use os.path, because windows.. style remove test test cleanup 2 draft of moving os scope inside platform scope add scope directories for all 3 of arch components, platform, os, target Style fix: whitespace
greenc-FNAL
force-pushed
the
per_os_config_feature3
branch
from
0023ab9 to
3986926
Compare
|
Rebased only. |
|
@scheibelp Bump? |
|
@spackbot help |
|
You can interact with me in many ways!
I'll also help to label your pull request and assign reviewers! |
…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]>
5 participants
The previous pull requests for this #16834 and #31028 got funky after rebasing, so doing a fresh branch.
This is an implementation for Feature requests #31026, and #2700 fixed for latest version.
It just adds an Operating System Release (i.e. "scientific7" "almalinux9") config branch along the lines of the the
platform one ("linux" "MacOs"). Many configs (i.e. packages.yaml) are more specific than just platform, and this
lets you support multiple OS versions in one spack tree.