Awesome feature! Needs documentation in the PR...

@citibeth Added a box to check in the description above.

@tgamblin @becker33 @davydden @adamjstewart and anybody else, I think this PR is ready for review.

I would proceed like that: if this is considered to be a viable addition to the core, I'll address the reviews and after that is done I'll write Sphinx docs before merging. Sounds fine?

I would like to learn more. But from what I've seen, I would suggest not merging this PR at this time. The core question here is, what functionality is added by the proposed feature that could not be done without it? What I've seen reviewing the code is the ability to replace specs like package+a+b+c with package+enable=a,b,c. Except for minor differences, the two are semantically equivalent if exclusive=False. Therefore, I don't think that exclusive=False is a feature we would want to include.

What about exclusive=True? That can also be handled currently by adding code that checks that at most one of a bunch of variants is set. With a couple of subroutines made available to package authors, this could be implemented easily, something like:
check_at_most_one_set('a', 'b', 'c')
(If done right, this checking could be available at Spec creation time, not just install time). Adding such check subroutines would be far less disruptive than the changes suggested in this PR.

More complicated cases --- for example, one of 'a' and 'b' can be set; and 'c' can be set only 'a' is set --- will need an explicit checker subroutine anyway.

Once you introduce two ways to do something, different users will choose one way or the other in an inconsistent fashion. That will make it harder for Spack users to write every install command line. "Is this a package where I say package+a or package+enable=a?"

I see that the Autotools package is able to now automatically generate --with and --without flags. Clever... but again, I'm not sure that's the direction we want to be going. It does shorten the length of the package.py file. But it also increases the amount of knowledge necessary to write a package. The move from Package to AutotoolsPackage / CMakePackage did not just make package files shorter, it enabled functionality that was not available before (spack setup). In this case, I don't think that saving package authors from writing --with or --without justifies this change, and it makes the package files more cryptic. Again, please create a helper function to do this explicitly, if that is what one wishes to do.

Where I had HOPED this PR was going --- but maybe I was wrong --- is allowing variants with values other than bool (and clearly other than enum). For example, mytextprocessor+maxlinelen=100. int or even double-valued variants would add capabilities that are not currently possible. For this reason, please write at least a first draft of the docs before seeking a final merge. Without that, it's hard to really tell what's being merged, or what is intended. Too many times I've said "yea yea" to a PR, and then realized it held "surprises" only after I saw the docs.

Finally...

• This PR has numerous trivial formatting changes that are unrelated to the content of the PR. They may be good changes; but when applied in this manner, it just contributes to churn and make it harder to evaluate this PR. It would be great if gratuitous change can be removed from this PR.

Adding such check subroutines would be far less disruptive than the changes suggested in this PR.

There's nothing disruptive here. In fact one of the design principle I followed was to keep everything backward compatible. Every package works as it worked before. The only differences are that now you can group variants semantically (fabrics, schedulers) and easily enforce check at concretization time, not install time. The additional functionalities (exclusive, validator) are there for you if you need to use them. I won't enforce their use on packages that don't need it.

For me a prove this is a worth change are the 85 lines deleted in mvapich2 or the 50 lines that we can potentially delete in cdo. Repetitive code has been made general and incorporated in the framework. You may not like it, but that's the change done here. And it's not disruptive

In this case, I don't think that saving package authors from writing --with or --without justifies this change, and it makes the package files more cryptic. Again, please create a helper function to do this explicitly, if that is what one wishes to do.

Except that you completely miss the point of reponsibility in the right place: that feature makes sense only if you have to do with an autotools package, and thus the code is in the AutotoolsPackage class and available to you only if you derive from it.

Where I had HOPED this PR was going --- but maybe I was wrong --- is allowing variants with values other than bool (and clearly other than enum). For example, mytextprocessor+maxlinelen=100. int or even double-valued variants would add capabilities that are not currently possible.

I wrote an extensive description for this PR, that you probably didn't care reading. I never stated anywhere I was going to provide custom types in variants.

The move from Package to AutotoolsPackage / CMakePackage did not just make package files shorter, it enabled functionality that was not available before (spack setup)

and

For this reason, please write at least a first draft of the docs before seeking a final merge. Without that, it's hard to really tell what's being merged, or what is intended. Too many times I've said "yea yea" to a PR, and then realized it held "surprises" only after I saw the docs

Not True. I introduced build_systems in #1182 and that was not the purpose. The purpose was to aggregate in a base class all the methods that were specific to a given build system and give package authors hooks to run things before and after certain phases. In general, I wanted to extract general code and make it available to packagers. This is also the purpose of this PR.

spack setup has been introduced in #543 and it was broken (see #1242), not tested but well documented. This is not the way I like to work. I prefer to write tests, agree on the code and when the interface is mostly stable, document it. I think we went over this argument at least four or five times. I won't change my mind on this.

This PR has numerous trivial formatting changes that are unrelated to the content of the PR. They may be good changes; but when applied in this manner, it just contributes to churn and make it harder to evaluate this PR. It would be great if gratuitous change can be removed from this PR.

This is a specious argument. Apart from a couple that could probably be reverted now (but why as they are not controversial?) all the others are to please flake8.

@alalazo I am truly sorry I didn't see the description at the top of the PR. I'm not sure why... I suppose I just didn't look for it in my haste. I did look for changes to .rst files, and did not find any. Again, my apologies.

After having read those docs, I still have questions.

Every package works as it worked before.

To be specific, I'm looking at openmpi/package.py. Supposing I used to say spack install openmpi+mxm+pmi. What command line would I now use? My first guess was it would be something like spack install openmpi+fabrics=mxm,pmi. But I don't know... please tell me what the user actually has to type, and how that is converted under the hood to a value for the variant fabrics. And what does the user see when they ask for help on this package?

The only differences are that now you can group variants semantically (fabrics, schedulers) and easily enforce check at concretization time, not install time. The additional functionalities (exclusive, validator) are there for you if you need to use them. I won't enforce their use on packages that don't need it.

Maybe we need to consider changing terminology. To me, a "variant" is something that comes after a + or - sign in a spec; and currently, all variants are of type bool. If this PR doesn't change the specs user must type, then I would think you are not so much creating multi-valued variants, as you are creating explicit variant groups that can be checked at spec time. If that's the case, maybe we should call them "variant groups", to not confuse with variants, and maybe the code to declare them should look something like:

+    variant_group(
+        'fabrics',
+        default='' if _verbs_dir() is None else 'verbs',
+        description='List of fabrics that are enabled',
+        variants=('psm', 'psm2', 'pmi', 'verbs', 'mxm'),
+        exclusive=False
+    )

For me a prove this is a worth change are the 85 lines deleted in mvapich2 or the 50 lines that we can potentially delete in cdo. Repetitive code has been made general and incorporated in the framework.

I still think we need to consider things carefully before folding more stuff into the framework. Because it makes it harder for a naive user to look at a recipe and have any clue what it does. This PR uses the word lambda in at least one package, which will almost certainly scare off some would-be Spackers. Any others have thoughts on this? @tgamblin @adamjstewart @davydden

Apart from a couple that could probably be reverted now (but why as they are not controversial?) all the others are to please flake8.

Clearly, anything to please flake8 needs to be made. But I see a lot I think are not. Reasons not to include them:

1. If two people have different views on how something should be formatted (and they are both PEP8-compliant), then there can be a "bouncing" back and forth, depending on who last edited the file. Your changes are not controversial. But they could too easily be undone (also gratuitously) by the next person editing these files.

2. If you fix something in one file that you happened to run across, but not 20 other files that need it, then the fix does nothing to improve the consistency of the code. When people want to do something, they copy an existing example. If it's done 2 ways in the code, how a new user does it will depend, randomly, on which example they copied.

3. It makes the PR longer and harder to read.

Where I had HOPED this PR was going --- but maybe I was wrong --- is allowing variants with values other than bool (and clearly other than enum). For example, mytextprocessor+maxlinelen=100. int or even double-valued variants would add capabilities that are not currently possible.

To me, a "variant" is something that comes after a + or - sign in a spec; and currently, all variants are of type bool.

Just to be clear, we've had non-boolean variants for a long time. Check out the NetCDF package for an example. I believe the syntax for calling variants like this is something like netcdf+mpi maxdims=1024. There's no + or - since it's a value, not a boolean. You can't really deactivate a number.

P.S. This PR looks awesome. I agree that we shouldn't convert all "provide support for XYZ" into a single enabled=(ABC, DEF, GHI) variant, but things like fabrics or gui_backend make perfect sense. It's really annoying to have to code in checks to make sure users don't specify mutually exclusive variants. I also like adding --with-{0}/--without-{0} methods as those end up getting used a lot.

I noticed you added the hash-change label. Does this PR change the hashes for every package, or only the ones that you modified?

@adamjstewart

I noticed you added the hash-change label. Does this PR change the hashes for every package, or only the ones that you modified?

As far as I can see only the ones I modified. But as I changed things that are used to evaluate the hash I added the tag to remain on the safe side.

This PR looks awesome. I agree that we shouldn't convert all "provide support for XYZ" into a single enabled=(ABC, DEF, GHI) variant, but things like fabrics or gui_backend make perfect sense.

Yes, the changes on cdo were definitely a bad choice from my side to show an example. They have been reverted (well, mostly because it seems pycharm has its own idea of formatting when you say go back to version in branch X)

To be specific, I'm looking at openmpi/package.py. Supposing I used to say spack install openmpi+mxm+pmi. What command line would I now use?

The command line would be:

spack install openmpi fabrics=\'mxm,pmi\'

This syntax is there since a long time. As far as I can tell it was introduced in #360 by @becker33.

Another orthogonal PR that would be worth trying with respect to user facing syntax is a modification of the parser to avoid the \' from the command line.

This PR uses the word lambda in at least one package, which will almost certainly scare off some would-be Spackers.

For this particular case: lambda has nothing to do with the changes in the core. It was in the cdo package and was used where a callable was expected. One could have coded a function, or any other type of callable. To me it came natural to use a lambda in that context.

I also don't agree with the general argument. lambda are part of the python core language. They don't need to be used, but they can be used. What I surely don't want is to restrict people on the set of python features they can use if they are coding a package.

The command line would be:

spack install openmpi fabrics=\'mxm,pmi\'

To me, this is a non-starter: users won't know whether to type spack install openmpi+mxm+pmi or spack install fabrics=mxm,pmi. The two don't really expose different semantics to the use user, they are just different variations of (essentially the same) semantics, which the user is forced to distinguish between. The word fabrics is not really relevant to the user, and is just one more thing the user must remember to get right. This is not just an objection to change in the UI; it's an objection to adding irrelevant (to the user) details that must be known and remembered to make it work.

We need to acknowledge that this PR is a change. It doesn't just add a feature, but also changes the way openmpi and other packages work, from a user's point of view. And it doesn't change the other 998+ packages in Spack. Which increases the degree of inconsistency in the system as a whole.

It may be true that syntax of variant=True has been around for a long time. But I have seen nothing about it in docs. Is this simply an alternate syntax for saying +variant? If so, then that is a different beast from changing the required syntax a user must use.

In any case, all my objections can be addressed if the PR is extended so the UI does not change. As long as the user can say openmpi+mxm+pmi and the right thing happens, then I would support this PR. Users should not have to remember the word fabrics, or the fact that mxm and pmi are part of fabrics. When they dig down into the help or the package source code, they can then discover that the variatns mxm and pmi are actually both part of the fabrics variant group; and at most one of them can be selected (or whatever the restrictions are for this variant group).

Please keep the UI the same, avoid adding unnecessary complication to the UI. openmpi+mxm+pmi should work, whether mxm and pmi are separate variants, or they are part of the variant group (multi-valued variant) called fabrics

I won't do this change here: the syntax changes because instead of having 2 boolean variants you have a multi-valued one in openmpi. Doing what you ask goes against the very nature of this PR. As usual the maintainer are free to decline this, but I notice that it is becoming an habit for you to start saying something is awesome and in the next comment require to undo everything.

i personally don't mind the change in UI. Spack is in alpha, new features sometimes break backward compatibility

Just to be clear: there's no "change in the UI". There are two packages that have been reworked to have different variants. If this is considered to be a "change in the UI", then so is #2323 and any other PR that changes the variants of at least a package in some way.

Just to be clear: there's no "change in the UI".

yes, i think should not qualify as "change in the UI" as it is indeed just rework of variants in a few packages. I used this term just because I quoted the text which used it.

@tgamblin @becker33

I believe there is some misunderstanding why I don't think this PR is right for Spack (in its current form). It's not that it changes things; but that it introduces more than one way to do the same thing, and will introduce random inconsistencies into our package repos.

Suppose I'm a packager, and I have a bunch of on-off switches for my package. I could choose to make each of them separate variant, or to group some of them into multi-valued variants. For example, I could decide to make libtype a multi-valued variant, and require mypackage libtype=shared,static. This might make sense to me because my package can't build shared and static together. Another packager could decide the other way, requiring theirpackage+shared+static; and it might make sense to them because they're traditional, or because shared and static can be built together. Once this process is underway, users will no longer be assured that apackage+static or apackage+shared is the right way to build a static or shared library; because some packages will work one way and some another way. They will have to ask spack info each time. And this scenario will be repeated for every set of optional features a package typically has: shared vs. static, language bindings, some dependency groups, etc.

Then some packager might (in an extreme case) decide to just put all variants into a multi-valued variant called variants; thus, instead of extremepackage+a+b+c, the user would have to type extremepackage variants=a,b,c. Sure, maybe you say such a PR wouldn't be accepted. But in order to not accept it, we will have to argue as a community and figure out when things should be separate variants vs. multi-valued variants. After a lot of discussion, we will probably end up with some conventions on this issue. But how have we gained from the previous state-of-the-art, when users always knew to say +a if they wanted feature a?

A multi-valued variant with exclusive=False is really no different from a bunch of "regular old boolean" variants. A multi-valued variant with exclusive=True is no different from "regular old boolean" variants with the idea that only one can be selected at once. The interlocking feature is good. But users are used to thinking about "variant a and c cannot be selected together." Why do they need to know about multi-valued variants in order to talk about which variants can / cannot be selected together? This adds conceptual complexity to the UI without any obvious gain.

It is this kind of confusion that I'm trying to avoid. Similar to the idea that package names should be all lower-case: it just make things easier for the user, they never have to ask "is this package upper or lower case?" (BTW, we still need to convert a few packages to lower case. Obviously, we would have been better off if Spack had enforced this from the beginning and barfed on non-lowercase package names).

This PR adds some nice features under the hood. For example:

1. The ability to better describe which subsets of variants are / are not allowed.

2. The tie-ins with AutotoolsPackage. (I'm not sold on it, but I like the idea of it in spite of not being sold).

I have suggested the concept of "variant groups" in order to keep the UI simple and predictable, while maintaining the under-the-hood improvements in this PR. My suggestion requires no fundamental changes to this PR, only:

1. Code to map a bunch of variants in a spec to whatever multi-valued variants are used under the hood. Thus, openmpi+mxm+pmi would be transformed into openmpi fabrics=mxm,pmi. This would happen pretty early in the process.

2. (OPTIONAL): Rename "multi-valued variant" to "variant group."

This change would allow packagers to choose either separate or multi-valued variants based on their coding needs, while having no effect on the user.

Suppose I'm a packager, and I have a bunch of on-off switches for my package. I could choose to make each of them separate variant, or to group some of them into multi-valued variants. For example, I could decide to make libtype a multi-valued variant, and require mypackage libtype=shared,static. This might make sense to me because my package can't build shared and static together. Another packager could decide the other way, requiring theirpackage+shared+static; and it might make sense to them because they're traditional, or because shared and static can be built together. Once this process is underway, users will no longer be assured that apackage+static or apackage+shared is the right way to build a static or shared library; because some packages will work one way and some another way. They will have to ask spack info each time. And this scenario will be repeated for every set of optional features a package typically has: shared vs. static, language bindings, some dependency groups, etc.

I kind of agree with this. There will be no way to enforce this, and it will end up differing inconsistently. Although to be fair, the only way to know what variants are available in the first place is to view the package.py or to run spack info, in either case it should be obvious what syntax to use.

I do agree that some kind of multi-valued variant/variant group is necessary though. Ensuring that mutually exclusive variants aren't combined is a pain in the ass right now. And things like fabrics and schedulers are logically related. It would be good to show the relation when running spack info. Btw, in it's current state, what does spack info openmpi print out?

@alalazo How hard would it be to create variant groups such that the "in the package.py" stuff looks like what you have now, but you can call it from the command-line like you used to (or call it both ways, package+a+b+c and package variants=a,b,c)?

The command line would be:

spack install openmpi fabrics=\'mxm,pmi\'

Is there any way we can get fabrics=mxm,pmi or fabrics='mxm,pmi' to work? Unrelated to this PR, but being able to do the same with cflags and friends would be awesome. I know we need quotes for cflags to allow spaces in-between flags, but it should already be grouped into a single variant by the time it gets to Python code without the backslash, right? For example, take the following minimalist sample:

import argparse

parser = argparse.ArgumentParser()
parser.add_argument('cflags')

args = parser.parse_args()

print(args.cflags.split(' '))

and then:

$ ./test.py cflags="-O2 -rpath"
['cflags=-O2', '-rpath']

does the reason have something to do with the concretizer? Couldn't we just add quotes to the string internally? Or are we just blindly passing everything after spack install to the concretizer without trying to parse it? I've seen other users confused about when to use backslash quotes (command line) and regular quotes (in the package.py).

@tgamblin I changed the doctrings only in spack/variants.py. In the other files they are consistent but wrong (meaning only reST). If you agree I would leave that refactoring for later, separate PRs. Like the ones on unit tests.

Removing hash-change label since this doesn't actually change existing specs. Old variants are still encoded the same way, aside from the obvious updates to mvapich2 and openmpi packages.

@adamjstewart: can you take this PR and try running spack find on your installed DB? I want to make sure it can read existing specs. I am reasonably confident looking at the PR, but it would be good to be sure.

Also, can you try running:

spack spec <something that depends on openmpi or mvapich2> ^/abc123

where abc123 is the hash of some old openmpi or mvapich2 installation?

@alalazo has shown that last one to break, but I think it's an existing bug that is made more prominent by this change, not a bug with this PR. Essentially, normalize() runs erroneously on the already-concrete /abc123 spec. Looks like this:

  File "/home/mculpo/PycharmProjects/spack/bin/spack", line 183, in _main
    return_val = command(parser, args)
  File "/home/mculpo/PycharmProjects/spack/lib/spack/spack/cmd/spec.py", line 84, in spec
    spec.normalize()
  File "/home/mculpo/PycharmProjects/spack/lib/spack/spack/spec.py", line 2007, in normalize
    self.validate_or_raise()
  File "/home/mculpo/PycharmProjects/spack/lib/spack/spack/spec.py", line 2065, in validate_or_raise
    raise UnknownVariantError(spec.name, not_existing)

Anyway, expect that bug, but I'm interested in whether you can also find new ones.

Our cluster is undergoing maintenance at the moment, so when it's back up I'll test this.

Alright, we're back up.

try running spack find on your installed DB

From a long-standing Spack installation, on develop, spack find finds 832 installed packages (including 7 installations of the same version of libxml2 with the same compiler?? *cough* changing hashes *cough*). When I switch to features/multi_valued_variants, spack find still finds all 832 installed packages.

Also, can you try running:

spack spec <something that depends on openmpi or mvapich2> ^/abc123

where abc123 is the hash of some old openmpi or mvapich2 installation?

$ spack spec -I hdf5+mpi ^/m4ked5j
Input spec
--------------------------------
     hdf5+mpi
[+]      ^[email protected]%[email protected] cflags="-axCORE-AVX2,AVX" cxxflags="-axCORE-AVX2,AVX" fflags="-axCORE-AVX2,AVX" ~debug~gforker+hydra~mrail~nemesis~nemesisib~nemesisibtcp+psm~remshell~slurm~sock arch=linux-centos6-x86_64 
[+]          ^[email protected]%[email protected] cflags="-axCORE-AVX2,AVX" cxxflags="-axCORE-AVX2,AVX" fflags="-axCORE-AVX2,AVX"  arch=linux-centos6-x86_64 
[+]              ^[email protected]%[email protected] cflags="-axCORE-AVX2,AVX" cxxflags="-axCORE-AVX2,AVX" fflags="-axCORE-AVX2,AVX" +sigsegv arch=linux-centos6-x86_64 
[+]                  ^[email protected]%[email protected] cflags="-axCORE-AVX2,AVX" cxxflags="-axCORE-AVX2,AVX" fflags="-axCORE-AVX2,AVX"  arch=linux-centos6-x86_64 
[+]          ^[email protected]%[email protected] cflags="-axCORE-AVX2,AVX" cxxflags="-axCORE-AVX2,AVX" fflags="-axCORE-AVX2,AVX"  arch=linux-centos6-x86_64 

Normalized
--------------------------------
==> Error: Package mvapich2 has no variant set(['nemesis', 'nemesisib', 'psm', 'sock', 'slurm', 'remshell', 'mrail', 'hydra', 'nemesisibtcp', 'gforker'])!

I'm personally okay with this. I don't normally try to force installations to use a specific hash. For any PR (not just this one), if a variant is added or removed, Spack no longer recognizes the previously installed versions. This is annoying, but I agree that you really can't tell how it was built. Although personally I think if a user isn't explicitly asking for it to be built a certain way, we should still use it. But yeah, nothing new with this PR.

@adamjstewart: Thanks! I think we can merge this, and address the spec issue in a separate PR.

Ok, y'all. Multi-valued variants are (finally) merged. @alalazo: can you update the docs?