Conditional variants by becker33 · Pull Request #24858 · spack/spack

becker33 · 2021-07-13T16:40:11Z

A common question from users has been how to model variants that are new in new versions of a package, or variants that are dependent on other variants. Our stock answer so far has been an unsatisfying combination of "just have it do nothing in the old versoin" and "tell Spack it conflicts".

This PR enables conditional variants, on any spec condition. The syntax is straightforward, and matches that of previous features.

variant('version_based', default=False, when='@2.0:', description="Variant that is only available in versions 2.0 and later")
variant('variant_based', default=False, when='+version_based', description="Variant that depends on another variant")

PR includes tests.

Closes #9740
This is a prereq to addressing #14337

adamjstewart · 2021-07-13T16:42:59Z

Can't wait to test this on packages like boost and PyTorch! With the old concretizer, I have to disable half of the variants in PyTorch because they don't exist.

tgamblin · 2021-07-14T17:27:26Z

@becker33: does this work well at all with the old concretizer? I love this feature but had been thinking it would really suck without a proper solver. I guess we don't have to worry too much longer, as 0.17 will make the new one default.

becker33 · 2021-07-14T19:40:12Z

@tgamblin it seems to work reasonably well with the old concretizer -- it's at least passing all the tests there. I do think it increases the capability to express relationships that the old concretizer can't solve, but it won't break existing packages/variants until someone updates to use it, and at that point I think it's better to model reality well than conform to the old concretizer well, given that the new one will be default soon.

adamjstewart · 2021-07-15T03:37:33Z

Tested this with py-torch with the following modifications:

Click to expand!

diff --git a/var/spack/repos/builtin/packages/py-torch/package.py b/var/spack/repos/builtin/packages/py-torch/package.py
index 63c7cac7cc..dc002ae210 100644
--- a/var/spack/repos/builtin/packages/py-torch/package.py
+++ b/var/spack/repos/builtin/packages/py-torch/package.py
@@ -49,51 +49,36 @@ class PyTorch(PythonPackage, CudaPackage):
 
     # All options are defined in CMakeLists.txt.
     # Some are listed in setup.py, but not all.
-    variant('caffe2', default=True, description='Build Caffe2')
+    variant('caffe2', default=True, when='@1.7:', description='Build Caffe2')
     variant('cuda', default=not is_darwin, description='Use CUDA')
-    variant('rocm', default=False, description='Use ROCm')
-    variant('cudnn', default=not is_darwin, description='Use cuDNN')
-    variant('fbgemm', default=True, description='Use FBGEMM (quantized 8-bit server operators)')
-    variant('kineto', default=True, description='Use Kineto profiling library')
-    variant('magma', default=not is_darwin, description='Use MAGMA')
+    variant('rocm', default=False, when='@1.0:', description='Use ROCm')
+    variant('cudnn', default=not is_darwin, when='@1.0: +cuda', description='Use cuDNN')
+    variant('fbgemm', default=True, when='@1.0:', description='Use FBGEMM (quantized 8-bit server operators)')
+    variant('kineto', default=True, when='@1.8:', description='Use Kineto profiling library')
+    variant('magma', default=not is_darwin, when='+cuda', description='Use MAGMA')
     variant('metal', default=is_darwin, description='Use Metal for Caffe2 iOS build')
-    variant('nccl', default=not is_darwin, description='Use NCCL')
+    variant('nccl', default=not is_darwin, when='+cuda platform=linux', description='Use NCCL')
+    variant('nccl', default=not is_darwin, when='+cuda platform=cray', description='Use NCCL')
+    variant('nccl', default=not is_darwin, when='+rocm platform=linux', description='Use NCCL')
+    variant('nccl', default=not is_darwin, when='+rocm platform=cray', description='Use NCCL')
     variant('nnpack', default=True, description='Use NNPACK')
-    variant('numa', default=not is_darwin, description='Use NUMA')
+    variant('numa', default=not is_darwin, when='platform=linux', description='Use NUMA')
+    variant('numa', default=not is_darwin, when='platform=cray', description='Use NUMA')
     variant('numpy', default=True, description='Use NumPy')
     variant('openmp', default=True, description='Use OpenMP for parallel code')
-    variant('qnnpack', default=True, description='Use QNNPACK (quantized 8-bit operators)')
-    variant('valgrind', default=not is_darwin, description='Use Valgrind')
-    variant('xnnpack', default=True, description='Use XNNPACK')
-    variant('mkldnn', default=True, description='Use MKLDNN')
+    variant('qnnpack', default=True, when='@1.0:', description='Use QNNPACK (quantized 8-bit operators)')
+    variant('valgrind', default=not is_darwin, when='@1.8: platform=linux', description='Use Valgrind')
+    variant('valgrind', default=not is_darwin, when='@1.8: platform=cray', description='Use Valgrind')
+    variant('xnnpack', default=True, when='@1.5:', description='Use XNNPACK')
+    variant('mkldnn', default=True, when='@1.0:', description='Use MKLDNN')
     variant('distributed', default=not is_darwin, description='Use distributed')
-    variant('mpi', default=not is_darwin, description='Use MPI for Caffe2')
-    variant('gloo', default=not is_darwin, description='Use Gloo')
-    variant('tensorpipe', default=not is_darwin, description='Use TensorPipe')
-    variant('onnx_ml', default=True, description='Enable traditional ONNX ML API')
-
-    conflicts('+cuda', when='+rocm')
-    conflicts('+cudnn', when='~cuda')
-    conflicts('+magma', when='~cuda')
-    conflicts('+nccl', when='~cuda~rocm')
-    conflicts('+nccl', when='platform=darwin')
-    conflicts('+numa', when='platform=darwin', msg='Only available on Linux')
-    conflicts('+valgrind', when='platform=darwin', msg='Only available on Linux')
-    conflicts('+mpi', when='~distributed')
-    conflicts('+gloo', when='~distributed')
-    conflicts('+tensorpipe', when='~distributed')
-    conflicts('+kineto', when='@:1.7.999')
-    conflicts('+valgrind', when='@:1.7.999')
-    conflicts('~caffe2', when='@0.4.0:1.6.999')  # no way to disable caffe2?
-    conflicts('+caffe2', when='@:0.3.1')  # caffe2 did not yet exist?
-    conflicts('+tensorpipe', when='@:1.5.999')
-    conflicts('+xnnpack', when='@:1.4.999')
-    conflicts('~onnx_ml', when='@:1.4.999')  # no way to disable ONNX?
-    conflicts('+rocm', when='@:0.4.999')
-    conflicts('+cudnn', when='@:0.4.999')
-    conflicts('+fbgemm', when='@:0.4.999,1.4.0')
-    conflicts('+qnnpack', when='@:0.4.999')
-    conflicts('+mkldnn', when='@:0.4.999')
+    variant('mpi', default=not is_darwin, when='+mpi', description='Use MPI for Caffe2')
+    variant('gloo', default=not is_darwin, when='+mpi', description='Use Gloo')
+    variant('tensorpipe', default=not is_darwin, when='@1.6: +mpi', description='Use TensorPipe')
+    variant('onnx_ml', default=True, when='@1.5:', description='Enable traditional ONNX ML API')
+
+    conflicts('+cuda+rocm')
+    conflicts('+fbgemm', when='@1.4.0')
 
     conflicts('cuda_arch=none', when='+cuda',
               msg='Must specify CUDA compute capabilities of your GPU, see '

Here is what I observed:

This works perfectly with both the original and clingo concretizers!

With depends_on, I can simulate "or" by using multiple lines. However, the following doesn't seem to work:

 variant('nccl', default=not is_darwin, when='+cuda platform=linux', description='Use NCCL')
 variant('nccl', default=not is_darwin, when='+cuda platform=cray', description='Use NCCL')
 variant('nccl', default=not is_darwin, when='+rocm platform=linux', description='Use NCCL')
 variant('nccl', default=not is_darwin, when='+rocm platform=cray', description='Use NCCL')

Another common feature request is variant defaults that change based on other variants. I don't think this is possible with the current implementation, right? I'm not sure what this would even look like...

Another idea that would be interesting. Right now there are a lot of packages with something like:

if self.spec.satisfies('@1.2:'):
    if '+foo' in self.spec:
        args.append('--enable-foo')
    else:
        args.append('--disable-foo')

GDAL is a good example of this. I'm wondering if the following would also work if the +foo variant only exists in 1.2+:

if '+foo' in self.spec:
    args.append('--enable-foo')
elif '~foo' in self.spec:
    args.append('--disable-foo')

becker33 · 2021-07-15T17:06:04Z

@adamjstewart thanks for checking this out!

This PR extends the when syntax to allow a list of conditions (for or). If you use that syntax, you can get or to work properly instead of using multiple lines.

variant('nccl', default=not is_darwin, when='+cuda platform=linux', description='Use NCCL')
variant('nccl', default=not is_darwin, when='+cuda platform=cray', description='Use NCCL')
variant('nccl', default=not is_darwin, when='+rocm platform=linux', description='Use NCCL')
variant('nccl', default=not is_darwin, when='+rocm platform=cray', description='Use NCCL')

could become

variant('nccl', default=not is_darwin, when=['+cuda platform=linux', '+cuda platform=cray', '+rocm platform=linux', '+rocm platform=cray'], description='Use NCCL')

The reason I did that is because separate defaults per-declaration cannot work without some really substantial work, and I don't want people to try

variant('foo', default=False, when='@1.1')
variant('foo', default=True, when='@1.2:')

However, as long as only the when-clause is changing, I can probably make that work in the interest of not confusing people who are used to the old syntax. (The new syntax does work for all the other commands that accept when clauses though, so it should catch on well enough for "or"). I'll take a look at making the duplicate variant declarations work for or.

adamjstewart · 2021-07-15T17:10:34Z

This PR extends the when syntax to allow a list of conditions

Does this work for all directives, not just variant? That's really cool!

becker33 · 2021-07-15T17:48:56Z

Yep, it works for all directives! I don't have tests in for all of them, but I have tests in for depends_on and all of them except for variant use the same code to handle it.

Also, multiply defined variants work now as long as you don't try to change the defaults/values/validators.

alalazo

Generally this looks great. I tried modifying cp2k and I can make all the CUDA related variants disappear if ~cuda is in the spec. This should give a sensible improvement in readability for complex specs!

The only major thing: I have been a bit puzzled by the extension of the when= argument to accept a list of specs. My first impression is that it is not obvious that this would be an OR and it is effectively syntactic sugar for having two directives with different when= arguments. It also deviates a bit from the declarative nature of directives. I guess I have two questions:

Is this feature technically necessary to get conditional variants?
Should we at least check what would be user reactions to it (mine and @adamjstewart seem to be opposite)?

Also, this slight modification:

diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index 45d9fd52d3..9b21e99bb3 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -252,7 +252,7 @@ def _wrapper(*args, **kwargs):
                 if DirectiveMeta._when_constraints_from_context:
                     # Check that directives not yet supporting the when= argument
                     # are not used inside the context manager
-                    if decorated_function.__name__ in ('version', 'variant'):
+                    if decorated_function.__name__ in ('version',):
                         msg = ('directive "{0}" cannot be used within a "when"'
                                ' context since it does not support a "when=" '
                                'argument')

is needed to make this feature work with the when context manager.

alalazo · 2021-07-27T14:34:54Z

lib/spack/spack/build_systems/autotools.py

+        variant_desc, _ = self.variants[name]
+        if set(variant_desc.values) == set((True, False)):


If I am reading this correctly here we are treating the absence of a variant as if it was set to False and so we'll emit e.g. a --disable-foo if self.enable_or_disabled('foo') is called. Just wanted to double check this is the semantics we want, rather than just disregarding it.

I think in this case it would throw an exception on line 443, because it unconditionally gets the variant from a dictionary.

alalazo · 2021-07-27T14:48:20Z

lib/spack/spack/cmd/info.py

            max(cols, 70) - max_name - max_vals - 14,
        )
-        self.column_widths = (max_name, max_vals, max_description)
+        self.column_widths = (max_name, max_when, max_vals, max_description)


This is a minor point, but we should probably revisit formatting. Right now I have this on my screen:

which is ok but may be improved in terms of:

Wrapping the Description line

Joining the when clauses for variants and avoid [] when it is always present

It would be nice to not display the when column at all if it isn't used by a package.

The output looks better to me, thanks! There are still a few super-minor issues - for instance this is from spack info gromacs:

I think though that we shouldn't hold this PR on that and we may fix minor issues like this in v0.17.1

alalazo · 2021-07-27T14:50:01Z

lib/spack/spack/directives.py

 _patch_order_index = 0


+def make_when_specs(value):


The function name is very similar to make_when_spec so maybe we should change it. Also it would probably be good to prepend both names with an underscore to make it clear they are used only in this module.

lib/spack/spack/solver/asp.py

lib/spack/spack/solver/concretize.lp

This is in addition to allowing multiple whens per-definition Multiply defined variants must have the same default/values/validators

lib/spack/spack/directives.py

lib/spack/spack/variant.py

alalazo · 2021-11-03T06:49:25Z

The failure on Fedora is happening on many PRs. It is unrelated to changes and possibly due to synchronization issues with upstream when new Fedora releases are pushed.

This broke in spack#24858

This broke in #24858

adamjstewart · 2022-01-04T18:07:55Z

Did we ever add support for when=[...]? It seems like that feature was removed from this PR.

This broke in spack#24858

spackbot-app bot added build-systems commands directives new-variant new-version tests General test capability(ies) labels Jul 13, 2021

becker33 force-pushed the features/conditional-variants branch 4 times, most recently from aeecaf2 to 1724798 Compare July 13, 2021 22:26

becker33 force-pushed the features/conditional-variants branch 4 times, most recently from 87652d4 to 2ff0898 Compare July 14, 2021 21:56

spackbot-app bot added the dependencies label Jul 14, 2021

becker33 force-pushed the features/conditional-variants branch 3 times, most recently from e40c5e7 to 421f3dc Compare July 16, 2021 21:39

tldahlgren requested review from adamjstewart and tgamblin July 21, 2021 23:40

tldahlgren assigned tgamblin and adamjstewart Jul 21, 2021

alalazo requested changes Jul 27, 2021

View reviewed changes

becker33 added 12 commits November 2, 2021 10:56

add tests for conditional variants

f91360f

remove unreachable conditional

453e55d

refactor for more pythonic representation

8c60b7d

make conditional variants fail when specified explicitly and ~condition

689544b

Allow multiply defined variants for multiple when clauses

4b0f476

This is in addition to allowing multiple whens per-definition Multiply defined variants must have the same default/values/validators

concretizer: add constraint no variant value for variant not on package

560f051

reorder variant_condition args to match other condition types

e9b798c

spack info: improve conditional variant format

71e3363

fix variant directive docstring

46798f4

fixup

5758717

valid hashes for mock package

eee2aeb

add error messages for new clingo failure modes

a589faa

becker33 force-pushed the features/conditional-variants branch from e04e94b to a589faa Compare November 2, 2021 18:00

allow variant directive in when context manager

3263ed6

alalazo reviewed Nov 2, 2021

View reviewed changes

lib/spack/spack/directives.py Outdated Show resolved Hide resolved

alalazo requested changes Nov 2, 2021

View reviewed changes

lib/spack/spack/variant.py Outdated Show resolved Hide resolved

address review bug

ca5eb36

spackbot-app bot added the documentation Improvements or additions to documentation label Nov 2, 2021

alalazo approved these changes Nov 3, 2021

View reviewed changes

alalazo merged commit 67cd92e into develop Nov 3, 2021

alalazo deleted the features/conditional-variants branch November 3, 2021 07:11

alalazo added a commit to alalazo/spack that referenced this pull request Nov 30, 2021

spack audit: fix API calls

813bb53

This broke in spack#24858

alalazo mentioned this pull request Nov 30, 2021

spack audit: fix API calls to variants #27713

Merged

alalazo added a commit that referenced this pull request Nov 30, 2021

spack audit: fix API calls (#27713)

645a7dc

This broke in #24858

haampie pushed a commit that referenced this pull request Nov 30, 2021

spack audit: fix API calls (#27713)

f9a0daa

This broke in #24858

alalazo added a commit that referenced this pull request Dec 23, 2021

spack audit: fix API calls (#27713)

e9f7fb0

This broke in #24858

alalazo mentioned this pull request Mar 16, 2022

Allow conditional possible values in variants #29530

Merged

3 tasks

capitalaslash pushed a commit to capitalaslash/spack that referenced this pull request Aug 30, 2022

spack audit: fix API calls (spack#27713)

811d34d

This broke in spack#24858

		variant_desc, _ = self.variants[name]
		if set(variant_desc.values) == set((True, False)):

Conversation

becker33 commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Jul 13, 2021

Uh oh!

tgamblin commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

becker33 commented Jul 14, 2021

Uh oh!

adamjstewart commented Jul 15, 2021

Uh oh!

becker33 commented Jul 15, 2021

Uh oh!

adamjstewart commented Jul 15, 2021

Uh oh!

becker33 commented Jul 15, 2021

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

becker33 Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

alalazo Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

adamjstewart Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

alalazo Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

alalazo Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alalazo commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Jan 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

becker33 commented Jul 13, 2021 •

edited

Loading

tgamblin commented Jul 14, 2021 •

edited

Loading

alalazo commented Nov 3, 2021 •

edited

Loading