Split 'module' into multiple commands by alalazo · Pull Request #7098 · spack/spack

alalazo · 2018-01-28T17:41:00Z

fixes #2215
fixes #2570
fixes #4400
fixes #6676
fixes #7281
fixes #8646

Summary

$ spack module <action> -m <module_type>

has been turned into multiple

$ spack module <module_type> <action>

commands, each one tied to a specific module type. This permits the specialization of the new commands with features that are module type specific.

A few known bugs have been fixed in the process.

List of modifications

Split spack module command
Update references in docs
Add unit tests for new, module specific, subcommands
Add regression test for Only create module files for explicitly installed packages #4400 + a few lines in the docs on how to blacklist implicit packages
Return a non-zero value if spack load fails for any reason
Added the --dependencies option to spack help load
Update auto-completion for commands
Regenerate a Docker image for the modules tutorial, and point to that in the docs
Issue a warning for the deprecated use of the spack module command

Example: specify a default module to be loaded for lmod

As an example the possibility to specify a default module to be loaded for lmod has been implemented:

$ module av

---------------------------------------------------------------- /home/mculpo/PycharmProjects/spack/share/spack/lmod/linux-ubuntu14.04-x86_64/Core ----------------------------------------------------------------
   bzip2/1.0.6-wig75ws    libsigsegv/2.11-fotodb6    m4/1.4.18-r26rlmg    zlib/1.2.8-nfd5zfc    zlib/1.2.11-eksallf (D)

------------------------------------------------------------------------------------------- /usr/share/modules/versions -------------------------------------------------------------------------------------------
   3.2.10

----------------------------------------------------------------------------------------- /usr/share/modules/modulefiles ------------------------------------------------------------------------------------------
   dot    module-git    module-info    modules    null    use.own

  Where:
   D:  Default Module

Use "module spider" to find all possible modules.
Use "module keyword key1 key2 ..." to search for all possible modules matching any of the "keys".

$ ls /home/mculpo/PycharmProjects/spack/share/spack/lmod/linux-ubuntu14.04-x86_64/Core/zlib/
1.2.11-eksallf.lua  1.2.8-nfd5zfc.lua

$ spack module lmod setdefault [email protected]
$ module av

---------------------------------------------------------------- /home/mculpo/PycharmProjects/spack/share/spack/lmod/linux-ubuntu14.04-x86_64/Core ----------------------------------------------------------------
   bzip2/1.0.6-wig75ws    libsigsegv/2.11-fotodb6    m4/1.4.18-r26rlmg    zlib/1.2.8-nfd5zfc (D)    zlib/1.2.11-eksallf

------------------------------------------------------------------------------------------- /usr/share/modules/versions -------------------------------------------------------------------------------------------
   3.2.10

----------------------------------------------------------------------------------------- /usr/share/modules/modulefiles ------------------------------------------------------------------------------------------
   dot    module-git    module-info    modules    null    use.own

  Where:
   D:  Default Module

Use "module spider" to find all possible modules.
Use "module keyword key1 key2 ..." to search for all possible modules matching any of the "keys".

$ ls -l /home/mculpo/PycharmProjects/spack/share/spack/lmod/linux-ubuntu14.04-x86_64/Core/zlib/
total 8
-rw-rw-r-- 1 mculpo mculpo 1431 gen 28 14:36 1.2.11-eksallf.lua
-rw-rw-r-- 1 mculpo mculpo 1423 gen 28 14:36 1.2.8-nfd5zfc.lua
lrwxrwxrwx 1 mculpo mculpo   17 gen 28 18:39 default -> 1.2.8-nfd5zfc.lua

alalazo · 2018-07-05T13:34:22Z

@tgamblin @adamjstewart Do you have time to look at this PR? Despite the lot of lines changed, the PR only restructures the module command so that:

$ spack module <subcommand> -m <module_type> ...

becomes:

$ spack <module_type> <subcommand> ...

and adds a new feature specific to lmod:

$ spack lmod setdefault

The rationale is to differentiate the commands so that we could map features that are specific to each module type in a more natural manner.

adamjstewart · 2018-07-05T15:13:45Z

I've never used spack module before, I've always used module load, so no opinions from me.

alalazo · 2018-07-10T15:47:07Z

@ax3l @davydden @healther and others, can you please give a test drive to this PR and see if it fixes the issues mentioned in the description in a way you like?

I went just through the module file tutorial and everything seems to work as expected.

ax3l

Just from reviewing the UI, it works great for me! Thank you so much!
Solves both the proper error on load of missing packages as well as loading only one package.

healther · 2018-07-12T05:11:08Z

Not sure if I'll get to it today, but I'll try to have tested this by the end of tomorrow

healther · 2018-07-12T08:19:59Z

Couple of things that I found (but I'm not sure whether I'm at fault or the current implementation):

Doing spack lmod refresh errors out with

==> Error: 'core_compilers' key not found in configuration file

while the other two (spack dotkit refresh and spack tcl refresh) don't produce errors (at least one of these commands touched my module files, but I didn't check which one). I'm uncertain whether all should work as I only have a spack-provided environment-module available (and I don't understand how the module systems relate to the module command)

I expected to have e.g. the perl module load its dependencies now. I.e. I expected autoload: direct to be the default, but maybe I misremembered.

On a more high level note: I think we should rearrange the help now, currently there is this section:

environment:
  dotkit                manipulate dotkit module files
  lmod                  manipulate hierarchical module files
  load                  add package to environment using `module load`
  tcl                   manipulate non-hierarchical module files
  unload                remove package from environment using `module unload`
  view                  produce a single-rooted directory view of packages

The module systems should be somehow group, shouldn't they?

'spack module' has been split into multiple commands, each one tied to a specific module type. This permits the specialization of the new commands with features that are module type specific (e.g. set the default module file in lmod when multiple versions of the same package are installed at the same time).

Added a subcommand that sets the default module to be loaded, if multiple installations of the same package are present.

This just because the fixture has been moved one level above the one it was originally defined. In this more general context there's more than one configuration file that could be patched for tests.

fixes spack#4400 The feature requested in spack#4400 was already part of the module file configuration, but it was neither tested nor documented. This commit takes care of adding a few lines in the documentation and a regression test.

This reverts commit 732c24f.

fixes spack#2215 fixes spack#2570 fixes spack#6676 fixes spack#7281 closes spack#3827 This PR reverts the use of `spack module loads` in favor of `spack module find` when loading module files via Spack. After this PR `spack load` will accept a single spec at a time, and will be able to interpret correctly the `--dependencies` option.

fixes spack#6321

As requested in the review all the commands meant to manage module files have been grouped under the `spack module` command. Unit tests have been refactored to match the new command structure.

alalazo · 2018-07-23T06:57:42Z

@tgamblin I uploaded spack/module-tutorial:latest on Dockerhub and made the requested changes. Let me know if there's anything else I need to do.

tgamblin · 2018-07-23T08:24:46Z

Can we tweak this slightly:

$ spack module refresh --delete-tree -m tcl -m lmod -y cmake lmod ^python
==> Error: deprecated command. Try to use:

        $ spack module tcl refresh --delete-tree -y cmake lmod ^python
        $ spack module lmod refresh --delete-tree -y cmake lmod ^python

instead.

to look at the -m argument and print:

==> Error: `spack module -m tcl` has been moved. Try this instead:
        $ spack module tcl refresh --delete-tree -y cmake lmod ^python

We should either say it's moved (if it's broken) or warn and do the old thing until we finally remove it (if it's deprecated)

In case a deprecated form of the module command is used, the program will exit non-zero and print an informative error message suggesting which command should be used instead.

alalazo · 2018-07-23T12:28:18Z

@tgamblin Latest commit

$ spack module refresh --delete-tree -m tcl -m lmod -y cmake lmod ^python
==> Error: `spack module refresh -m tcl -m lmod ...` has been moved. Try this instead:

        $ spack module tcl refresh --delete-tree -y cmake lmod ^python
        $ spack module lmod refresh --delete-tree -y cmake lmod ^python

tgamblin

@alalazo: this is looking very good to me. One last question on recursive loading below.

tgamblin · 2018-07-24T06:26:08Z

share/spack/setup-env.sh

                    fi ;;
                "load")
-                    if _sp_full_spec=$(command spack $_sp_flags module loads --input-only $_sp_subcommand_args --module-type tcl "${_sp_spec[@]}"); then
+                    if _sp_full_spec=$(command spack $_sp_flags module tcl find $_sp_subcommand_args "${_sp_spec[@]}"); then


@alalazo: doesn't this and the rest of d4b4be4 get rid of the loading of dependencies? Is this PR a decent place to fix that or is this something you want to save for later? I can't find the PR where we recently discussed this, but I do think the default module scheme should load transitive run dependencies by default.

@michaelkuhn: can you comment on this PR?

This keeps the existing behavior, that is, spack load spec only loads spec and spack load -r spec loads spec and all of its dependencies.

I agree that autoloading (at least) the run dependencies by default makes sense. I have been using autoload: 'all' for quite some time for our installation since users kept forgetting to use -r. Maybe a new autoload option would make sense for this?

The issue where we discussed this was #8639.

Is this PR a decent place to fix that or is this something you want to save for later?

If it is fine with you I'd save this for a later (and smaller) PR. In any case I plan to continue working on module issues in the next couple of weeks.

doesn't this and the rest of d4b4be4 get rid of the loading of dependencies?

No, this keeps the current behavior, except that it prevents spack load from loading multiple versions of the same package (see #6676).

tgamblin · 2018-07-24T14:40:59Z

I think the only thing this needs is more tests!

alalazo · 2018-07-24T14:51:50Z

I think the only thing this needs is more tests!

Do you have anything in particular in mind? I added regressions for #2215, #2570 and #4400

tgamblin · 2018-07-24T18:33:14Z

It looks like spack module loads is not tested, so there should really be a test case for that. I think that can be a separate PR though.

The commands have been deprecated in spack#7098, and have been failing with an error message since then. Cleaning the code since it is unlikely that somebody is still using them.

The commands have been deprecated in #7098, and have been failing with an error message since then. Cleaning the code since it is unlikely that somebody is still using them.

alalazo added the WIP label Jan 28, 2018

alalazo force-pushed the features/separate_module_commands branch from 7c5d756 to b8ab3e6 Compare January 28, 2018 18:44

alalazo force-pushed the features/separate_module_commands branch from b8ab3e6 to 2fde417 Compare February 13, 2018 14:23

alalazo force-pushed the features/separate_module_commands branch from 2fde417 to 54cdb8a Compare March 19, 2018 09:05

adamjstewart added modules commands labels Mar 21, 2018

alalazo force-pushed the features/separate_module_commands branch from 54cdb8a to 4b791ef Compare April 1, 2018 08:36

alalazo mentioned this pull request Apr 8, 2018

Multiple module categories #3380

Closed

alalazo force-pushed the features/separate_module_commands branch from 4b791ef to 623acef Compare April 11, 2018 20:53

alalazo force-pushed the features/separate_module_commands branch from 2e349a5 to 87ccb12 Compare April 20, 2018 07:35

alalazo added ready and removed WIP labels Apr 20, 2018

alalazo force-pushed the features/separate_module_commands branch from ffd13eb to ade16fc Compare May 21, 2018 09:49

alalazo requested review from becker33 and tgamblin May 21, 2018 10:29

alalazo force-pushed the features/separate_module_commands branch from ade16fc to 07629b5 Compare July 5, 2018 12:22

adamjstewart mentioned this pull request Jul 5, 2018

module refresh will not generate lmod files despite config #8646

Closed

tgamblin self-assigned this Jul 6, 2018

alalazo force-pushed the features/separate_module_commands branch 4 times, most recently from e8b7a6d to c011b96 Compare July 10, 2018 07:04

alalazo removed the ready label Jul 10, 2018

alalazo force-pushed the features/separate_module_commands branch from b3c8685 to c90c7e6 Compare July 10, 2018 15:39

ax3l approved these changes Jul 11, 2018

View reviewed changes

alalazo added 11 commits July 23, 2018 07:48

Added 'setdefault' subcommand to 'spack lmod'

168d42b

Added a subcommand that sets the default module to be loaded, if multiple installations of the same package are present.

Updated references to spack module in the documentation.

40525a5

Added a unit test for 'spack lmod setdefault'

2cc26ce

Renamed 'patch_configuration' fixture to 'module_configuration'

352038c

This just because the fixture has been moved one level above the one it was originally defined. In this more general context there's more than one configuration file that could be patched for tests.

Revert "Fix recursive module loading."

d4b4be4

This reverts commit 732c24f.

Added --dependencies to the help text of spack load

f573165

fixes spack#6321

module file tutorial: rebuilt docker image and updated the script

36791d2

Grouped all the module commands under spack module

d1f72c4

As requested in the review all the commands meant to manage module files have been grouped under the `spack module` command. Unit tests have been refactored to match the new command structure.

alalazo force-pushed the features/separate_module_commands branch from bb7a505 to a9886b7 Compare July 23, 2018 06:13

This comment has been minimized.

Sign in to view

alalazo removed the don't-merge-yet label Jul 23, 2018

alalazo added 2 commits July 23, 2018 13:46

Die with a meaningful error if a deprecated command is used

0c4ed2a

In case a deprecated form of the module command is used, the program will exit non-zero and print an informative error message suggesting which command should be used instead.

The auto-completion file is now consistent with spack module

4d5a985

alalazo force-pushed the features/separate_module_commands branch from a9886b7 to 4d5a985 Compare July 23, 2018 11:46

tgamblin requested changes Jul 24, 2018

View reviewed changes

tgamblin approved these changes Jul 24, 2018

View reviewed changes

tgamblin merged commit 5b8b7a5 into spack:develop Jul 24, 2018

alalazo deleted the features/separate_module_commands branch July 25, 2018 08:24

alalazo mentioned this pull request Aug 13, 2021

Remove handling for deprecated module commands #25411

Merged

Conversation

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

Uh oh!

Summary

List of modifications

Example: specify a default module to be loaded for lmod

Uh oh!

alalazo commented Jul 5, 2018

Uh oh!

adamjstewart commented Jul 5, 2018

Uh oh!

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

Uh oh!

Uh oh!

ax3l left a comment

Choose a reason for hiding this comment

Uh oh!

healther commented Jul 12, 2018

Uh oh!

healther commented Jul 12, 2018

Uh oh!

This comment has been minimized.

alalazo commented Jul 23, 2018

Uh oh!

tgamblin commented Jul 23, 2018

Uh oh!

alalazo commented Jul 23, 2018

Uh oh!

tgamblin left a comment • 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!

tgamblin Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

michaelkuhn Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

adamjstewart Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

alalazo Jul 24, 2018 • 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!

alalazo Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Jul 24, 2018

Uh oh!

alalazo commented Jul 24, 2018

Uh oh!

tgamblin commented Jul 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alalazo commented Jan 28, 2018 •

edited

Loading

alalazo commented Jul 10, 2018 •

edited

Loading

tgamblin left a comment •

edited

Loading

alalazo Jul 24, 2018 •

edited

Loading