Uniquify suffixes added to module names

[sethrj]

Consider a user that wants to mark CUDA-enabled modules with a -cuda suffix. To also account for libraries that use cuda-enabled MPI (perhaps to differentiate them from the same library that uses non-cuda MPI), they set their modules.yaml file to:

modules:
  enable::
    - lmod
  lmod:
    all:
      suffixes:
        '+cuda': 'cuda'
        '^mpi+cuda': 'cuda'

Although this creates non-conflicting module names for foo ^mpi+cuda and foo ^mpi~cuda, it gives bar+cuda ^mpi+cuda an suffix of -cuda-cuda.

This patch deduplicates module extensions so that the latter suffix will simplify to -cuda. It also sorts the suffixes to improve robustness.

[alalazo]

It also sorts the suffixes to improve robustness.

Right now suffixes are added in the order in which they are written. This PR change that behavior. I don't mind this behavior as long as it is documented, but are we sure that there won't be user requests to have mpi-cuda instead of cuda-mpi or similar things? Asking because my experience with modules is that people are very tied to their own way of naming things...

Although this creates non-conflicting module names for foo ^mpi+cuda and foo ^mpi~cuda, it gives bar+cuda ^mpi+cuda an suffix of -cuda-cuda

I see that the behavior of +foo in spec changed in 515b404. Previously it was not necessary to have ^mpi+cuda. I agree with deduplication, not sure about sorting. In case we have a function that performs stable deduplication in llnl.util.lang.dedupe.

[sethrj]

@alalazo Great, I wasn't aware of the stable deduplication, and I wasn't aware any existing ordering was documented. I'm used to assuming that .items() returns an arbitrarily ordered sequence, but I am aware that many containers in Spack use YAML-constructed ordered dictionaries.

I will happily change sorted(set(suffixes)) to llnl.util.lang.dedupe(suffixes) if it'll make this PR less contentious.

[sethrj]

@alalazo Judging by the Python 2 test failures after reverting the sort, the ordering of self.conf.get('suffixes') is not based on the order in modules.yaml, so different python versions generate different module names (yikes!). I'm going to revert that change so that suffixes will have a stable ordering from now on.

[alalazo]

Judging by the Python 2 test failures after reverting the sort, the ordering of self.conf.get('suffixes') is not based on the order in modules.yaml, so different python versions generate different module names (yikes!)

That must have changed at some point - shame for the lack of proper testing 😢 Well, giving thumbs up on this then, but waiting for somebody else to chime in before merging.

[sethrj]

@alalazo It's been a week since the feedback request to #modules, so I believe there are no barriers to merging.

[alalazo]

@sethrj Thanks for the reminder!

[sethrj]

Thanks for the review and merge!

[RemiLacroix-IDRIS]

Right now suffixes are added in the order in which they are written. This PR change that behavior. I don't mind this behavior as long as it is documented, but are we sure that there won't be user requests to have mpi-cuda instead of cuda-mpi or similar things? Asking because my experience with modules is that people are very tied to their own way of naming things...

I am just noticing that the ordering changed now that the change is causing some issues with our modules. :(

foo/X.Y.Z-cuda-mpi becomes foo/X.Y.Z--cuda-mpi for example, which we aren't really happy about because it is causing duplicated modules if we don't pay attention and we cannot have consistent module naming without breaking existing submission scripts.

Additionally, the documentation was not updated as far as I can tell: https://spack.readthedocs.io/en/latest/module_file_support.html#selection-by-anonymous-specs

Order does matter
The modifications associated with the all keyword are always evaluated first, no matter where they appear in the configuration file. All the other spec constraints are instead evaluated top to bottom.

Edit: I am willing to contribute a PR that would make everybody happy (if that's possible).

[alalazo]

Additionally, the documentation was not updated as far as I can tell:

Maybe we can word it better, but that sentence is meant to say that if you have:

modules:
  tcl:
    foo:
      kwd: ...
    all:
      kwd: ...
    bar:
      kwd: ...

The order in which kwd is constructed for foo is to evaluate all first and merge it with foo.

[RemiLacroix-IDRIS]

@alalazo I would think it makes sense that "modifications" also applies to suffixes. In practice it was the case before.