Assign priorities to configuration scopes by alalazo · Pull Request #48420 · spack/spack

alalazo · 2025-01-06T16:28:17Z

Currently, environments can end up with higher priority than -C custom config scopes and -c command line arguments sometimes. This shouldn't happen -- those explicit CLI scopes should override active environments.

Up to now configuration behaved like a stack, where scopes could be only be pushed at the top. 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 f1d0371
Remove Configuration.pop_scope 61dd2f2
Remove unify:false from custom -C scope in pipelines 4a5a12d On develop, pipelines are relying on the environment being able to override -C scopes, which is a bug. After this fix, we need to be explicit about the unification strategy in each stack, and remove the blanket unify:false from the highest priority scope

alalazo · 2025-01-07T07:42:13Z

Failure in pipelines is unrelated, and due to system errors:

lib/spack/llnl/util/lang.py

psakievich

I've looked over the code and the concept seems sound to me. The idea of stack like behavior within a given priority is a more robust design. It also seems truer to our description of how configs will work with the users. I'm still mulling a bit about the defaults selected but not to the point of wanting to hold up a merge. Thanks @alalazo for taking the time to shore up this design.

lib/spack/llnl/util/lang.py

haampie · 2025-01-10T15:46:02Z

I don't see any code change in bootstrap 🤔 what about

spack -c config:install_tree:root:/tmp spec zlib

bootstrapping to /tmp instead of ~/.spack/bootstrap?

alalazo · 2025-01-10T16:19:40Z

@haampie There shouldn't be any change to bootstrap. The bootstrap store is specified in another place, so if you want to modify it from the command line you should:

$ spack -c bootstrap:root:/tmp spec zlib

even though the simpler way to do it is:

$ spack bootstrap root /tmp

Anyhow, I'm not aiming at changing any bootstrap semantic in this PR.

wdconinc · 2025-02-24T01:07:55Z

@haampie I think this PR is approved by reviewers, but there are some unresolved conversations by you. Just wanna make sure it didn't fall off the radar.

alalazo · 2025-02-24T07:21:06Z

@wdconinc This was supposed to wait a bit, so #46792 (which later became #48784) could be merged. I think it's now clear that the blocking PR needs more time to be merged than originally foreseen.

Also, I now see I need to "fix" hep pipelines generation here. On develop we have:

unify: when_possible

Unfortunately, we rely on the bug being fixed to get it, so it seems we are using unify: false in this branch.

This commit removes the "unify:false" in higher priority custom scopes, and makes the unification strategy explicit in each pipeline.

wdconinc · 2025-02-24T16:47:50Z

Also, I now see I need to "fix" hep pipelines generation here.

@alalazo If you want, we can switch the hep pipeline to unify: false as well. There are several expensive builds in there, though, and I'd want to make sure we don't build them more than necessary. If that's a direction to go in, I can do that in a separate PR. Let me know.

Alternatively, I was trying to move those hep pipelines closer to unify: true, so maybe that's a direction to go in as well.

alalazo · 2025-02-24T16:57:24Z

@wdconinc I just pushed the fix in the last commit, just waiting to see pipelines ✔️ We can change the unification strategy in a following PR.

psakievich · 2025-02-25T00:15:12Z

So are we waiting on #48784 or not? I guess I'm a little lost on that. I'll just keep approving for now...😅

tgamblin · 2025-02-25T08:51:41Z

@kwryankrattiger see the pipeline changes here -- I think they'll be less relevant if we use less -C and more include:, but they're relevant at least with this change.

kwryankrattiger · 2025-02-25T09:28:57Z

@kwryankrattiger see the pipeline changes here -- I think they'll be less relevant if we use less -C and more include:, but they're relevant at least with this change.

It has been on my list to convert all of the stacks to include configs instead of using -C. It has been a source of more confusion than convenience and the original idea for dynamic scopes has proven to be better expressed through includes or using "config add".

alalazo · 2025-02-25T09:38:29Z

I missed one thing here: we also set a few default images with -C, so the pipelines rely on this ~~bug~~ behavior much more than I thought

We didn't see failures in CI here cause there was nothing to rebuild 😞 I'll submit a fix asap.

EDIT: Looking further into this I noticed that what we document in the code is different from what we have in docs here. The bug in the issue remains a bug, but the correct priority of -C scopes is disputable

This reverts commit 3ad99d7.

All the build jobs in pipelines are apparently relying on the bug that was fixed. The issue was not caught in the PR because generation jobs were fine, and there was nothing to rebuild. Reverting to fix pipelines in a new PR. This reverts commit 3ad99d7.

…49185)" This reverts commit 9e508b0.

Currently, environments can end up with higher priority than `-C` custom config scopes and `-c` command line arguments sometimes. This shouldn't happen -- those explicit CLI scopes should override active environments. Up to now configuration behaved like a stack, where scopes could be only be pushed at the top. 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` - [x] Remove `unify:false` from custom `-C` scope in pipelines On the last modification: on `develop`, pipelines are relying on the environment being able to override `-C` scopes, which is a bug. After this fix, we need to be explicit about the unification strategy in each stack, and remove the blanket `unify:false` from the highest priority scope Signed-off-by: Massimiliano Culpo <[email protected]>

All the build jobs in pipelines are apparently relying on the bug that was fixed. The issue was not caught in the PR because generation jobs were fine, and there was nothing to rebuild. Reverting to fix pipelines in a new PR. This reverts commit 3ad99d7.

alalazo added bugfix Something wasn't working, here's a fix v0.23.1 PRs to backport for v0.23.1 labels Jan 6, 2025

spackbot-app bot added core PR affects Spack core functionality environments stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies) utilities documentation Improvements or additions to documentation labels Jan 6, 2025

alalazo requested review from haampie, psakievich and tgamblin January 6, 2025 18:07

alalazo force-pushed the bugfix/config-priority-based-order branch from 2b17111 to 8c0cc36 Compare January 6, 2025 20:06

haampie reviewed Jan 9, 2025

View reviewed changes

lib/spack/llnl/util/lang.py Outdated Show resolved Hide resolved

psakievich approved these changes Jan 10, 2025

View reviewed changes

psakievich previously approved these changes Jan 10, 2025

View reviewed changes

haampie reviewed Jan 10, 2025

View reviewed changes

lib/spack/llnl/util/lang.py Outdated Show resolved Hide resolved

lib/spack/llnl/util/lang.py Outdated Show resolved Hide resolved

lib/spack/llnl/util/lang.py Outdated Show resolved Hide resolved

lib/spack/llnl/util/lang.py Outdated Show resolved Hide resolved

alalazo dismissed psakievich’s stale review via b16a1e8 January 10, 2025 16:38

tldahlgren mentioned this pull request Jan 10, 2025

Support conditional and optional includes #46792

Closed

9 tasks

psakievich previously approved these changes Jan 13, 2025

View reviewed changes

haampie mentioned this pull request Feb 3, 2025

Backports: v0.23.1 #48845

Merged

27 tasks

haampie added v0.23.2 PRs to backport for v0.23.2 and removed v0.23.1 PRs to backport for v0.23.1 labels Feb 19, 2025

wdconinc assigned haampie Feb 24, 2025

alalazo added 2 commits February 24, 2025 09:35

Add PriorityOrderedMapping

348f394

Use PriorityOrderedMapping instead of collections.OrderedDict for config

d870010

alalazo added 7 commits February 24, 2025 09:36

Add exceptions to docs

3c9299d

Add exceptions to docs

e89c366

Fix docstring

205baaa

Remove unneeded enum value

781b034

Rely on sort being stable, and on _priorities being ordered correctly

23de8af

Save a few lines

ef84bdc

pipelines: make unification strategy explicit

4a5a12d

This commit removes the "unify:false" in higher priority custom scopes, and makes the unification strategy explicit in each pipeline.

alalazo dismissed psakievich’s stale review via 4a5a12d February 24, 2025 08:55

alalazo force-pushed the bugfix/config-priority-based-order branch from b16a1e8 to 4a5a12d Compare February 24, 2025 08:55

spackbot-app bot added the gitlab Issues related to gitlab integration label Feb 24, 2025

psakievich approved these changes Feb 25, 2025

View reviewed changes

tgamblin approved these changes Feb 25, 2025

View reviewed changes

tgamblin added this to the v1.0.0 milestone Feb 25, 2025

tgamblin merged commit 3ad99d7 into spack:develop Feb 25, 2025
37 checks passed

alalazo deleted the bugfix/config-priority-based-order branch February 25, 2025 09:04

alalazo added a commit that referenced this pull request Feb 25, 2025

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

ec3ced8

This reverts commit 3ad99d7.

alalazo mentioned this pull request Feb 25, 2025

Revert "Assign priorities to configuration scopes" #49185

Merged

alalazo added a commit that referenced this pull request Feb 25, 2025

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

5c89a1d

…49185)" This reverts commit 9e508b0.

alalazo mentioned this pull request Feb 25, 2025

Assign priorities to configuration scopes (take 2) #49187

Merged

5 tasks

greenc-FNAL mentioned this pull request Feb 25, 2025

Merge fron upstream 2025-02-25 FNALssi/spack#511

Merged

alalazo mentioned this pull request Oct 22, 2025

config: CLI scopes should override environments #50853

Merged

3 tasks

Conversation

alalazo commented Jan 6, 2025 • edited by tgamblin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Jan 7, 2025

Uh oh!

Uh oh!

psakievich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

haampie commented Jan 10, 2025

Uh oh!

alalazo commented Jan 10, 2025

Uh oh!

wdconinc commented Feb 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

wdconinc commented Feb 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

psakievich commented Feb 25, 2025

Uh oh!

tgamblin commented Feb 25, 2025

Uh oh!

Uh oh!

kwryankrattiger commented Feb 25, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alalazo commented Jan 6, 2025 •

edited by tgamblin

Loading

alalazo commented Feb 24, 2025 •

edited

Loading

alalazo commented Feb 24, 2025 •

edited

Loading

alalazo commented Feb 25, 2025 •

edited

Loading