setup_dependent_environment overwritten by module-defined external compiler

[mgsternberg]

Problem

In Spack, a compiler definition that uses a module: token can interfere with or nullify any environment variable defined by setup_dependent_environment() or setup_dependent_package() of a package upstream in the DAG.

Question

How does one get around this, with the fewest assumptions and least a priori knowledge?

• Can one defer calling the setup_dependent_* hooks until after the compiler is loaded?

  If I'm not mistaken, that's the canonical way of doing things outside of Spack. I can see why Spack would load compilers last, perhaps to largely (though not entirely) guarantee a consistent compiler environment?

• Blacklist "known" variables for module load in the specific situation of loading a compiler module? – I think that could only cure specific symptoms.

• Have the user modify the module that defines the compiler? – Ditto for symptoms-only, and I'd say that's impractical for many users.

  On my own cluster, that's what I had done, if unrelated to Spack: I create modulefiles for intel-parallel-studio by a script from psxevars.sh but excised any MPI variables and paths and spirits those away into a separate impi package. This avoided a hit from the main example that follows:

Main Example: I_MPI_ROOT

The present issue crystallized from the root cause for the failure in issue #8410, which concerns $I_MPI_ROOT, defined by both intel-parallel-studio (any edition) and intel-mpi.

Consider the scenario where a user runs:

spack install clientpkg %intel ^intel-mpi

and where the user has in compilers.yaml:

- compiler:
    …
    spec:       intel@18.0.3
    # Module provided externally or perhaps even internally by Spack:
    modules:    [intel/18]
    paths:
        …

Then, there are two failure modes:

Failure mode 1: MPI Runtime vs. MPI Development version

When intel-parallel-studio is @composer.* or @professional.*, it includes an MPI runtime, and therefore sets I_MPI_ROOT. This value supersedes any specific value or the internal default if unset for any prerequisite ^intel-mpi version.

When compiling a client package, mpicc etc. from intel-mpi will error out due to missing static libs only found under $I_MPI_ROOT of intel-mpi, but not that of intel-parallel-studio. Sadly, that information is buried in config.log. The user sees the rather more dramatic message:

configure: error: C compiler cannot create executables

Failure mode 2: Silent version overshadowing

When intel-parallel-studio is @cluster.*, a version mismatch occurs for MPI components: In the eyes of the user and Spack, the client package is compiled with intel-mpi@foo, whereas mpicc (which is from intel-mpi@foo) will take its libs etc. from intel-parallel-studio.

Disconcertingly, this will occur silently.

Now, a user would not typically need or use ^intel-mpi when one has %intel-parallel-studio@cluster.*, but Spack will let you do it. It may not be clear to a new user that it's typically not needed, and it's bad for advanced users who specifically choose a different MPI version.

Other affected packages: MKL, …

The same is true for the other intel-foo library-style packages, of which intel-mkl is most critical.

The MKL is provided by all studio versions, so failure mode 1. does not occur, but failure mode 2. might, especially when the user has no access to newer compilers but does for the MKL, which has been available under a "simplified" (no-cost) license since 2017.

Variable tracing

Here's a more detailed tracing of the MPI variables for the case discussed in #8410 :

spack --debug -v -v install -v -v hdf5@1.10.2+mpi ^intel-mpi

Gives (formatted for readability):

==> Installing hdf5

==> setup_package.setup_dependent_package:
    spec 'intel-mpi@2018. ...' received .mpi* properties:
    {
	'MPICC':   '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiicc',
	'MPICXX':  '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiicpc',
	'MPIF77':  '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiifort',
	'MPIF90':  '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiifort'

	'MPIFC':   '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiifort',
    }
...
==> setup_dependent_environment._setup_dependent_env_callback.mpi_setup_dependent_environment:
    adding to spack_env:
    {
	'MPICC':      '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiicc',
	'MPICXX':     '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiicpc',
	'MPIF77':     '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiifort',
	'MPIF90':     '.../intel-mpi/2018.3.222-intel-18.0.3/.../intel64/bin/mpiifort',

	'I_MPI_CC':   '.../lib/spack/env/intel/icc',
	'I_MPI_CXX':  '.../lib/spack/env/intel/icpc',
	'I_MPI_F77':  '.../lib/spack/env/intel/ifort'
	'I_MPI_F90':  '.../lib/spack/env/intel/ifort',
	'I_MPI_FC':   '.../lib/spack/env/intel/ifort',
	'I_MPI_ROOT': '.../.../intel-mpi/2018.3.222-intel-18.0.3/Hipwooo/compilers_and_libraries_2018.3.222/linux/mpi',
    }


==> '/usr/bin/modulecmd' 'python' 'load' 'intel-parallel-studio/composer.2018.3/intel-18.0.3/Hnd7rbb'
...

.. and fails with:

  >> 33    configure: error: in `/tmp/stern/spack-stage/spack-stage-MQP6Az/hdf5-1.10.2':
  >> 34    configure: error: C compiler cannot create executables

$ module show intel-parallel-studio/composer.2018.3/intel-18.0.3/Hnd7rbb | sed 's,[ \t:]\+/,\n\t/,g'

(formatted for readability):

• MPI:

  prepend-path	LD_LIBRARY_PATH
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mpi/intel64/lib
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mpi/mic/lib
  prepend-path	CLASSPATH
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mpi/intel64/lib/mpi.jar
  setenv		I_MPI_ROOT
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mpi
  prepend-path	MANPATH
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mpi/man
  prepend-path	PATH
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mpi/intel64/bin
  

• MKL:

  prepend-path	CPATH
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mkl/include
  prepend-path	LD_LIBRARY_PATH
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mkl/lib/intel64_lin
  prepend-path	LIBRARY_PATH
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mkl/lib/intel64_lin
  setenv		MKLROOT
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mkl
  prepend-path	NLSPATH
    .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mkl/lib/intel64_lin/locale/%l_%t/%N
  prepend-path	PKG_CONFIG_PATH
      .../intel-parallel-studio/composer.2018.3-intel-18.0.3/.../mkl/bin/pkgconfig
  

• likewise for DAAL, IPP, etc.

Location where I_MPI_ROOT is used

This occurs right on top of mpiicc :

$ grep -C1 I_MPI  /home/SHARE/soft/spack/spack-stern/opt/spack/linux-centos6-x86_64/\
intel-mpi/2018.3.222-intel-18.0.3/Hipwooo/compilers_and_libraries_2018.3.222/linux/mpi/intel64/bin/mpiicc 

namely,

prefix=.../intel-mpi/2018.3.222-intel-18.0.3/Hipwooo/compilers_and_libraries_2018.3.222/linux/mpi
# The environment variable I_MPI_ROOT may be used to override installation folder path
if [ -n "$I_MPI_ROOT" ] ; then
    prefix=$I_MPI_ROOT;
fi

The lib missing in #8410 is specifically mentioned as well; it is available as static lib only, and is linked explicitly even for shared linkage:

...
    I_MPI_OTHERLIBS=" $libdir/libmpigi.a"
...
    I_MPI_OTHERLIBS=" -lmpigi"
...

[scheibelp]

Can one defer calling the setup_dependent_* hooks until after the compiler is loaded?

At the moment the order is hard-coded (to load compiler modules after setup_dependent_environment). Do you think that the opposite order would be universally better? Or would you prefer a way to customize the order?

[mgsternberg]

@scheibelp

At the moment the order is hard-coded […]. Do you think that the opposite order would be universally better?

As I wrote:

[loading compilers first is] the canonical way of doing things outside of Spack. I can see why Spack would load compilers last, perhaps to largely (though not entirely) guarantee a consistent compiler environment?

The current Spack strategy will backfire for any omnibus development package that provides more than just a compiler. Today, perhaps only Parallel Studio fits that bill, but tomorrow other packages may come along that want to be all things to all people. With compilers loaded last (and in the form of modules), all such omnibus packages will silently overshadow alternative sub-packages that a user may legitimately want, as shown for Intel-MPI and MKL in the intro.

I am aware that changing the order touches on perhaps deep architectural choices made for Spack that I don't have insight on. If the default order is not changed, a configurable order provides at least a way out, but it puts the burden on the user and it could may make issue replications tricky.

[scheibelp]

I am aware that changing the order touches on perhaps deep architectural choices made for Spack

The order of compiler vs. module loads is not particularly difficult to change and I don't think there would be too much controversy around it. As an example, #8245 (which was closed in favor of #8346) changed the order.

@alalazo do you see a major reason to maintain the current order? I know in #8245 you mention the line

All module loads that otherwise would belong in previous functions have to occur after the spack_env object has its modifications applied.

but I think that was primarily because the environment cleaning actions like unsetting LD_LIBRARY_PATH are currently bundled with the dependency environment modifications.

[alalazo]

@mgsternberg @scheibelp The problem:

In Spack, a compiler definition that uses a module: token can interfere with or nullify any environment variable defined by setup_dependent_environment() or setup_dependent_package() of a package upstream in the DAG

seems to be a generalization of #8346 (there we were only interested in CC, etc.). I think we could probably solve this issue with an extension of #8346 by:

1. Keeping track of which variables will be modified by spack_env
2. Passing that list to the preserve_environment context manager

Would this solution work for you? Do you see major drawbacks?

[mgsternberg]

@alalazo

I think we could probably solve this issue with an extension of #8346

That's what I described by:

Blacklist "known" variables for module load in the specific situation of loading a compiler module? – I think that could only cure specific symptoms.

… and it certainly won't tackle the problem "with the fewest assumptions and least a priori knowledge".

The relevant env. var names include I_MPI_ROOT and MKLROOT. While #8346 reasonably hard-codes CC & friends, the vars here are far more specialized and cover only today's needs. Therefore, a list of additional variables must be user-configurable and presumably will have to live in the compilers.yaml preferences(*), each in a sister leaf of any offending modules: tag. As tag name, preserve_environment: would suggest itself. Don't use "blacklist" because that term is already used twice elsewhere in Spack in similar but different contexts and therefore could be confusing.

(*) For goodness' sake, don't include that list in the hash definition of the compiler spec.

[alalazo]

@mgsternberg

The relevant env. var names include I_MPI_ROOT and MKLROOT. While #8346 reasonably hard-codes CC & friends, the vars here are far more specialized and cover only today's needs. Therefore, a list of additional variables must be user-configurable and presumably will have to live in the compilers.yaml preferences(*), each in a sister leaf of any offending modules: tag. As tag name, preserve_environment: would suggest itself. Don't use "blacklist" because that term is already used twice elsewhere in Spack in similar but different contexts and therefore could be confusing.

I think you misunderstood my proposal (or I was not clear exposing it). What I want to do is to use the knowledge already in spack_env to prevent modifications to all the variables modified explicitly by a package in the DAG. In any case the modifications I had in mind were just a few lines, so see #8688

[mgsternberg]

Sorry, yes, I missed the core idea. I commented at #8688, which I fear is going too far.

What about the idea of deferring the setup_dependent_* hooks until after the compiler is loaded, i.e., switching the current sequence? I figured this would touch on architectural choices and thus have ramifications in kind, but @scheibelp mentioned that this would not be out of the question.

[alalazo]

What about the idea of deferring the setup_dependent_* hooks until after the compiler is loaded, i.e., switching the current sequence? I figured this would touch on architectural choices and thus have ramifications in kind, but @scheibelp mentioned that this would not be out of the question.

I am not opposed to it, though it may come with its own issues (an incompatibility like I_MPI_ROOT might also go the other way around, i.e. being overridden by packages while it should stay as it was set by compiler modules). In that case I would probably proceed like @scheibelp did in #8245, but with a small modification in the order:

1. Clear the environment variables first (and emit a warning or at least a debug message if any is found?).
2. Load the modules for compilers. At this stage we should have a working compiler.
3. Construct and apply all the other modifications.

Thinking about it: we already have a factory for environment modifications called EnvironmentModifications.from_sourcing_file that returns the list of modifications to the environment you would have by sourcing a file. Would it be worth adding something like EnvironmentModifications.from_loading_modules?

The added value is that if all the modifications are in the same object they can be analyzed together. It won't magically solve the issue, but at least in your case you would have seen a message like:

==> Warning: Suspicious requests to set or unset 'CC' found
==> Warning: 	    	env.set('CC', join_path(link_dir, compiler.link_paths['cc'])) at /git/spack/lib/spack/spack/build_environment.py:146
==> Warning: 	--->	spack_env.set('CC', spack_cc) at /git/spack/var/spack/repos/builtin/packages/gdbm/package.py:49

pointing to a clear incompatibility among the two requests.

[mgsternberg]

@alalazo

proceed like @scheibelp did in #8245, but with a small modification in the order:

1. Clear the environment variables first (and emit a warning or at least a debug message if any is found?).
2. Load the modules for compilers. At this stage we should have a working compiler.
3. Construct and apply all the other modifications.

If I understand correctly, the application of env. modifications in #8245 is already in the right order, done after the compiler setup at the very last step of setup_package() as spack_env.apply_modifications(), and your suggestion is deferred construction so you could warn if a compiler setting is being touched. Thoughts on this:

• For the first step, the term "clear" is misleading since it suggests what env -i does. Rather, "sanitize" in the function name would be more apt.

• For the problem on hand, I'd say a supervised load_module() via a .from_loading_modules() as you suggest is not needed for a compiler module if you do load it first. I don't know about other occasions it might apply (external DAG packages?). I'd opine to keep things simple and not insert a layer into load_module().

• In the construction of env. mods., deferred or otherwise, the setup_dependent_* hooks in the various package codes can only act on EnvironmentModifications objects spack_env and run_env. Thus, detection of overshadowing compiler-controlled vars would have to go into EnvironmentModifications.{append,extend} or perhaps an execute callback directly in NameValueModifier, rather than into a supervised load_module().

  But I have reservations about trying to issue warnings as such:

  • You'd still need a priori knowledge to decide which vars to warn about and which not, to keep the noise down, wouldn't you?
  • Moreover, compilers can be affected by env. variables that have not been mentioned in the compiler modulefile or elsewhere. Spack cannot possibly know them all, and I'd say it should not try to interject.

I'd say #8245 needs to be reconsidered.