Assign priorities to configuration scopes (take 2) by alalazo · Pull Request #49187 · spack/spack

alalazo · 2025-02-25T10:36:49Z

Reverts #49185
fixes #48414
fixes #49188

Currently, the custom config scopes are pushed at the top when constructing configuration, and are demoted whenever a context manager activating an environment is used - see #48414 for details. Workflows that rely on the order in the docs are thus fragile, and may break

This PR allows to assign priorities to scopes, and ensures that scopes of lower priorities are always "below" scopes of higher priorities. When scopes have the same priority, what matters is the insertion order.

Modifications:

Add a mapping that iterates over keys according to priorities set when
adding the key/value pair
Use that mapping to allow assigning priorities to configuration scopes
Assign different priorities for different kind of scopes, to fix a bug, and
add a regression test
Simplify Configuration constructor
Remove Configuration.pop_scope

…49185)" This reverts commit 9e508b0.

Signed-off-by: Massimiliano Culpo <[email protected]>

alalazo · 2025-02-25T14:50:06Z

@tgamblin @kwryankrattiger I'm looking back at this, and found setting the right priority of -C more confusing than before, after seeing this in our docs.

In the code we always added -C scopes last, and explicitly kept "command_line" at top priority. The custom scopes were instead floating as shown in #48414.

In #48420 I tried to set custom > environment, under the rational that what is set from the command line is always higher priority. The PR was reverted because pipelines rely on -C being lower scope, as apparently docs prescribe.

In this second attempt, I switched the priority of custom scopes and environment, so that environment > custom scopes. This is done in 5eddf0b

This ordering reflects the docs, and should require no changes to pipelines. This seems also better in case other, non public, pipelines share the same -C idiom to construct the overall config.

Now, I'm just waiting for pipelines to build something correctly, then I'll revert dcabe1b Let me know if you have thoughts.

alalazo · 2025-02-25T14:55:48Z

Also, while looking back at this I also found another bug, see #49188 This is fixed in 865d824

kwryankrattiger

LGTM, I think overall a general improvement.

psakievich

The hero we didn't know we needed. Thanks @alalazo !

tgamblin · 2025-02-25T18:35:01Z

RE:

In #48420 I tried to set custom > environment, under the rational that what is set from the command line is always higher priority. The PR was reverted because pipelines rely on -C being lower scope, as apparently docs prescribe.

In this second attempt, I switched the priority of custom scopes and environment, so that environment > custom scopes. This is done in 5eddf0b

I think we're still not thinking about this quite right. There are a few things going on here:

The currently active environment in Spack, set with SPACK_ENV, -e, or -D
Environments OR config scopes included on the CLI via -C
Config options on the CLI set via -c

I think SPACK_ENV should be the lowest priority of these -- it's some environment the user has already activated with spacktivate or spack env activate.

The others are really all on the CLI, and I think we should respect the order they appear. e.g., if I do this:

spack -C my/config/directory -e some-env -c concretizer:unify:true

Then my/config/directory should be lowest precedence, some-env should override it, and concretizer:unify:true should override that. if I instead specify:

spack -e some-env -C my/config/directory -e some-env -c concretizer:unify:true

Then I think some-env should be lowest, then my/config/directory, then concretizer:unify:true, and so on.

So I think the scopes should really look more like this:

    BUILTIN = 0
    CONFIG_FILES = 1
    ENVIRONMENT = 2
    COMMAND_LINE = 3

ENVIRONMENT is for the active environment as set by SPACK_ENV. COMMAND_LINE should include anything on the CLI, regardless of how it was set. Does that mirror how CI currently works and allow us to override things on the CLI as expected?

I realize this might be tricky, as I think environments are currently set up in a common way regardless of whether they are set by SPACK_ENV, -e, -D, etc., so if it's complicated maybe these semantics should be a subsequent breaking PR, and this PR should just model the current behavior while introducing the concept of priorities.

But as it is, the description of the PR is incorrect: it doesn't fix #48414, right? It's more of a wontfix?

alalazo · 2025-02-25T19:19:08Z

it doesn't fix #48414, right? It's more of a wontfix?

It makes the behavior consistent with docs (which I was not aware of, before this morning). So in that sense it fixes the issue, because the order will be the same before and after the context manager of the reproducer in #48414.

I realize this might be tricky, as I think environments are currently set up in a common way regardless of whether they are set by SPACK_ENV, -e, -D, etc., so if it's complicated maybe these semantics should be a subsequent breaking PR, and this PR should just model the current behavior while introducing the concept of priorities

Breaking this into two PRs is not bad imo, as I think there are a few aspects to discuss for the breaking behavior.

If we go for cli order, then do we allow:

spack -e /path1 -e /path2 ...

? ~~In this case it seems -e means more "single file scope", while -C means directory scope.~~¹ Then which environment should we say it's activated? Is the spec list only that of the last environment, or the merge of all -e? 🤔

Right now, on develop it seems we only allow a single environment from the command line, which is the last one appearing.

Also, while it makes sense to treat cli options positionally, it's probably breaking some existing workflows to consider:

spack -c option -e /env-path ...

different from the opposite:

spack -e /env-path -c option ...

so I would separate the changes in two PRs.

Just saw Support spack --config-scope <env> #45046 so I guess we also have a difference between spack -C env1 -e env2 and vice versa. ↩

alalazo · 2025-02-25T19:49:09Z

There is another caveat to the breaking behavior. In this PR we have a single priority for the environment scope, meaning SPACK_ENV, -e, and -D environments are treated the same. Since there can be only a single environment active, it's clear how:

with environment(new_env):
    ...

should modify the scopes, before, within and after the context manager.

If we lump the -e and -D environments into the COMMAND_LINE, and keep the SPACK_ENV environment in ENVIRONMENT we need:

To distinguish between these two different kinds of environments, which have now different priority
Keep track another way of the position of the -e and -D environments within the COMMAND_LINE group
Decide at which priority the environment pushed by the context manager should go

Overall this seems to complicate a lot the current mechanism.

tgamblin · 2025-02-25T21:34:44Z

If we go for cli order, then do we allow:

spack -e /path1 -e /path2 ...

No, I don't think we do, as this sets the install behavior for spack install, and currently at least it only makes sense for one environment. I think allowing multiple -C for that case is fine, at least for now. You can pick which one you want specs from by setting it with -e, and all the others can be -C.

There is another caveat to the breaking behavior. In this PR we have a single priority for the environment scope, meaning SPACK_ENV, -e, and -D environments are treated the same. Since there can be only a single environment active, it's clear how:
with environment(new_env):
    ...

Yeah we'll need to work this out separately. Let's hold off on that until some subsequent PR to introduce breakage if needed. I don't like the current semantics (they are counterintuitive to me) but I also don't think they are frequently used, so holding off and just getting this in as-is is fine.

I think we should consider breaking this before 1.0 though -- as I don't think it's a large break outside our CI and I think the ordering could end up being counterintuitive in the long run.

alalazo · 2025-02-26T06:35:38Z

@tgamblin I disabled auto-merge to check pipelines and revert dcabe1b before merging this (that commit was there just to try rebuilds on all pipelines)

alalazo · 2025-02-26T08:00:40Z

Ok, errors in the pipelines are all system failures, so not relevant here. Linking for future reference:

The runner and builder images seem correct.

This reverts commit dcabe1b. Signed-off-by: Massimiliano Culpo <[email protected]>

Currently, the custom config scopes are pushed at the top when constructing configuration, and are demoted whenever a context manager activating an environment is used - see #48414 for details. Workflows that rely on the order in the [docs](https://spack.readthedocs.io/en/latest/configuration.html#custom-scopes) are thus fragile, and may break This PR allows to assign priorities to scopes, and ensures that scopes of lower priorities are always "below" scopes of higher priorities. When scopes have the same priority, what matters is the insertion order. Modifications: - [x] Add a mapping that iterates over keys according to priorities set when adding the key/value pair - [x] Use that mapping to allow assigning priorities to configuration scopes - [x] Assign different priorities for different kind of scopes, to fix a bug, and add a regression test - [x] Simplify `Configuration` constructor - [x] Remove `Configuration.pop_scope` --------- Signed-off-by: Massimiliano Culpo <[email protected]>

Currently, active environments, from the CLI or from `$SPACK_ENV`, will override configuration set on the command line. This is confusing, as users would normally expect command line options to override environment options (e.g. SPACK_ENV). See [more discussion here](#49187 (comment)). This PR changes how we process configuration options on the CLI so that: 1. Any CLI option overrides an active environment from `SPACK_ENV` 2. CLI options that add config scopes (`-D`, `-C`, and `-e`) are now considered for scope precedence in the order they appear on the CLI. What this means is that if you write: * `spack -C config_dir`: `config_dir` will override any active enviornment. * `spack -C config_dir -e env`: `env` will override `config_dir` * `spack -e env -C config_dir -e env`: `config_dir` will override `env` And similar for `-D`. This makes the order much more obvious. This is a breaking change, in that historically environments have been higher precedence than config scopes. - [x] rework scope adding in `main.py` - [x] add an action to handle config from CLI - [x] add shell tests and unit tests Signed-off-by: Todd Gamblin <[email protected]>

Currently, active environments, from the CLI or from `$SPACK_ENV`, will override configuration set on the command line. This is confusing, as users would normally expect command line options to override environment options (e.g. SPACK_ENV). See [more discussion here](#49187 (comment)). This PR changes how we process configuration options on the CLI so that: 1. Any CLI option overrides an active environment from `SPACK_ENV` 2. CLI options that add config scopes (`-D`, `-C`, and `-e`) are now considered for scope precedence in the order they appear on the CLI. What this means is that if you write: * `spack -C config_dir`: `config_dir` will override any active enviornment. * `spack -C config_dir -e env`: `env` will override `config_dir` * `spack -e env -C config_dir -e env`: `config_dir` will override `env` And similar for `-D`. This makes the order much more obvious. This is a breaking change, in that historically environments have been higher precedence than config scopes. - [x] rework scope adding in `main.py` - [x] add an action to handle config from CLI - [x] add shell tests and unit tests --------- Signed-off-by: Todd Gamblin <[email protected]>

Revert "Revert "Assign priorities to configuration scopes (#48420)" (#…

5c89a1d

…49185)" This reverts commit 9e508b0.

Configuration: custom scopes < environment scope

5eddf0b

Signed-off-by: Massimiliano Culpo <[email protected]>

spackbot-app bot added the update-package label Feb 25, 2025

alalazo added 2 commits February 25, 2025 14:54

Fix environment deactivation in context manager

865d824

Signed-off-by: Massimiliano Culpo <[email protected]>

gmake: change to trigger pipelines build

dcabe1b

Signed-off-by: Massimiliano Culpo <[email protected]>

alalazo force-pushed the revert-49185-revert-48420-bugfix/config-priority-based-order branch from 50de8de to dcabe1b Compare February 25, 2025 13:55

spackbot-app bot added the shell-support label Feb 25, 2025

kwryankrattiger reviewed Feb 25, 2025

View reviewed changes

psakievich previously approved these changes Feb 25, 2025

View reviewed changes

psakievich self-assigned this Feb 25, 2025

tgamblin previously approved these changes Feb 25, 2025

View reviewed changes

tgamblin enabled auto-merge (squash) February 25, 2025 21:36

alalazo disabled auto-merge February 26, 2025 06:31

Revert "gmake: change to trigger pipelines build"

09222ff

This reverts commit dcabe1b. Signed-off-by: Massimiliano Culpo <[email protected]>

alalazo dismissed tgamblin’s stale review via 09222ff February 26, 2025 08:04

alalazo dismissed psakievich’s stale review via 09222ff February 26, 2025 08:04

psakievich approved these changes Feb 26, 2025

View reviewed changes

tgamblin approved these changes Feb 26, 2025

View reviewed changes

tgamblin merged commit dbd5311 into develop Feb 26, 2025
37 checks passed

tgamblin deleted the revert-49185-revert-48420-bugfix/config-priority-based-order branch February 26, 2025 18:52

greenc-FNAL mentioned this pull request Mar 4, 2025

recipes/jsonnet CMake FNALssi/spack#521

Closed

tgamblin mentioned this pull request Jun 7, 2025

config: CLI scopes should override environments #50853

Merged

3 tasks

tgamblin mentioned this pull request Oct 24, 2025

Configuration scopes: fix severe ordering issue when -C and -e are used together #51461

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assign priorities to configuration scopes (take 2)#49187

Assign priorities to configuration scopes (take 2)#49187
tgamblin merged 5 commits intodevelopfrom
revert-49185-revert-48420-bugfix/config-priority-based-order

alalazo commented Feb 25, 2025 •

edited

Loading

Uh oh!

alalazo commented Feb 25, 2025 •

edited

Loading

Uh oh!

alalazo commented Feb 25, 2025

Uh oh!

kwryankrattiger left a comment

Uh oh!

psakievich left a comment

Uh oh!

tgamblin commented Feb 25, 2025 •

edited

Loading

Uh oh!

alalazo commented Feb 25, 2025 •

edited

Loading

Uh oh!

alalazo commented Feb 25, 2025

Uh oh!

tgamblin commented Feb 25, 2025

Uh oh!

alalazo commented Feb 26, 2025

Uh oh!

alalazo commented Feb 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

alalazo commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Feb 25, 2025

Uh oh!

kwryankrattiger left a comment

Choose a reason for hiding this comment

Uh oh!

psakievich left a comment

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

alalazo commented Feb 25, 2025

Uh oh!

tgamblin commented Feb 25, 2025

Uh oh!

alalazo commented Feb 26, 2025

Uh oh!

alalazo commented Feb 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alalazo commented Feb 25, 2025 •

edited

Loading

alalazo commented Feb 25, 2025 •

edited

Loading

tgamblin commented Feb 25, 2025 •

edited

Loading

alalazo commented Feb 25, 2025 •

edited

Loading