Update: These questions have been answered in the comments below, resulting in significant evolution of this PR's implementation. Please see the primary description above for the most current explanation of the features and details of this PR

Questions for reviewers:

1. There are now two functions named all_commands() in the Spack system: one in spack.main which finds all available top level commands and caches them centrally, and the other in spack.cmd which is more general purpose, taking arguments. Is this a reasonable way to do things. Is there another way which would be preferable?
2. As a result of following the previous function structure, which looks like it was intended to keep "deep" knowledge of extensions restricted to extensions.py, there is a "bounce": spack.cmd.get_module() may call spack.extensions.get_module(), which in turn calls spack.cmd.get_module_from(). Is this acceptable (my compiled-language upbringing leaves me a little queasy, here) or is there some different code structure that would be preferable?

The current implementation does impose the convention that top level command spack xxx be located in xxx.cmd.xxx.py in order to avoid globbing and provide predictability for the import attempt. As you point out however, that breaks your canonical example (not intentional---I had forgotten that the canonical example did not follow that convention). If you think it important to maintain the flexibility I can do the initial glob through the command extensions and record the correct import location for each found command extension. This would allow any given configured extension location to be able to provide multiple new commands.

From the commit message for 'Address assorted comments': (1b18471)

spack.cmd.all_commands(command_list, command_paths) -> spack.cmd.find_commands(*command_paths). Any required caching should be external to this function.

spack.main.all_commands() has been restored to spack.cmd.

spack.cmd.help improvements:

• [drive-by] Correct help text for --spec.
• A failure to load a command for help purposes gives a full stack trace if --debug is active and a more helpful error message if not.

spack.extensions improvements:

• spack.extensions.load_command_extension(command, path) is now load_command_extension(command) and has absorbed the functionality of spack.extensions.get_module(command). Specifically, load_command_extension() will detect whether the extension command at issue can be imported using __import__() or should be loaded from source file via llnl.util.lang.load_module_from_file().
• Extension-specific exception class CommandNotFoundError providing information on unrecognized (e.g. misspelled) commands.
• A new function _init_extension_command_map() builds a map to track the correct location from which to load a particular command extension to avoid having to try each extension path in turn.

As a side effect of the above changes, the ability to have multiple command extensions per path has been restored.

Also, a module load failure during spack.main.add_all_commands() now produces a generic failure message identifying the command at issue, or a full stack trace if --debug is active.

@alalazo Ping.

@alalazo @scheibelp Since PR #11209 has been merged into develop and this PR has been rebased against same, it is no longer blocked.

Also: could you update the PR description? I think the following bit is stale:

spack.extensions.get_module() now uses import() (via spack.cmd.get_module_from()) rather than load_command_extension(), because the latter does not deal correctly with package-level globals,

And to be clear I assume that the following is the primary intent (especially now that #11209 has been merged):

Spack now behaves sensibly when a command is misspelled or otherwise cannot be found, including for spack help.

@scheibelp I think I've addressed everything: please let me know if I missed something.

While rationalizing the exception handling, I remove a handling block in main.py that was put there by #11209 as no longer necessary. I believe some recent changes by @tldahlgren might have facilitated that.

@alalazo @scheibelp Ping.

@alalazo @scheibelp @adamjstewart @tldahlgren @becker33 If no-one has substantive issues remaining after my last answer to Massimiliano, I would like to push to have this merged.

@chissg At a first glance I'm still of the opinion expressed in #11145 (comment) regarding get_module_from - I also need to find time to review this more thoroughly after your latest changes (hopefully within this week). In the meanwhile did you check if you could potentially refactor spack-dev to:

1. Maintain the same ui
2. Use the current extension model

and what would be required on the spack-dev extension side in case?

To be clear: I'm not asking that you actually do the refactor, I just want to understand if that refactor would be at all possible and what would be the pain points.

EDIT: Looking for a simple reproducer of your blockers was the intent of the mock extension created above.

@alalazo Thank you for your response.

In order to re-factor SpackDev to operate within the current extension model, the top level command would require explicit knowledge of every sub-command, including both a call to configure the parser and one or more calls to implement functionality. Subsequently extending SpackDev with new sub-commands would require the addition of (at minimum) two calls and an import to the top level command implementation (over and above the implementation of the sub-command module itself), vs no change whatsoever under this PR.

The ability to encapsulate sub-commands entirely and avoid the necessity for this explicit knowledge of sub-commands in the implementation of top level commands is analogous to the existing ability with top level commands (the Spack application main code has no explicit knowledge of the "install"—or any other—commands, for instance) and as a result the implementation of this feature uses much the same code. We can discuss the specifics of how that is achieved in order to satisfy technical concerns you may have, but to date I have not seen an explicit argument from you against the idea of extending command encapsulation to sub-commands.

Refactoring SpackDev to fit some different idea of where sub-command and other support code should be located either physically or within Python namespaces is practically trivial and can certainly be done. I am perfectly fine with enforcing the convention that commands and sub-commands be in a cmd namespace, for instance, but the idea of implicitly added sub-commands necessitates the ability to separate command and sub-command implementations both physically and logically to avoid confusion between commands and sub-commands, and between populations of sub-commands belonging to different commands.

I believe there is a useful discussion to be had on the relative merits of explicit per-file loading of code modules vs sys.path-based __import__() calls. In this I was following the precedent set by the top level command encapsulation, but am not particularly tied to one versus the other. I do believe the same method should be used for sub-command encapsulation and top level commands, and I was loathe to change the way it was done for top level commands just because reasons. If after discussion of the pros and cons we settle on the other rather than the one, I would be just as happy. However, given the pre-existing comment regarding the intended eventual replacement of llnl.util.lang.load_module_from_file() with importlib.import_module(), I was under the impression we would be converging on sys.path-based implementation for all commands including extensions, which would anyway necessitate some adjustment of the current extensions paradigm.

Finally: with this PR in its eventual final form and merged upstream, I see the scope for a further PR providing the turnkey ability to add sub-commands from a designated path and/or namespace.

I look forward to your comments on the technical aspects of this PR when you have had chance to review it in its current form.

Apologies but in order to meet our next targeted release https://github.com/spack/spack/projects/10 I think my next review will be pushed out to 10/21. I have added a reminder at that time and I hope to get to this sooner but currently that's my estimate.

@scheibelp Looks like quite the push! Thanks for the update. Looking forward to seeing you at SC19.

@scheibelp @alalazo I'd like to plan to discuss this at the telcon Thursday with a view to understanding what needs to be done to get this PR merged.

@alalazo @scheibelp To pick up from the discussion at FNALssi/spack-dev#5 (comment):

#11145 contains several improvements that have nothing to do with subcommand encapsulation, and in its earlier form solved a problem that was subsequently solved in a different way by @alalazo's #11209—things might have looked a little different if #11209 had already existed (see issue 1 below).

Allowing subcommand (etc.) discovery—while being originally motivated by a desire for encapsulation in SpackDev—has additional use cases. Fermilab's fork of Spack includes legacy support for the Fermilab-specific "UPS" package management system (FNALssi#21, FNALssi#23) in the form of two "module" types: ups_table and ups_version (cf Lmod, environment modules). It would be very desirable to be able to provide this functionality externally without altering Spack core in any way (since this is not something in which anyone outside Fermilab and its related experimental collaborations would be interested), and one prerequisite for same is the ability to add external subcommands to an internal Spack command (spack module). While subcommand discovery as currently envisioned in this PR does not enable that, it would be a step toward it.

With respect to the complexity of this PR:

• I believe that if necessary, several features of [WIP] Generalize command-finding to support sub-commands, incl. for extensions. #11145 are separable to a large extent (but still closely related) and could be multiple PRs.
• If we can resolve some technical issues (at least one of which would still exist without subcommand encapsulation), I believe the footprint of [WIP] Generalize command-finding to support sub-commands, incl. for extensions. #11145 required solely to enable subcommand encapsulation will be substantially reduced. I would also hope to be able to provide subcommand encapsulation with minimal additions to the command itself.

So, the issues to consider:

1. An evolution of the prescribed directory hierarchy to allow for the physical separation of subcommand implementations would allow subcommand location can be centralized without exposing either find_commands(...) or get_command_module_from(...) to the command implementation. Something like <ext>/cmd/<cmd1>.py, <ext>/cmd/<cmd1>/<subcmd1>.py, <ext>/cmd/<cmd1>/<subcmd1>/<subsubcmd1>.py, etc., to toss out a strawman example. Subcommand location should also work for internal Spack top level commands (and subcommands), and preferably there would need to be as little change as possible in behavior, code or directory structure in the event an extension command gets absorbed into Spack proper.
2. I originally went with generalizing the preexisting top-level-command mechanism (all_commands() -> find_commands(), get_module() -> get_command_module_from()) because it seemed more straightforward to extend that rather than build discovery into extension handling, and because I wanted the facility to be available to internal commands also.
   Generalizing the top-level command finding mechanism to handle subcommand location does accentuate the dichotomy between top-level internal commands and command extensions (__import__ vs load_module_from_file()). However, I would draw your attention to the comment in extensions.py:

   spack/lib/spack/spack/extensions.py

   Lines 90 to 91 in 24ec9f0

   	# TODO: Upon removal of support for Python 2.6 substitute the call
   	# TODO: below with importlib.import_module(module_name)

   
   If I understand the importlib documentation correctly, importlib.import_module(...) enforces the loading of modules just like Python's import keyword, implying that implementing the TODO here would push one toward harmonizing the extension loading mechanism with the top-level command mechanism anyway. Even without the TODO one could also argue that extension command loading should be import (or __import__())-like solely on the grounds of consistency with internal command loading.

Assuming we can agree:

1. on a directory hierarchy for extensions that is compatible with locating subcommands in extensions or Spack proper, and
2. that it is desirable for Spack and extension command loading to be harmonized (subject to the ability to retain extensions.<X> namespacing for configured extensions),

then I believe it is possible to significantly simplify extension command and subcommand location / loading while avoiding the need to add to sys.path or require the use of arbitrarily flexible commands (see find_commands(), get_command_module_from) to command implementations.

If I'm reading the various pieces of documentation correctly with respect to Python finders, it would appear that careful definition of a finder for extensions.<X> should allow for something like:

def get_command_module(cmd_name):
    spack.cmd.require_cmd_name(cmd_name)
    try:
        module = get_command_module_from(cmd_name, 'spack')
    except ImportError:
        module = get_command_module_from(cmd_name, 'spack.extensions')
    return module

where the implementation of get_command_module_from() uses importlib.import_module() rather than __import__(). One would need to ensure that ensure_package() gets called and the extension's directory structure is otherwise kosher (presumably as part of the finder). Unless there's a bug or limitation I missed, this should even work in Python 2.6 (some finesse in the definition of the finder to handle find_module() vs find_spec() might be appropriate).

One can argue over whether a Spack extension command should be allowed to implement and preempt an internal Spack command, in which case the order of the get_command_module() calls in the above snippet would be reversed.

Harmonizing the loading in this way also allows for a scheme for subcommand location and invocation along the lines of:

def setup_parser(subparser):
    sp = subparser.add_subparsers(metavar='SUBCOMMAND', dest='dev_command')
    spack.command_loading.add_subcommands(subparser)

def dev(parser, args):
    spack.command_loading.subcommand(args.dev_command)(parser, args)

add_subcommands() and subcommand() would both utilize the name of the calling module in conjunction with knowledge of the directory structure rules to find subcommand modules, load them, and connect to the parser or invoke their command function as appropriate.

One final advantage of harmonizing Spack command loading and extension command loading and providing the ability to discover modules in designated places is that it lays the groundwork for configuration and use of externally-provided build systems and module generators (and other Spack features) in addition to simply commands.

After that wild ride, a proposal for discussion on telcon:

1. A discussion of the two "...issues to consider..." with a view to finding agreement on the two points ("Assuming we can agree...").
2. A discussion of how to move forward with the features already present in [WIP] Generalize command-finding to support sub-commands, incl. for extensions. #11145 and any changes indicated by the previous discussion.

While harmonization of Spack command and extension command loading is not strictly necessary for the enablement of subcommand loading, it does seem to be indicated as independently desirable (via TODO), and utilizes most of the same abstractions and mechanisms.

If we can resolve some technical issues (at least one of which would still exist without subcommand encapsulation), I believe the footprint of #11145 required solely to enable subcommand encapsulation will be substantially reduced. I would also hope to be able to provide subcommand encapsulation with minimal additions to the command itself.

Can you write a brief bullet list of requirements for achieving "subcommand encapsulation"? I feel like we are using the same term for different things (as I believe in FNALssi/spack-dev#5 commands are as "encapsulated" as they were before under this PR).

I believe that if necessary, several features of #11145 are separable to a large extent (but still closely related) and could be multiple PRs.

If you can extract the part that improves error messages and error handling without touching the command loading / storing mechanism I think that would be a lot less controversial to discuss and will probably be merged faster.

However, I would draw your attention to the comment in [ ... ] of consistency with internal command loading.

Not sure what you mean above, but the broader explanation of why the comment is there can be found at #11209 (comment)

After that wild ride, a proposal for discussion on telcon: [ ... ]

👍 Can you point to the various features added besides the improvement in error-handling (see comment above) and the refactor of command loading?

If we can resolve some technical issues (at least one of which would still exist without subcommand encapsulation), I believe the footprint of #11145 required solely to enable subcommand encapsulation will be substantially reduced. I would also hope to be able to provide subcommand encapsulation with minimal additions to the command itself.

Can you write a brief bullet list of requirements for achieving "subcommand encapsulation"? I feel like we are using the same term for different things (as I believe in FNALssi/spack-dev#5 commands are as "encapsulated" as they were before under this PR).

I think we agree on the definition, but my plan was always to eventually merge the bulk of the work done in spack-dev's top level command into a class or pair of functions in Spack core that could be accessed by any command, either an extension command or one within Spack core. I wanted to get some eyes on everything first and have things mature before abstracting out that code for a subsequent PR.

I think this is an important point, because then we can consider:

1. A large part of import_subcommands_automatically() from FNALssi/spack-dev#5 can be replaced by find_commands() from [WIP] Generalize command-finding to support sub-commands, incl. for extensions. #11145.
2. import_subcommands_automatically() has a raw use of __import__(), of which one would like to avoid yet another example in Spack core (and still more uses in individual commands if never abstracted into core). Ideally one would generalize and re-factor the command loading (and/or extension command loading) to accommodate the required extra flexibility without essentially duplicating code.
3. As I said in the loooong comment above: the interface exposed to the top level commands themselves looks a whole lot less arbitrary if we can agree on a prescribed directory structure for extension commands that accommodates subcommands in predictable locations within the command package.

With the three points above, I believe that the reasonable changes to Spack core to that would accommodate the generalization of the subcommand handling code from FNALssi/spack-dev#5 would start to look very similar to what would be in #11145 + generalization of the subcommand handling code from FNALssi/spack-dev + simplifications allowed by point (3).

#11209 was an alternative implementation of part of #11145, rather than being an improvement to same growing out of a conversation and shared understanding of what SpackDev needed from an extension command system that #8612 did not provide. In a post-#11209 world, I realize that #11145 would look somewhat different than it does now but I was not successful in starting a conversation (regarding, among other things, point (3)) that would lead to those improvements.

However, I would draw your attention to the comment in [ ... ] of consistency with internal command loading.

Not sure what you mean above, but the broader explanation of why the comment is there can be found at #11209 (comment)

After re-reading #11209 (comment), I'd like to understand what is "not recommended" about __import__() apart from the obvious scary complexity of the function. In a world that still has to support Python 2.6, I would have thought that __import__() would be a reasonable contender in the absence of importlib.import_module(). I believe that, if it can be done so elegantly, it would be beneficial to reduce the distinction between loading methods of internal and external Spack commands. If it were possible to reduce the number of command loading calls from two (or three, with respect to FNALssi/spack-dev#5) to one, so much the better.

After that wild ride, a proposal for discussion on telcon: [ ... ]

👍 Can you point to the various features added besides the improvement in error-handling (see comment above) and the refactor of command loading?

• The command caching in extensions.py improves performance of the help and commands functions while providing a consistent load time for an extension command that is not dependent on its place in the extension path configuration order.
• The {require_,}{cmd,python}_name() functions provide the ability to easily encode the requirements for any code handling commands or the files in which they are implemented and to convert between one naming convention and the other as required. Code using same where appropriate is automatically more transparent and understandable.
• find_commands() is an abstraction from all_commands() which can be reused by the extension command caching and any code handling subcommands.

The command caching in extensions.py improves performance of the help and commands functions while providing a consistent load time for an extension command that is not dependent on its place in the extension path configuration order.

I have 3 extensions enabled, spack-dev is searched last:

config:
  extensions:
  - /home/culpo/PycharmProjects/spack-container
  - /home/culpo/PycharmProjects/spack-testcommand
  - /home/culpo/PycharmProjects/spack-dev

Timing on the current develop:

$ git status
On branch develop
Your branch is up to date with 'upstream/develop'.

nothing to commit, working tree clean

$ time spack help &> /dev/null

real	0m0,607s
user	0m0,533s
sys	0m0,058s

$ time spack commands &> /dev/null

real	0m0,448s
user	0m0,395s
sys	0m0,036s

$ time spack dev -h 
usage: spack dev [-h] SUBCOMMAND ...

develop multiple Spack packages simultaneously

positional arguments:
  SUBCOMMAND
    getdeps   install missing dependencies of packages in a SpackDev area
    init      initialize a spackdev area
    info      describe a spackdev area
    build_env
              run a command in the build environment of a spackdev package, or start a shell in same.
    stage     stage packages in a spackdev area
    findext   search for system packages and add to packages.yaml

optional arguments:
  -h, --help  show this help message and exit

real	0m0,414s
user	0m0,363s
sys	0m0,033s

Timing on this branch:

$ git status
On branch PR/11145
nothing to commit, working tree clean

$ time spack help &> /dev/null

real	0m0,607s
user	0m0,555s
sys	0m0,036s

$ time spack commands &> /dev/null

real	0m0,459s
user	0m0,414s
sys	0m0,028s

$ time spack dev -h 
usage: spack dev [-h] SUBCOMMAND ...

develop multiple Spack packages simultaneously

positional arguments:
  SUBCOMMAND
    getdeps   install missing dependencies of packages in a SpackDev area
    init      initialize a spackdev area
    info      describe a spackdev area
    build_env
              run a command in the build environment of a spackdev package, or start a shell in same.
    stage     stage packages in a spackdev area
    findext   search for system packages and add to packages.yaml

optional arguments:
  -h, --help  show this help message and exit

real	0m0,407s
user	0m0,358s
sys	0m0,031s

On my development box there's no sensible speed-up (nor any slow-down). Can you share your numbers?

@alalazo Starting with spack-scripting, I created 5 differently-named versions of the filter command, and then made 3 more copies of the tree, again tweaking the command name, giving us four groups of five commands filter{,x,y,z}{,2,3,4,5}. Running multiple times to account for caching and jitter, the run to run jitter is wider than any difference between commands in develop vs #11145 , or between spack filter -h and spack filterz5 -h in either case. It appears that this is a case of premature pessimization: mea culpa.

Closing as stale (trying to clean up old PRs). Feel free to reopen if you think otherwise.