Skip to content

Conversation

@alalazo
Copy link
Member

@alalazo alalazo commented Dec 17, 2025

fixes #51112

On develop nodes in a DAG get a penalty of 1 when a variant value is not the default. This might lead to cases of non-determinism, due to having multiple optimal solutions.

Here we give different penalties to different non-default values, thereby reducing the non-determinism in concretization.

@alalazo alalazo added concretization bugfix Something wasn't working, here's a fix labels Dec 18, 2025
@alalazo
Copy link
Member Author

alalazo commented Dec 18, 2025

This bugfix comes with a slight regression:

radiuss.develop.csv
radiuss.pr.csv

comparison

@haampie haampie closed this Dec 19, 2025
@haampie haampie reopened this Dec 19, 2025
@haampie
Copy link
Member

haampie commented Dec 19, 2025

Last comment here before Christmas break:

I would still try to avoid the sorted call on the values. Yes, sorted leads to determinism, but since Spack has no clue what the values represent, it is effectively the same as randomization: if the default fabric in libfabric does not work, you get the next one in alphabetical order. If the C standard C17 does not work, you get C99 because it comes next in alphabetical order, and if that doesn't, you get C11. That makes no sense.

However, apparently we do not keep track of the order as defined in package.py but throw values into sets, meaning that the scope of this PR becomes larger.

I think that's worth it, since merging this with sorted and fixing it later means there's an unnecessary concretizer change.

For what it's worth, if someone else wants to review this PR in the meantime and has different opinions, that's fine. I'll catch up early January.

@alalazo alalazo force-pushed the solver/variant-rework branch from b58d353 to ae9a1dc Compare December 22, 2025 18:32
fixes spack#51112

On develop nodes in a DAG get a penalty of 1 when a variant
value is not the default. This might lead to cases of
non-determinism, due to having multiple optimal solutions.

Here we give different penalties to different non-default values,
thereby reducing the non-determinism in concretization.

Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
This requires modifications to DinjointSetsOfValues to
store the values in the order they are declared.

A few unit tests have been modified to ensure we check
the order is preserved.

Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
@haampie haampie merged commit f3d5ce3 into spack:develop Jan 9, 2026
31 of 32 checks passed
@alalazo alalazo deleted the solver/variant-rework branch January 9, 2026 16:26
alalazo added a commit to alalazo/spack that referenced this pull request Jan 14, 2026
…ction

Fixes a bug introduced in spack#51780

Variants defined with a validator function can have:
```
pkg_fact(Package, variant_possible_value(VariantID, Value))
```
that are not associated with a penalty and are not part of the
variant definition. In this case set the penalty to a very high
value to avoid selecting them accidentally.

Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Something wasn't working, here's a fix concretization solver unit-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix non-determinism in variant values

2 participants