Spack can be extended with external commands by alalazo · Pull Request #8612 · spack/spack

alalazo · 2018-06-29T14:00:35Z

This PR gives Spack the ability to hook external commands. This is done adding a new keyword (extensions) to config.yaml, which contains a list of paths pointing to valid extensions.

Changelist

Added support to hook external commands
Added support to spack create, so that it can create a stub structure for extensions
Added documentation on external commands

What is a valid extension?

A valid extension is any path in your filesystem which respect a certain naming and layout for files:

$ tree spack-scripting/
spack-scripting/ # The top level directory must match the format 'spack-{extension_name}'
├── pytest.ini # Optional file if the extension ships its own tests
├── scripting # Folder that may contain modules that are needed for the extension commands
│   └── cmd # Folder containing extension commands
│       └── filter.py # A new command that will be available 
└── tests # Tests for this extension
    ├── conftest.py 
    └── test_filter.py
└── templates # Templates that may be needed by the extension

Both naming and layout are tentative at the moment, so comments to improve them are welcome.

Example

To try out a sample extension:

Clone spack somewhere.
Clone this repository:

$ pwd
/home/user

$ mkdir tmp && cd tmp
$ git clone https://github.com/alalazo/spack-scripting.git
Cloning into 'spack-scripting'...
remote: Counting objects: 11, done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 11 (delta 0), reused 11 (delta 0), pack-reused 0
Receiving objects: 100% (11/11), done.

Add the following line to config.yaml:

config:
  extensions:
    -  /home/user/tmp/spack-scripting

To check that everything is hooked properly, try looking for the filter command:

$ spack filter --help
usage: spack filter [-h] [--installed | --not-installed]
                    [--explicit | --implicit] [--output OUTPUT]
                    ...

filter specs based on their properties

positional arguments:
  specs            specs to be filtered

optional arguments:
  -h, --help       show this help message and exit
  --installed      select installed specs
  --not-installed  select specs that are not yet installed
  --explicit       select specs that were installed explicitly
  --implicit       select specs that are not installed or were installed implicitly
  --output OUTPUT  where to dump the result

You can also run the unit tests that are shipped with an extension:

$ spack test --extension=scripting
============================================================== test session starts ===============================================================
platform linux -- Python 3.6.5, pytest-3.2.5, py-1.4.34, pluggy-0.4.0
rootdir: /home/mculpo/tmp/spack-scripting, inifile: pytest.ini
collected 5 items                                                                                                                                 

tests/test_filter.py ...xx
============================================================ short test summary info =============================================================
XFAIL tests/test_filter.py::test_filtering_specs[flags3-specs3-expected3]
XFAIL tests/test_filter.py::test_filtering_specs[flags4-specs4-expected4]

=========================================================== slowest 20 test durations ============================================================
4.82s setup    tests/test_filter.py::test_filtering_specs[flags0-specs0-expected0]
0.20s teardown tests/test_filter.py::test_filtering_specs[flags4-specs4-expected4]
0.17s call     tests/test_filter.py::test_filtering_specs[flags4-specs4-expected4]
0.14s call     tests/test_filter.py::test_filtering_specs[flags2-specs2-expected2]
0.13s call     tests/test_filter.py::test_filtering_specs[flags3-specs3-expected3]
0.13s call     tests/test_filter.py::test_filtering_specs[flags1-specs1-expected1]
0.11s call     tests/test_filter.py::test_filtering_specs[flags0-specs0-expected0]
0.00s setup    tests/test_filter.py::test_filtering_specs[flags3-specs3-expected3]
0.00s setup    tests/test_filter.py::test_filtering_specs[flags2-specs2-expected2]
0.00s setup    tests/test_filter.py::test_filtering_specs[flags4-specs4-expected4]
0.00s setup    tests/test_filter.py::test_filtering_specs[flags1-specs1-expected1]
0.00s teardown tests/test_filter.py::test_filtering_specs[flags2-specs2-expected2]
0.00s teardown tests/test_filter.py::test_filtering_specs[flags0-specs0-expected0]
0.00s teardown tests/test_filter.py::test_filtering_specs[flags3-specs3-expected3]
0.00s teardown tests/test_filter.py::test_filtering_specs[flags1-specs1-expected1]
====================================================== 3 passed, 2 xfailed in 5.78 seconds =======================================================

davydden

I think it’s a nice feature 👍

gartung · 2018-10-09T17:52:30Z

@alalazo What do you do if you need to load another python file from the same extension direcotry as the custom command?

greenc-FNAL · 2018-10-31T19:00:30Z

@alalazo @tgamblin @adamjstewart @scheibelp What's the status on this PR, please? It seems moribund, but we (Fermilab / SpackDev) are very interested in it. So much so, in fact, that we have some proposed changes, viz:

spack.cmd.all_commands() is more general, allowing for the function to locate extension subcommands.
spack.cmd.get_module() now treats extension commands more like standard Spack commands, importing them as modules rather than loading from file. This allows (e.g.) extension commands to use package globals to pass data between setup_parser() and my_command_name() functions.
New command spack.cmd.get_module_from() to enable loading of modules from namespaces other than spack -- essential for allowing extension subcommands, and providing handy factorization for the extension command module loading functionality.
This is a breaking change from the original PR, as __init__.py files are now necessary at the cmd_name/, cmd_name/cmd, and cmd_name/cmd/cmd_name/ directory levels.

How should we proceed, please? Non-null options include:

Submit a PR directly to https://github.com/epfl-scitas/spack.git:features/command_plugin
Make a new PR to supercede this one.
Post code implementing the above as requested changes on the current PR.

Any input appreciated.

alalazo · 2018-10-31T22:46:36Z

@alalazo @tgamblin @adamjstewart @scheibelp What's the status on this PR, please? It seems moribund, but we (Fermilab / SpackDev) are very interested in it.

The PR is not abandoned but just waiting for somebody to review it (in fact I have quite a few PRs in that state). As there's interest, I'll rebase it asap.

So much so, in fact, that we have some proposed changes, viz
...
How should we proceed, please? Non-null options include:

Submit a PR directly to https://github.com/epfl-scitas/spack.git:features/command_plugin

Make a new PR to supercede this one.

Post code implementing the above as requested changes on the current PR.

If you don't mind I'd proceed with 3. (basically post a review to the PR with requested changes). I am not sure I fully got what you need and that would give me the opportunity to discuss and understand your use case. Would that be fine with you?

…ck#8612 (ish)

greenc-FNAL · 2018-11-01T13:59:13Z

We can certainly discuss it here, but at 200+ lines for the two commits I have, pasting them in here would get pretty messy. I can point you to the relevant branch of my fork and we can refer to items in there, perhaps?

The commits in question are from my feature/enhanced-comments branch, specifically:
spack.cmd.all_commands becomes more general..., and Module loading enhancements....

The first commit generalizes the command-finding facilities of Spack to allow for the location of commands in places other than spack/cmd/, and the second factorizes and generalizes Spack's import-based command module loading system to facilitate not only the import-based loading of extension commands, but also the loading of sub-commands from arbitrary namespaces. As a byproduct of the latter, the new function llnl.util.lang.load_module_from_file() is obsolete and could be removed, but that is not addressed by these commits.

Taken together, these commits enable the formulation of extensions like SpackDev, where the top level command dev connects to (from its point of view) an arbitrary set of sub-commands in a similar fashion as the Spack command itself. Evolving the concept further, add_subcommand() could be generalized and added to spack.cmd, making the addition of sub-commands to extension commands (or top-level Spack commands, or sub-commands of either) trivial.

Thoughts appreciated.

greenc-FNAL · 2019-02-25T21:35:31Z

@alalazo What is the nature of your push on Dec 4? Was it just a rebase or were there substantive changes. Should I update my enhancements branch with respect to epfl-scitas:features/command_plugin? Our SpackDev product has been converted to be an external subcommand and utilize Spack data structures, so we now rely on this functionality. We'd like to get this PR and our enhancements into Spack mainline relatively soon, if possible.

alalazo · 2019-02-27T10:24:39Z

@chissg

What is the nature of your push on Dec 4?

I am regularly rebasing most of my PRs on top of the latest develop to:

Solve conflicts with recently merged PRs as soon as I can
Retrigger CI so that the PR will be tested when merged with a more recent commit in develop

So there have been no additional commits or changes.

Our SpackDev product has been converted to be an external subcommand [...]

That's great to hear! I'll also hope that this and other PRs of mine will get reviewed soon, but as the number of contributions grows I understand this might require some time due to priorities. Would it be worth to try to incorporate your changes in this PR in some way, and then ping people more aggressively to review it once we are ready?

scheibelp · 2019-03-14T22:17:37Z

I can start looking at this on 3/25

greenc-FNAL · 2019-03-21T14:21:59Z

@alalazo Sorry I missed your last entry here. I promised @scheibelp I would formally review this before next week in my capacity as someone who is using this PR already. I am perfectly fine with your adding my (just brought up-to-date with develop and your PR branch) enhancement branch to your PR and making whatever changes you feel necessary to integrate them. Is there anything about what they do that you disagree with?

greenc-FNAL

I believe this is fine as far as it goes. My suggested enhancements could be added to this PR or submitted by me separately post-merge.

Desirable enhancements for the future:

Command extensions to be provided from external repos.
More flexibility in paths to extension commands and location of (e.g.) subcommands and supporting code.
Ability to use spack flake8 to examine command extensions and supporting code.

scheibelp

A few requests:

where possible, wrap details of management of the extension directory into the extension module. I have a couple comments about this; in general that means avoiding using config.get("extensions") in modules outside the extensions module.
there are a couple stylistic changes that don't appear related to the PR. I'd prefer they were removed (if you want me to squash) or placed in separate commits (if you want to rebase)

lib/spack/spack/cmd/__init__.py

lib/spack/spack/test/pytest.ini

scheibelp · 2019-03-25T17:51:00Z

lib/spack/spack/test/conftest.py


 @pytest.fixture(scope='session')
-def _ignore_stage_files():
+def ignore_stage_files():


This appears unrelated to other changes in the PR. If suddenly other test modules were using this fixture, removing the preceding _ would make more sense.

I think fixtures should always be without preceding _, but can live with that. ~~Will change it.~~

If suddenly other test modules were using this fixture, removing the preceding _ would make more sense.

Sorry for the noise above, I had to recall why I did this. It turns out this change is needed if you want to use the the statement:

from spack.test.conftest import *

in an extension command. By default, if __all__ is not defined, the statement above imports all names that are not prefixed with an underscore.

scheibelp · 2019-03-25T18:14:38Z

lib/spack/spack/cmd/__init__.py

+        tty.debug('Imported {0} from built-in commands'.format(pname))
+    except ImportError:
+        # If built-in failed the import search the extension
+        # directories in order


Does "in order" matter? In one sense it does: any extension directory can support multiple commands, and so you could have conflicting implementations of a command (although if that was the intent with this PR I would be concerned about handling template conflicts).

If conflicts are the main reason for imposing an order, I think it would be simpler to require that each extension directory only supports one command. I acknowledge it may be convenient to implement several commands in one extension directory (in particular if they have related functionality that you want to test); so in general I'm OK with having multiple commands in one extension directory.

Also, IMO the next several lines should be placed into a function inside the extensions module.

Does "in order" matter? In one sense it does: any extension directory can support multiple commands, and so you could have conflicting implementations of a command

"In order" matters for that reason. If there are multiple commands with the same name you'll currently get the first one that is matched. Those kind of conflicts are probably a niche use-case right now, but this seemed a simple and sensible solution at the time of writing the PR. Templates should work the same, even though again I don't think anybody came up with a use case that requires overriding a template completely.

I acknowledge it may be convenient to implement several commands in one extension directory (in particular if they have related functionality that you want to test); so in general I'm OK with having multiple commands in one extension directory.

This was my intent when writing this. An extension is meant to provide functionality for a specific purpose and it can be that more than one command is needed for that.

Also, IMO the next several lines should be placed into a function inside the extensions module.

👍

scheibelp · 2019-03-25T18:23:29Z

lib/spack/spack/extensions.py

+        Paths where to search for command files.
+    """
+    for path in paths:
+        extension = extension_name(path)


Is there a reason to make this a warning instead of a failure? IMO if the user adds an extension folder, they will not want to skip it if they added it incorrectly - they would likely prefer the process to stop.

The reasoning is that this doesn't compromise Spack's core functionality, so it should not be an error. I can change it if people prefer otherwise.

lib/spack/spack/tengine.py

alalazo · 2019-03-26T12:23:29Z

@scheibelp @chissg Let me know if, besides any other modification to the existing code, you'd like to implement in this PR the two missing items in the description above (support for the creation of a stub extension and docs).

greenc-FNAL · 2019-03-26T13:03:36Z

@alalazo I have no immediate need for the docs or command extension, although I would agree they are both desirable.

scheibelp

I have one additional request

lib/spack/spack/cmd/__init__.py

fixes spack#7967 This commit add hooks to Spack so that it can extend commands reading from a set of external folders. The extensions must respect a strict directory layout to be considered valid.

The command to run unit tests has been modified to allow running tests in Spack's extensions.

…#11209) #8612 added command extensions to Spack: a command implemented in a separate directory. This improves the implementation by allowing the command to import additional utility code stored within the established directory structure for commands. This also: * Adds tests for command extensions * Documents command extensions (including the expected directory layout)

alalazo added feature A feature is missing in Spack WIP commands labels Jun 29, 2018

alalazo requested review from adamjstewart, davydden and tgamblin June 29, 2018 14:00

davydden reviewed Jun 29, 2018

View reviewed changes

alalazo mentioned this pull request Aug 16, 2018

imports: spack uses importlib when available, instead of imp #8995

Merged

2 tasks

alalazo force-pushed the features/command_plugin branch from 9ac801c to af6a355 Compare August 29, 2018 09:13

greenc-FNAL mentioned this pull request Sep 20, 2018

Updates from building CMSSW FNALssi/spack#1

Closed

alalazo force-pushed the features/command_plugin branch from af6a355 to 3d0b5b3 Compare October 31, 2018 22:47

greenc-FNAL added a commit to greenc-FNAL/spackdev that referenced this pull request Nov 1, 2018

Checkpoint on conversion to Spack command extension, per PR spack/spa…

ba90a5d

…ck#8612 (ish)

becker33 requested a review from scheibelp November 1, 2018 20:57

alalazo force-pushed the features/command_plugin branch from 3d0b5b3 to 91c71a1 Compare December 4, 2018 14:31

alalazo mentioned this pull request Jan 16, 2019

Added a new command to filter input specs based on their properties #7899

Closed

alalazo force-pushed the features/command_plugin branch from 91c71a1 to d5c47bd Compare February 27, 2019 10:13

scheibelp self-assigned this Mar 14, 2019

greenc-FNAL requested review from davydden and removed request for davydden March 21, 2019 21:19

greenc-FNAL approved these changes Mar 21, 2019

View reviewed changes

scheibelp requested changes Mar 25, 2019

View reviewed changes

alalazo force-pushed the features/command_plugin branch from d5c47bd to c6ae2da Compare March 26, 2019 11:57

alalazo mentioned this pull request Mar 26, 2019

Added regression marker to pytest.ini #11011

Merged

scheibelp requested changes Mar 27, 2019

View reviewed changes

lib/spack/spack/cmd/__init__.py Outdated Show resolved Hide resolved

alalazo added 3 commits March 28, 2019 11:14

Spack can now hook external commands

f51350a

fixes spack#7967 This commit add hooks to Spack so that it can extend commands reading from a set of external folders. The extensions must respect a strict directory layout to be considered valid.

External command can provide their own tests

d6206da

The command to run unit tests has been modified to allow running tests in Spack's extensions.

Sort list outside of loops

8d0950d

alalazo force-pushed the features/command_plugin branch from c6ae2da to 16fdf56 Compare March 28, 2019 11:32

alalazo removed the WIP label Mar 28, 2019

alalazo added 4 commits March 28, 2019 16:32

Factored getting an extension module into its own function

c92a054

Factored retrieving template dirs for extensions into its own function

c0259ba

Factored getting extensions command paths into its own function

6500e24

Added tests for 'llnl.util.lang.load_module_from_file'

df50d2b

alalazo force-pushed the features/command_plugin branch from 16fdf56 to df50d2b Compare March 28, 2019 15:46

scheibelp approved these changes Mar 28, 2019

View reviewed changes

scheibelp merged commit 0a00635 into spack:develop Mar 28, 2019

alalazo deleted the features/command_plugin branch March 29, 2019 08:13

alalazo mentioned this pull request Apr 17, 2019

Spack extensions can import code from modules in root or cmd folder #11209

Merged

2 tasks

tgamblin mentioned this pull request Jul 8, 2019

Export Spack API #11824

Closed

greenc-FNAL mentioned this pull request Nov 4, 2019

[WIP] Generalize command-finding to support sub-commands, incl. for extensions. #11145

Closed

alalazo mentioned this pull request Apr 26, 2021

[WIP] Update dynamic import: avoid deprecated method #20072

Closed

Conversation

alalazo commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelist

What is a valid extension?

Example

Uh oh!

davydden left a comment

Choose a reason for hiding this comment

Uh oh!

gartung commented Oct 9, 2018

Uh oh!

greenc-FNAL commented Oct 31, 2018

Uh oh!

alalazo commented Oct 31, 2018

Uh oh!

greenc-FNAL commented Nov 1, 2018

Uh oh!

greenc-FNAL commented Feb 25, 2019

Uh oh!

alalazo commented Feb 27, 2019

Uh oh!

scheibelp commented Mar 14, 2019

Uh oh!

greenc-FNAL commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greenc-FNAL left a comment

Choose a reason for hiding this comment

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alalazo commented Mar 26, 2019

Uh oh!

greenc-FNAL commented Mar 26, 2019

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alalazo commented Jun 29, 2018 •

edited

Loading

greenc-FNAL commented Mar 21, 2019 •

edited

Loading

alalazo Mar 26, 2019 •

edited

Loading