Add a "sticky" property to variants #28630

alalazo · 2022-01-27T11:06:23Z

Refers to #19736
Refers to #26721

This PR adds a new sticky argument to the variant directive. The argument is set to False by default, in which case the semantics of variants during concretization is the usual one. If set to True the variant being marked sticky cannot be changed by clingo during concretization and will either be set to:

An explicit value set in a spec literal
The default value set in package.py or in packages.yaml

Modifications:

Add a sticky argument to the variant directive
Add unit tests to verify the semantic of sticky
Add documentation
Re-add Add variant to allow unsupported compiler & CUDA combinations #19736 and make the allow-unsupported-compiler sticky

alalazo · 2022-01-27T15:10:53Z

@tgamblin @becker33 I'm waiting for GA to be working again before adding back the variant on CUDA, but the logic to make "sticky" variant work is already here.

haampie · 2022-01-27T15:42:53Z

lib/spack/docs/packaging_guide.rst

+The concretizer thus is not free to pick an arbitrary value to work
+around conflicts, but will error out instead.
+Setting this property on a variant is useful in cases where the
+variant allows some dangerous or controversial options (e.g. using unsupported versions


It's not very dangerous, is it? It's more about enforcing pedantic rules by default, but still allowing the user to opt out.

It's useful when the variant can be dangerous, not that making it sticky is dangerous

haampie · 2022-01-27T15:45:20Z

Should it also be tested that packages.yaml values for sticky variants are still preferences, not constraints? E.g. prefer ~x in packages.yaml but +x is "sticky" and there is a conflict on ~x so +x is concretized to nonetheless?

alalazo · 2022-01-27T15:51:21Z

@haampie Settings in packages.yaml override the default for package.py (it's already like that).

alalazo · 2022-01-27T15:55:01Z

lib/spack/spack/test/concretize_preferences.py

+    def test_sticky_variant_accounts_for_packages_yaml(self):
+        with spack.config.override(
+                'packages:sticky-variant', {
+                    'variants': '+allow-gcc'
+                }
+        ):
+            s = Spec('sticky-variant %gcc').concretized()
+            assert s.satisfies('%gcc') and s.satisfies('+allow-gcc')


This test checks that we account for packages.yaml to override the default in package.py

Yeah, I meant the other way around.

In my understanding packages.yaml specifies preferences, whereas the abstract spec input specifies constraints.

Now suppose the expressed preference in packages.yaml for the value of a sticky variant would be unsatisfiable, then I would find it consistent if the value toggled back.

So:

class MyPkg: variant('x', default=False, sticky=True) conflicts('+x')

packages: mypkg: variants: '+x'

should concretize to ~x instead of failing?

The reason being: (a) the stickiness applies to the particular variant default value the package author sets, and (b) if we later add support for discriminating between hard constraints in packages.yaml vs soft preferences (as we currently only have), it'd be consistent if everything in packages.yaml remains a preference until then

The stickiness applies to the variant, packages.yaml can override the default.

Okay, then I'll stop saying packages.yaml specifies preferences 🙃

I share some of @haampie's concerns here. In all other cases, packages.yaml is a soft preference, I'm not sure it's intuitive that it becomes a hard preference for sticky variants

So here's how I see it:

The preference is due to the fact that packages.yaml overrides the default in package.py (even when the variant is not sticky)

If the variant is sticky the default value cannot be changed by clingo

The claim in point 1. follows by:

spack/lib/spack/spack/solver/concretize.lp

Lines 485 to 501 in bae2a26

% The default value for a variant in a package is what is prescribed:

%

% 1. On the command line

% 2. In packages.yaml (if there's no command line settings)

% 3. In the package.py file (if there are no settings in

% packages.yaml and the command line)

%

variant_default_value(Package, Variant, Value)

:- variant_default_value_from_package_py(Package, Variant, Value),

not variant_default_value_from_packages_yaml(Package, Variant, _),

not variant_default_value_from_cli(Package, Variant, _).

variant_default_value(Package, Variant, Value)

:- variant_default_value_from_packages_yaml(Package, Variant, Value),

not variant_default_value_from_cli(Package, Variant, _).

variant_default_value(Package, Variant, Value) :- variant_default_value_from_cli(Package, Variant, Value).

Furthermore, if we use the semantics currently proposed here the variant can be sticky whether the packager selects the "permissive" or the "restrictive" behavior as a default in package.py. Think of having:

variant('allow-all-compilers', default=True, sticky=True)

I might want to say:

packages: foo: variants: ~allow-all-compilers

to prefer the restrictive default. With the semantic you propose instead that would be impossible from packages.yaml (if the permissive semantics is the default in package.py clingo would be happy to switch it in case I try to reset it in packages.yaml).

I can see what @haampie is saying... kind of. I'm having a hard time coming up with a use case for the soft preference for a sticky variant, though. It seems inconsistent to me that if the variant is sticky, and you override it, that it would not still be sticky. I've always thought of packages.yaml as overriding preferences and not semantics.

Do you have a use case for preference semantics for sticky variants @haampie?

With the hard constraints we are proposing in #27987, you'd be able to do:

packages: all: require:[+allow-all-compilers]

So we could wait for that.

I do think people will be very confused when they set the value for a sticky variant the current way, and it's no longer sticky when they do that. Also, I can't tell if @haampie expects the soft preferences to only fall back to the sticky default, or if he wants all options to be open to the concretizer once you specify a package preference for a sticky variant.

Personally I think sticky variants always being sticky is fine, because if the user wants soft preference semantics for them in packages.yaml, they can put a list of values instead of a single value. I wouldn't say that for regular variants, because I don't expect the user to list out all values of all variants. But I think it's more intuitive to me if the stickiness is preserved. You have to know the semantics of other variants to use them... why are these different?

But maybe a use case could convince me otherwise?

becker33

I think @haampie's concern is the main issue going forward with this.

lib/spack/spack/directives.py

becker33 · 2022-01-27T20:02:43Z

lib/spack/spack/test/concretize_preferences.py

+    def test_sticky_variant_accounts_for_packages_yaml(self):
+        with spack.config.override(
+                'packages:sticky-variant', {
+                    'variants': '+allow-gcc'
+                }
+        ):
+            s = Spec('sticky-variant %gcc').concretized()
+            assert s.satisfies('%gcc') and s.satisfies('+allow-gcc')


I share some of @haampie's concerns here. In all other cases, packages.yaml is a soft preference, I'm not sure it's intuitive that it becomes a hard preference for sticky variants

lib/spack/docs/packaging_guide.rst

…lt (spack#26721)" This reverts commit 33ef7d5.

alalazo · 2022-01-31T07:57:11Z

@corbett5 I have seen your comment in #26721 In case you want to try this PR to leave feedback, it should solve the issues seen in #19736 with clingo swapping the default value of the variant to work around the conflict. To allow unsupported compilers in this PR you just need to add:

packages:
  cuda:
    variants:  '+allow-unsupported-compilers'

to your packages.yaml

* Add sticky variants * Add unit tests for sticky variants * Add documentation for sticky variants * Revert "Revert 19736 because conflicts are avoided by clingo by default (spack#26721)" This reverts commit 33ef7d5. * Add stickiness to "allow-unsupported-compiler"

* Add a "sticky" property to variants (spack#28630) * cuda, nvhpc: update recipes from upstream/develop. * nvhpc: deploy 22.3, drop 21.2 and 22.1 * cuda: deploy 11.6.1 to match nvhpc 22.3 * py-pytorch: update patch checksum. Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>

spackbot-app bot added conflicts directives documentation Improvements or additions to documentation new-variant new-version tests General test capability(ies) labels Jan 27, 2022

alalazo requested review from becker33 and tgamblin January 27, 2022 15:09

haampie reviewed Jan 27, 2022

View reviewed changes

alalazo commented Jan 27, 2022

View reviewed changes

alalazo added 3 commits January 27, 2022 17:50

Add sticky variants

f595342

Add unit tests for sticky variants

f16578e

Add documentation for sticky variants

97020e2

alalazo force-pushed the feature/sticky_variants branch from 2983fcf to 97020e2 Compare January 27, 2022 16:50

becker33 requested changes Jan 27, 2022

View reviewed changes

alalazo added 3 commits January 29, 2022 14:06

Fix minor issues with docs

e86e680

Revert "Revert 19736 because conflicts are avoided by clingo by defau…

8f668f0

…lt (spack#26721)" This reverts commit 33ef7d5.

Add stickiness to "allow-unsupported-compiler"

6c76e1c

spackbot-app bot added build-systems update-package labels Jan 29, 2022

alalazo marked this pull request as ready for review January 29, 2022 14:40

alalazo mentioned this pull request Feb 2, 2022

Installing PyTorch 1.8.2 on Ubuntu 18.04 #28679

Closed

becker33 approved these changes Feb 2, 2022

View reviewed changes

becker33 merged commit cd04109 into spack:develop Feb 2, 2022

alalazo deleted the feature/sticky_variants branch February 2, 2022 18:05

olupton mentioned this pull request Apr 8, 2022

Deploy NVHPC 22.3 and CUDA 11.6.1 BlueBrain/spack#1553

Merged

github-actions bot mentioned this pull request Feb 10, 2023

deploy: show changed files in PR BlueBrain/spack#1828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a "sticky" property to variants #28630

Add a "sticky" property to variants #28630

alalazo commented Jan 27, 2022 •

edited

Loading

alalazo commented Jan 27, 2022

haampie Jan 27, 2022

becker33 Jan 27, 2022

haampie commented Jan 27, 2022 •

edited

Loading

alalazo commented Jan 27, 2022

alalazo Jan 27, 2022

haampie Jan 27, 2022 •

edited

Loading

alalazo Jan 27, 2022

haampie Jan 27, 2022

becker33 Jan 27, 2022

alalazo Jan 27, 2022

tgamblin Jan 28, 2022 •

edited

Loading

becker33 left a comment

becker33 Jan 27, 2022

alalazo commented Jan 31, 2022

	% The default value for a variant in a package is what is prescribed:
	%
	% 1. On the command line
	% 2. In packages.yaml (if there's no command line settings)
	% 3. In the package.py file (if there are no settings in
	% packages.yaml and the command line)
	%
	variant_default_value(Package, Variant, Value)
	:- variant_default_value_from_package_py(Package, Variant, Value),
	not variant_default_value_from_packages_yaml(Package, Variant, _),
	not variant_default_value_from_cli(Package, Variant, _).

	variant_default_value(Package, Variant, Value)
	:- variant_default_value_from_packages_yaml(Package, Variant, Value),
	not variant_default_value_from_cli(Package, Variant, _).

	variant_default_value(Package, Variant, Value) :- variant_default_value_from_cli(Package, Variant, Value).

Add a "sticky" property to variants #28630

Add a "sticky" property to variants #28630

Conversation

alalazo commented Jan 27, 2022 • edited Loading

alalazo commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie commented Jan 27, 2022 • edited Loading

alalazo commented Jan 27, 2022

Choose a reason for hiding this comment

haampie Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgamblin Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

becker33 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalazo commented Jan 31, 2022

alalazo commented Jan 27, 2022 •

edited

Loading

haampie commented Jan 27, 2022 •

edited

Loading

haampie Jan 27, 2022 •

edited

Loading

tgamblin Jan 28, 2022 •

edited

Loading