Conversation
|
This promises to bring some order to the chaos! :-)
spack help
Try this order: list, info, install, uninstall, find. Break out compilers and arch into a new section. Order the overal sections as:
If it doesn't already, Subcommands should stay under the same heading in The new sections on spack help --specHow was a user to ever know about this sub-subcommand? Really.. the original |
citibeth
left a comment
There was a problem hiding this comment.
Should there be an inversion of control here? i.e. when a subcommand is defined, you declare at that point in the code (a) which section it belongs in, and (b) whether it should be included in basic vs. extended help? I think that would be more maintainable than the structure here.
👍
Maybe call it "setup"? Also, does anyone even use
❤️ |
|
@citibeth @adamjstewart: thanks for the feedback. I like all these suggestions.
Agree. How do you recommend controlling the order in which commands appear? We could add |
|
@citibeth <https://github.com/citibeth>
Should there be an inversion of control here? i.e. when a subcommand is
defined, you declare at that point in the code (a) which section it belongs
in, and (b) whether it should be included in basic vs. extended help? I
think that would be more maintainable than the structure here.
Agree. How do you recommend controlling the order in which commands
appear? We could add section, advanced, and priority fields and us the
priority to order things in sections. I generally find it cumbersome to
maintain keys like that, but I do like having the section/difficulty
metadata in the commands themselves.
The other option would be to put "advisory" fields in the central
location. For example, an ordering could be listed centrally:
order = ('key1', 'key2', ...)
A unit test would check that the keys listed in the order field are exactly
the keys that should be in that section.
|
461ac47 to
8cba63a
Compare
7409556 to
4333f11
Compare
- Full help is now only generated lazily, when needed. - Executing specific commands doesn't require loading all of them. - All commands are only loaded if we need them for help. - There is now short and long help: - short help (spack help) shows only basic spack options - long help (spack help -a_ shows all spack options - Both divide help on commands into high-level sections - Commands now specify attributes from which help is auto-generated: - description: used in help to describe the command. - section: help section - level: short or long - Clean up command descriptions - Add a `spack docs` command to open full documentation in the browser.
4333f11 to
b29122b
Compare
|
Ok @citibeth @alalazo @adamjstewart: I think this is done.
I got rid of Note that the lazy loading speeds up all spack commands -- we don't have to load all the Thoughts? |
|
FYI: for more detail, see the PR description. I reworked it to include the various new features. |
alalazo
left a comment
There was a problem hiding this comment.
This is great! The improvements on spack help are evident, and now the help is clean and tidy. Things I liked most:
- Subsections are presented in the order they are needed
- Developers help is hidden by default
I think it's a "must merge immediately". Found just a minor point in _main.
| main(sys.argv) | ||
| # Once we've set up the system path, run the spack main method | ||
| import spack.main # noqa | ||
| sys.exit(spack.main.main()) |
There was a problem hiding this comment.
I think the clean-up here is remarkable, and will probably also help with #1385
| In a normal Spack installation, this is invoked from the bin/spack script | ||
| after the system path is set up. | ||
| """ | ||
| from __future__ import print_function |
There was a problem hiding this comment.
This python module is also quite an improvement over the previous state of things!
| return_val = command(parser, args) | ||
| except SpackError as e: | ||
| e.die() # gracefully die on any SpackErrors | ||
| except KeyboardInterrupt: |
There was a problem hiding this comment.
I think here we lost a couple of lines in rebase:
except Exception as e:
tty.die(str(e))0dab7e8 to
46cf615
Compare
| @@ -24,6 +24,8 @@ | |||
| ############################################################################## | |||
|
|
|||
| description = "run pydoc from within spack" | |||
There was a problem hiding this comment.
We now have 2 commands, spack doc and spack docs. Based on the name alone, it's impossible to guess what each command does and how they differ. Can we rename spack doc? What does spack doc even do?
There was a problem hiding this comment.
It just runs pydoc (aka help()) within the Spack system path. e.g., try: spack doc spack.spec.Spec
How about spack pydoc?
There was a problem hiding this comment.
Ooh, this is very interesting. Yeah, spack pydoc sounds better to me. Can you add documentation on both of these commands to the Developer Guide? Just a single paragraph blurb and an example for running spack pydoc.
|
|
||
| description = "print architecture information about this machine" | ||
| section = "system" | ||
| level = "short" |
There was a problem hiding this comment.
Still haven't decided whether spack arch should be long or short. It's not commonly used anymore now that we switched to distro and most distributions are detected correctly.
There was a problem hiding this comment.
We can always change it later. Personally I think people want to know what their architecture is.
lib/spack/spack/cmd/find.py
Outdated
| from spack.cmd import display_specs | ||
|
|
||
| description = "find installed spack packages" | ||
| description = "query and show installed packages" |
There was a problem hiding this comment.
I think I like the old description better. It solidifies the idea that spack find is for "finding" installed packages. A lot of new users confuse spack find and spack list because other packages managers use them for the reverse meanings. I'm not suggesting we rename these commands, at least not in this PR. But we should consider what other package managers use and if we differ from them we need to make it abundantly clear. I'll open up a separate issue to discuss naming of commands.
There was a problem hiding this comment.
See #4159 for potential flame wars on command renaming.
| help='command to get help on') | ||
| help_cmd_group = subparser.add_mutually_exclusive_group() | ||
| help_cmd_group.add_argument('help_command', nargs='?', default=None, | ||
| help='command to get help on') |
There was a problem hiding this comment.
I'm not sure I understand. Mutually exclusive groups generally have multiple options, only one of which can be used at the same time. Here you have 2 mutually exclusive groups. Did you mean to make one mutually exclusive group so that you can either run spack help <command> or spack help --all but not both?
There was a problem hiding this comment.
Good catch. There used to be spack help --spec, which was mutually exclusive from the others. It's gone now.
|
|
||
| description = "build and install packages" | ||
| section = "basic" | ||
| level = "short" |
There was a problem hiding this comment.
In the context of spack help --all, it makes more sense for install and uninstall to go in Building.
There was a problem hiding this comment.
Do you want it to be possible to move commands? I went for simple -- i.e., @citibeth's recommendation that things not move between short and long help, and the recommendation that commands only be in one section. Is it worth it to change those restrictions?
There was a problem hiding this comment.
I was actually proposing that install and uninstall be moved to Building for both short and long descriptions.
There was a problem hiding this comment.
Would you call basic usage something else in that case? I was trying to group the most important commands under basic usage.
There was a problem hiding this comment.
Nah, I would keep basic usage the same. building would be the next section so they don't have to travel far to see what they want.
There was a problem hiding this comment.
I would keep them in basic usage. What is more basic in Spack than installing and uninstalling stuff?
There was a problem hiding this comment.
Ok I actually tried this. I changed basic to query packages and it seems to work -- I like it. Update coming. install/uninstall/spec are the only build commands shown in the top-level help so it is still very clear.
lib/spack/spack/cmd/list.py
Outdated
| from llnl.util.tty.colify import colify | ||
|
|
||
| description = "print available spack packages to stdout in different formats" | ||
| description = "query and show packages that can be installed" |
There was a problem hiding this comment.
Based on my comment above, it might be better to say "list available packages". But I get that you want to make it clear that spack list can also accept an argument for searching.
| 'extensions': 'extensions', | ||
| 'help': 'further help', | ||
| 'packaging': 'packaging', | ||
| 'system': 'system', |
There was a problem hiding this comment.
Use Camel Case for these. For example:
Basic Usage
There was a problem hiding this comment.
Well, that's title case (because it has spaces), but sure.
I actually tried this, and here are the issues: "optional arguments" and "positional arguments" section headers are still lowercase due to being build into argparse, and getting rid of them is a pain. I tried overriding them in the constructor for SpackArgumentParser, and if you do that, the help argument still ends up in the original optional arguments group, and your arguments end up in the Optional Arguments group.
Also, we print the description field at the top of spack help <command>, and it looks weird to have that be lowercase and the section headers in title case. So I went with what argparse does and tried to stick to lowercase everywhere.
How important is this one to you?
There was a problem hiding this comment.
Ah, good point. Keep them lowercase then. I made the same change in #2847 because I noticed that the default --help messages were also lowercase.
There was a problem hiding this comment.
Cool. We can revisit this later if we want. But argparse is really nasty and I'm trying to avoid modifying its internals. The argparse package we have in external is a backport to support 2.6; nothing custom. I'd rather keep it that way.
There was a problem hiding this comment.
Agreed. There is actually a way to override these things without hacking argparse, but I'd rather not do it just to make things capitalized.
lib/spack/spack/main.py
Outdated
| section_order = { | ||
| 'basic': ['list', 'info', 'install', 'uninstall', 'find'], | ||
| 'build': ['fetch', 'stage', 'patch', 'configure', 'build', 'clean', | ||
| 'restage'] |
There was a problem hiding this comment.
As I mentioned above, it seems silly that install and uninstall aren't in this list.
There was a problem hiding this comment.
Do you want them in both lists, or do you want install to move between sections depending on short or long help?
There was a problem hiding this comment.
I think it makes more sense to always have them in build for both short and long. I know this adds a build group to the short help message. But it makes a lot more sense for the long help message.
There was a problem hiding this comment.
So, this brings up a question of what the help should say when we start having binary packages. In that case, install may default to installing a signed binary. Should it still go in build? What's a good section title that covers all that?
There was a problem hiding this comment.
Hmm. I don't know enough about the binary install plans. Wouldn't it install binaries if they are available and build from source if not? I think that's how Homebrew does it. I wouldn't want 2 commands for that.
There was a problem hiding this comment.
Yes. I'm just pointing out that if you stick it in a section called "building" that might be confusing. But maybe we can deal with that when we come to it.
I actually liked this part of the help message. For the first month of using Spack, I would refer to it when I was trying to remember what order things should go in (apparently order is sometimes important?). And it made sense that it was a recursive definition where each spec could depend on another spec with the same attributes. |
|
@adamjstewart: do you want to bring it back? I don't mind doing it, but where would you put it? Top of basic help? Bottom before the further help part? Separate command? |
|
@tgamblin How about top (or bottom) of long help? That way it doesn't clutter the short help. |
|
@adamjstewart @alalazo: Ok, I've updated (see above) and re-added |
with |
|
@alalazo @adamjstewart: I updated the
Thoughts? I like the prior |
c9f1f22 to
c9c4609
Compare
| libdwarf @g{%intel} ^libelf@g{%gcc} | ||
| libdwarf, built with intel compiler, linked to libelf built with gcc | ||
| mvapich2 @g{%pgi} @B{fabrics=psm,mrail,sock} | ||
| mvapich2, built with pgi compiler, with support for multiple fabrics |
There was a problem hiding this comment.
This is awesome, especially the examples. I can't wait to see all of this in color!
- Full help is now only generated lazily, when needed. - Executing specific commands doesn't require loading all of them. - All commands are only loaded if we need them for help. - There is now short and long help: - short help (spack help) shows only basic spack options - long help (spack help -a) shows all spack options - Both divide help on commands into high-level sections - Commands now specify attributes from which help is auto-generated: - description: used in help to describe the command. - section: help section - level: short or long - Clean up command descriptions - Add a `spack docs` command to open full documentation in the browser. - move `spack doc` command to `spack pydoc` for clarity - Add a `spack --spec` command to show documentation on the spec syntax.
- Full help is now only generated lazily, when needed. - Executing specific commands doesn't require loading all of them. - All commands are only loaded if we need them for help. - There is now short and long help: - short help (spack help) shows only basic spack options - long help (spack help -a) shows all spack options - Both divide help on commands into high-level sections - Commands now specify attributes from which help is auto-generated: - description: used in help to describe the command. - section: help section - level: short or long - Clean up command descriptions - Add a `spack docs` command to open full documentation in the browser. - move `spack doc` command to `spack pydoc` for clarity - Add a `spack --spec` command to show documentation on the spec syntax.
- Full help is now only generated lazily, when needed. - Executing specific commands doesn't require loading all of them. - All commands are only loaded if we need them for help. - There is now short and long help: - short help (spack help) shows only basic spack options - long help (spack help -a) shows all spack options - Both divide help on commands into high-level sections - Commands now specify attributes from which help is auto-generated: - description: used in help to describe the command. - section: help section - level: short or long - Clean up command descriptions - Add a `spack docs` command to open full documentation in the browser. - move `spack doc` command to `spack pydoc` for clarity - Add a `spack --spec` command to show documentation on the spec syntax.
Reworks the top-level Spack help to be more helpful.
Tasks:
bin/spacktospack.main.mainmethodspack docscommand opens docs in a browserspack help --spec, expand examplesThere are now three ways to get help:
spack help/spack -h: default short help for basic usage, with commands broken into some simple sections.spack help -a: help on everything, with more commands broken into more sections.spack help --spec: show help on the spec syntaxspack docs: open the spack help in a web browser.The default
spack helpnow looks like this:And
spack help --alllooks like this:Help contents
Commands now provide three properties that are used to auto-generate the help output:
sectioncontrols the top-level help sections in the help output, andlevelcontrols what goes in short or long help.main.py
Most of the complicated part of
bin/spackhas been moved intolib/spack/spack/main.py, which contains themain(args)method, which can be called programmatically. It returns an exit code.bin/spacknow just handles setting up the system path, and then it hands off to the spack package.There are some controls for
main.pythat let us override the default alphabetical command order and names of sections, so we can fine-tune the help output for clarity.Lazy command loading
Previously,
spackwould build a complete parser for all commands every time it is run. This means we have to load all the commands, which is a lot of python files.main.pyis a little fancier -- it builds a minimal parser with a dummy argument first, and loads only the needed command. This can speed things likespack locationup, even on my laptop with SSD:develop:features/better-help:Spack only ever loads all the commands when they're needed to build the full help output.
spack docsThere is now a
spack docscommand that opens the spack docs in your browser. That, the docs URL, and all the other help commands are described prominently at the bottom of the defaultspack -houtput.