Skip to content

Command Line Defaults#2705

Closed
citibeth wants to merge 2 commits intospack:developfrom
citibeth:efischer/161231-CmdLineDefaults
Closed

Command Line Defaults#2705
citibeth wants to merge 2 commits intospack:developfrom
citibeth:efischer/161231-CmdLineDefaults

Conversation

@citibeth
Copy link
Copy Markdown
Member

This PR adds command line defaults to Spack. Default command line arguments can now be specified in a configuration file called aliases.yaml. For example:

aliases:
    all: --debug
    spec: --install-status --long
    install: --install-status
    setup: --default-version

Example: When the user types spack spec xz, that will be converted to spack --debug spec --install-status --long xz.

Question: What should the YAML file be called?

  • aliases.yaml (because it's kind of like Bash aliases)?
  • defaults.yaml (because it sets defaults for command lines)?
  • default-args.yaml (because that's more specific than defaults.yaml)?

@citibeth citibeth added the feature A feature is missing in Spack label Dec 31, 2016
@adamjstewart
Copy link
Copy Markdown
Member

I like aliases.yaml.

If I understand correctly, with this PR I'll be able to tell spack install that it should always be --verbose, right?

@citibeth
Copy link
Copy Markdown
Member Author

If I understand correctly, with this PR I'll be able to tell spack install that it should always be --verbose, right?

Exactly. Or for machines that require --dirty, an aliases.yaml file can be put in the appropriate arch-specific configuration adding the --dirty flag to spack install.

@citibeth
Copy link
Copy Markdown
Member Author

All the tests pass, except the doc test. I'm getting the following error from Travis:

Warning, treated as error:
WARNING: Unexpected return code 2 from command u'spack info mpich'

When I try it myself, I get this behavior too:

$ spack info mpich
usage: spack info [-h] PACKAGE
spack info: error: too few arguments

Looks like this PR still needs a little work...

@adamjstewart
Copy link
Copy Markdown
Member

Maybe you can't parse args multiple times? It seemed a little hacky anyway.

@adamjstewart
Copy link
Copy Markdown
Member

Also, I can't believe the documentation tests are the only thing that catches such a grave bug.

@citibeth
Copy link
Copy Markdown
Member Author

citibeth commented Jan 1, 2017

Maybe you can't parse args multiple times?

Good guess, but actually you can.

It seemed a little hacky anyway.

“Life is pain, highness. Anyone who says differently is selling something.”
― William Goldman

This PR is too good to throw away (IMHO), so I just re-did my argparse Jujitsu. The idea now is to find the position of the subcommand in the command line, and split up the original arguments around it. Then re-assemble the command line with the defaults added in. I verified that Spack works just fine with repeated flags, in case the user specified a flag that was already in the defaults.

I find the position of the subcommand by first parsing the command line with a parser that has just the args for main Spack, leaving everything from the Spack command on as unparsed. Then I use list lengths to split up the original args.

Also, I can't believe the documentation tests are the only thing that catches such a grave bug.

I'm hoping I don't have to find a solution to that problem in this PR.

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented Jan 8, 2017

I like the concept of simplifying the spack command line, but I have reservations about this for the same reason the git maintainers do. Specifically:

Currently git does not allow aliases to override builtins. I
understand the reasoning behind this, but I wonder if it's overly
conservative.

It is not.

Most shells support overriding commands with aliases, and I'm not sure
why git needs to be more conservative than the shell.

Because sane shells do not expand aliases when used in a script, and gives
a handy way to defeat the alias even from the command line.

$ alias ls='ls -aF' 
$ echo ls >script 
$ chmod +x script 

and compare:

$ ./script 
$ ls 
$ /bin/ls 

I tend to agree with the issues raised above that if you allow everything in this PR, then it's going to be very hard to write a script for Spack that works reproducibly for different users. I would like to keep Spack scriptable and to ensure a little bit that when we talk to a user on GitHub, that we're talking about the same semantics for a given command.

Also, the settings here overlap with some of those in config.yaml. For example:

Exactly. Or for machines that require --dirty, an aliases.yaml file can be put in the appropriate arch-specific configuration adding the --dirty flag to spack install.

Why not just set dirty: true in config.yaml? That exists. color: true may soon exist (see #2441). We have control over what defaults we allow to be set here (i.e., we can keep them sane), and I don't mind adding more settings in config.yaml.

That said, I would welcome a PR that allows real aliases, similar to git's. e.g.:

aliases:
    dinstall: install --dirty
    in: install
    rm: uninstall

The one caveat would be the same as the one git enforces: aliases cannot override builtin commands. That has the advantage, I think, of providing all the power you want from this PR, but not trampling the scripting capabilities. Your options would have to be implemented as separate commands, but that's a minor thing and I think it will serve to keep future conversations we have on github more sane.

The other cool thing about git aliases is that they can take arguments. I don't think that would actually be hard to implement in Spack, and people could likely implement even cooler things with that capability.

So basically I am saying I like this, but I think we should take a page from git and apply the same caution that works for them. I think the time spent improving this will be worth it.

Thoughts?

@citibeth
Copy link
Copy Markdown
Member Author

citibeth commented Jan 8, 2017

I tend to agree with the issues raised above that if you allow everything in this PR, then it's going to be very hard to write a script for Spack that works reproducibly for different users. I would like to keep Spack scriptable and to ensure a little bit that when we talk to a user on GitHub, that we're talking about the same semantics for a given command.

Let's look at the situation more holistically. Spack scripts are already non-reproducible between users, due to the large number of things that can ALREADY be in packages.yaml. Having spack install rely on what's already installed (a common request, and one I believe is "on the docket") would make Spack even more non-reproducible, even for a single user.

Luckily, it is possible to use Spack in reproducible way. With #2686 in place, I have completely emptied my ~/.spack/packages.yaml file, splitting it into 4 separate packages.yaml files in 4 scopes: (a) CentOS7-specific, (b) Project-specific, (c) Specific to certain variants I use in some builds of my project, (d) Sets appropriate versions to develop. I now specify the Spack configuration scopes I want to use on the command line, where it becomes explicit and reproducible. I have two Bash aliases I use for this:

# Use to install released software
alias rspack='\spack -c ~/spscopes/centos7 -c ~/spscopes/gissversions'
# Use to build software in development
alias dspack='\spack -c ~/spscopes/centos7 -c ~/spscopes/gissversions -c ~/spscopes/twoway -c ~/spscopes/develop'
# Disable raw Spack in case I forget
alias spack='nothing'

By moving configuration from the user-level scope to command line scopes, I have made things reproducible where they were not before.

I could do the same thing with my aliases.yaml file: put them in a scope that I specify on the command line, and then use Bash aliases to specify that scope. One could even enforce that stipulating that aliases.yaml must come from a scope specified on the command line; however, why should Spack enforce this for aliases.yaml but not packages.yaml --- both of which can cause severe reproducibility problems if not managed carefully.

Finally, let's look at what I'm actually using my aliases.yaml for (which I do have in ~/.spack/aliases.yaml):

aliases:
    # all: --debug
    spec: --install-status --long
    install: --install-status
    #setup: --default-version
    setup: --install-status
    info:

As you can see, none of the alias commands I've selected have any effect on reproducibility.

Conclusion: Reproducibility in Spack is a significant issue, and user can shoot themselves in the foot. This PR does not particularly increase or decrease that possibility. However, #2686 DOES provide a tool that allows users to work in a reproducible way, if they so choose.

Exactly. Or for machines that require --dirty, an aliases.yaml file can be put in the appropriate arch-specific configuration adding the --dirty flag to spack install.

Why not just set dirty: true in config.yaml? That exists. color: true may soon exist (see #2441). We have control over what defaults we allow to be set here (i.e., we can keep them sane), and I don't mind adding more settings in config.yaml.

I suppose that's another way to go about it, and would be appropriate for the aliases I wanted this PR for. However... this PR was quick, and in one fell swoop adds all possibility functionality --- without changing the code that uses those configuration options. Adding the parameters one by one to config.yaml would be a lot more work. And unless we have a systematic approach to allowing certain options to be set both on the command line or in config.yaml, the two will get out of sync again as somebody adds a new command line options.

That said, I would welcome a PR that allows real aliases, similar to git's. e.g.:

aliases:
    dinstall: install --dirty
    in: install
    rm: uninstall

That doesn't cover the use case I want this PR for. Now users have to type spack dinstall instead of spack install (more on that below).

Nor is your example convincing. Because typically --dirty is used on certain systems that need it (such as our supercomputer with its compiler built in a crazy brain-dead way). In that case, you need to always type spack install --dirty, not spack install. The dangers is people will forget, and they'll type spack install. Making them type spack dinstall instead of spack install saves a few keystrokes, but it doesn't really help that core problem.

That has the advantage, I think, of providing all the power you want from this PR,

It provides none of the power I want from this PR. If I'm willing to type non-standard Spack commands (eg spack dinstall instad of spack install), I might as well just do it in Bash:

alias spack-spec='spack spec --install-status --long'
alias spack-install='spack install --install-status'
alias spack-setup='spack setup --install-status'

This would be minorly inconvenient for me, but I might get used to it. However, it could be embarassing in other cases. Suppose I'm on a system using some crazy compiler that requires --dirty. So I do:

alias spack-diy='spack diy --dirty'
alias spack-install='spack install --dirty'

Now I sit my users down with a Spack tutorial and tell them to type spack-install every time they see spack install; but to not similarly modify the other 50 Spack commands they might type. You can see how well THAT will go over. No... if my system requires --dirty, the most effective thing to do will be to configure Spack that way in a System-wide scope.

but not trampling the scripting capabilities.

As long as you only put aliases.yaml (and packages.yaml too) in scopes specified on the command line, scripting capabilities will be unaffected.

The other cool thing about git aliases is that they can take arguments. I don't think that would actually be hard to implement in Spack, and people could likely implement even cooler things with that capability.

Again... not the functionality I'm looking for in this PR.

Thoughts?

The proposal to add appropriate options to config.yaml is the only one that provides the functionaly I needed out of this PR. Unless someone wants to do that work, I think this PR is the best we have for now, and it gets the job done.

@adamjstewart
Copy link
Copy Markdown
Member

I would definitely like the ability to set default flags for Spack subcommands. I almost always want:

$ spack install --verbose
$ spack spec --install-status
$ spack find --paths --variants

I don't particularly care whether this is done by adding an aliases.yaml or adding these flags to config.yaml. But I agree with @citibeth that config.yaml support may not be updated as quickly as new flags are added or old flags are removed/renamed. I don't see a major issue with reproducibility. Yes, Spack should be scriptable, and yes, if someone aliases spack install to spack install --only dependencies, then Spack is no longer scriptable. What if we add a flag to Spack like spack --no-aliases that ignores all user provided aliases? This makes it still scriptable.

@tgamblin
Copy link
Copy Markdown
Member

Do others have an opinion on this? @alalazo? @davydden? @mamelara @mplegendre? @becker33 ? @lee218llnl ? sounds like I might be in the minority.

@davydden
Copy link
Copy Markdown
Member

Personal minor concern, mostly OT: I would not grow the array of configuration files. If there is already config.yaml, maybe the functionality should be added through it. Otherwise in a year one may end up having tens of files and it would be a pain to remember which one actually influence what.

@citibeth
Copy link
Copy Markdown
Member Author

citibeth commented Jan 10, 2017 via email

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Jan 10, 2017

@tgamblin I tend to agree with your opinion above, in particular:

Also, the settings here overlap with some of those in config.yaml.
Why not just set dirty: true in config.yaml? That exists. color: true may soon exist (see #2441). We have control over what defaults we allow to be set here (i.e., we can keep them sane), and I don't mind adding more settings in config.yaml.

and with @davydden on the number of configuration files.

@citibeth
Copy link
Copy Markdown
Member Author

citibeth commented Jan 10, 2017 via email

@adamjstewart
Copy link
Copy Markdown
Member

I'm wondering why we have separate YAML files to begin with. They could all be concatenated into one.

Just because they could doesn't mean they should. They provide logical separation, so if I ask you for your compiler settings, you can give me your compilers.yaml and not a several hundred line behemoth.yaml. Or if I want to see your package settings, you can just give me your packages.yaml.

I don't mind a new aliases.yaml. I also don't mind putting them in config.yaml either.

@tgamblin
Copy link
Copy Markdown
Member

Looking at the specific command-line options that @adamjstewart and I have
said we want to use this for... do you mind adding those to config.yaml?

Nope. I like those actually. I also think making spack install show the spack spec output (or some type of summary) by default is a good idea.

@tgamblin
Copy link
Copy Markdown
Member

RE: configuration proliferation: I also like fewer config files, though I am not sure I would like a single one. So far:

  1. config.yaml: I'd like to put most new things here.
  2. packages.yaml: rationale was to make this separate because it can get very long. It's per-package data so I think it deserves its own file.
  3. compilers.yaml: again, this is long, per-compiler data. At LLNL has like 30 elements when spack is first run. compilers.yaml should eventually be consolidated with packages.yaml, when compilers are normal, potentially external dependencies. So this one will go away.
  4. mirrors.yaml, repos.yaml: these could probably be consolidated with config.yaml

One near-term concern I have about concatenating is that commands edit the YAML files, and while I've hacked the YAML parser a bit, we haven't hacked it to round-trip comments. So you'll lose comments for things like spack compiler add. If you consolidate the files, you'd lose any comments in your larger config.yaml too.

That said, consolidating is easy. The files all start with unique section keys (this was intentional) so concatenating them doesn't actually make them harder to parse or differentiate. @mplegendre and i have talked about allowing the top-level sections to be either separate files or concatenated into one. I think that would be easy to pull off.

@krafczyk
Copy link
Copy Markdown
Contributor

Could an argument --no-aliases be added to resolve this issue @tgamblin @citibeth? Personally I think @citibeth is probably right and there isn't really a problem adding this functionality, but if there was a special argument to turn it off that should relieve most of @tgamblin's concerns. Probably easier said than done though..

@citibeth
Copy link
Copy Markdown
Member Author

@mplegendre To be more specific... I would like the option of providing configuration scopes as a consolidated single file, rather than a directory of YAML files. I don't necessarily want to do this to my ~/.spack directory. But I am now generating a bunch of user-defined configuration scopes that go on top of the standard scopes (see #2686). For those, I would like to create a single file per scope, rather than a directory full of files. I'm not bothered by auto-generation issues, I don't intend to auto-generate these configurations anyway.

As we formalize ideas around Spack environments, I think it's worthwhile separating options that change how concretization works into a separate file. A quick check through docs on config.yaml indicates that is likely already the case (only packages.yaml changes concretization).

@citibeth
Copy link
Copy Markdown
Member Author

Personally I think @citibeth is probably right and there isn't really a problem adding this functionality

I said more than that. Actually, I said that Spack is already not scriptable because of packages.yaml. That horse has already left the barn.

The article on git explained why the --no-aliases option isn't preferable. But at least for the options I want to default, there is no need for it anyway.

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented Jul 9, 2020

Superseded by #17229

@tgamblin tgamblin closed this Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands feature A feature is missing in Spack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants