Env mod 4

[mgsternberg]

Introduction

This PR provides fixes for Spack to function with Environment Modules 4.x and to address minor coding issues.

I factored out the main fix addressed here from #7469. As mentioned in that PR in general, the issue is fundamental and would render Spack a non-starter for new users on a similar platform.

Issues and Fixes

• Spack threw UnboundLocalError when using Environment Modules 4.x.

  • Solution: Add a continue statement in lib/spack/spack/environment.py:from_sourcing_file().

  This is the absolute minimum needed to accommodate Env-Mod-4.x. It turns out that a changed content of BASH_FUNC_module() triggered this problem (see next point).

• BASH_FUNC_module() and ENV must be blacklisted to not show up in generated modulefiles.

  • Root cause: Env-Mod-4 initializes module() differently, depending on the tty being interactive or not, or more specifically, where stderr goes (see reason in code):

  $ env - bash -l -c 'env' | grep 'module()'
  BASH_FUNC_module()=() {  _moduleraw $* 2>&1
  $ env - bash -l -c 'env' 2> /dev/null | grep 'module()'
  BASH_FUNC_module()=() {  if [ "$MODULES_SILENT_SHELL_DEBUG" = '1' ]; then

  and:

  $ ( env;  env - bash -l -c 'env' ) | grep ^ENV
  ENV=/home/username/.bashrc
  ENV=/usr/share/Modules/init/profile.sh

  Blacklisting BASH_FUNC_module() happens to sidestep the initial problem, but the latter should still be fixed to address the uninitialized variable issue.

• A TypeError/NoneType was thrown if blacklist was in the YAML config but had all entries commented out (as happened during debugging), or equivalently has no entries.

  • Solution: Extend a relevant if clause.

Background

The Environment Modules software project recently saw some major changes when Xavier Delaruelle xavier.delaruelle@cea.fr took on maintanence. He froze the former C-based implementation at 3.x, since it had become difficult to maintain and extend, and instead made the formerly alternative Tcl implemention the primary engine. Like Modules-C, the Modules-Tcl goes back quite some time and was mature but nonetheless new and interesting features are available with 4.x.

Pre-release versions of Environment Modules-4, like Modules-Tcl previously, did not work at all with Spack for their lack of a modulecmd script/executable. The 4.x release version conveniently added such a script, which Spack can latch on to. For end users, more involved interfaces are available in a module function for Bash and numerous scripting languages, including Python. Those front-end functions support the concept of quarantining variables, in particular LD_LIBRARY_PATH, which is needed to ensure that any user-side environment modifications do not interfere with running the Tcl interpreter for the Modules engine itself. Since Spack itself isolates module commands, however, this quarantine may not strictly be needed.

Development note

While implementing the above, I got the impression that lib/spack/spack/environment.py:from_sourcing_file() looks somewhat, shall we say, "organically grown", which is concerning given the complexity of its task. That function could benefit from being split into two tasks:

1. sourcing a file and reading the resulting environment, and
2. comparing two environments stored as dict objects.

Factoring out task 2 should make it rather more easy to test. It could be done here or in a separate PR. Of interest might be createmodule.py (it is under GPLv2). It uses a slightly different algorithm, jumping off with the rather nifty construct (prepend,append) = env2[key].split(env1[key]). There is also a Bash-based version which, remarkably, is shorter.

[mgsternberg]

Oh my – having factored out this PR from #7469, I realized there that blacklist in lib/spack/spack/environment.py:from_sourcing_file() is wholly unrelated to the one or more module-flavor-specific environment_blacklist entries in modules.yaml 🤦‍♂️.

This decouples the issues above from one another, but doesn't change their impact, in particular for the primary one.

[mgsternberg]

I rolled back the change to lib/spack/spack/util/module_cmd.py as it won't be relevant anymore once #8570 is merged.

The other changes are still very much needed to fix bugs in environment_modifications() and from_sourcing_file(), as well as to accommodate Environment Modules 4.x as described in the Intro.

[alalazo]

Basically LGTM. Can you add some tests that stress the lines you changed or added?

[alalazo]

If the name seems confusing, what do you think of renaming blacklist and whitelist to something that conveys better their role in this function and remove the comment above?

On a side not: I also slightly prefer to construct a list explicitly rather than splitting a string, given that we have items in it like _.

[xdelaruelle]

@mgsternberg @alalazo it would be great to have this PR merged to enable the use of new Modules releases (4+). What can be done to help ?

[alalazo]

@xdelaruelle As far as I am concerned a rebase + unit tests + reply to the few minor comments above would do.

[alalazo]

Looking at the issues reported here. Concerning:

A TypeError/NoneType was thrown if blacklist was in the YAML config but had all entries commented out (as happened during debugging), or equivalently has no entries.

This is what Spack at 617c1a3 reports:

$ spack config blame modules
==> Error: /home/mculpo/PycharmProjects/spack/etc/spack/modules.yaml:3: None is not of type 'array'

so it doesn't seem to be an issue anymore.

[alalazo]

Spack threw UnboundLocalError when using Environment Modules 4.x.

BASH_FUNC_module() and ENV must be blacklisted to not show up in generated modulefiles.

I also can't reproduce the two bugs above using 617c1a3 with the intel module and environment-modules@4.2.2

[mgsternberg]

@alalazo wrote:

A TypeError/NoneType was thrown if blacklist was in the YAML config but had all entries commented out (as happened during debugging), or equivalently has no entries.

This is what Spack at 617c1a3 reports:

$ spack config blame modules
==> Error: /home/mculpo/PycharmProjects/spack/etc/spack/modules.yaml:3: None is not of type 'array'

so it doesn't seem to be an issue anymore.

It's not a showstopper anymore but the error message is terrible. It uses Python lingo for a YAML file and therefore looks rather like a bug.

Example

The following is valid YAML (so says yamllint 1.15) and Spack clearly parses the syntax just fine but it stumbles on its own schema:

modules:
  tcl:
    all:
      filter:
        environment_blacklist:
          # foo

$ spack config blame modules
==> Error: /home/stern/.spack/modules.yaml:5: None is not of type 'array'

That error message would be far better for the user if it expressed that an empty YAML sequence (array) is disallowed by the Spack schema.

But, not only does that seem overly harsh, you can't intuitively comment out the key of the sequence, because the trouble applies to the parent YAML mapping (dict) as well:

modules:
  tcl:
    all:
      filter:
#        environment_blacklist:
#          # foo

$ spack config blame modules
==> Error: /home/stern/.spack/modules.yaml:4: None is not of type 'object'

Moreover, the trouble recursively climbs up the hierarchy:

modules:
#  tcl:
#    all:
#      filter:
#        environment_blacklist:
#          # foo

$ spack config blame modules
==> Error: /home/stern/.spack/modules.yaml:1: None is not of type 'object'

Suggestion

Permit (tolerate) empty YAML sequences and mappings in Spack config files.

Is this something worthy of discussing further?