Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow user-local modules with hierarchical naming schemes #1472

Merged
merged 25 commits into from
Jan 20, 2016

Conversation

geimer
Copy link
Contributor

@geimer geimer commented Nov 12, 2015

If one uses EB's default MNS, user-local modules are no problem, as a site only needs to 'module use' something like '$HOME/.local/modules' in their modules setup script. However, when using an HMNS, this per-user location needs to be taken into account by the EB-generated modules which extend
$MODULEPATH.

This PR introduces a new option 'subdir-user-modules' (empty by default) which allows to define a location relative to the user's $HOME in which a user-local modules hierarchy (according to the module naming scheme, configurable via det_user_modpath_extensions) is searched. The corresponding modules will then include an additional module use statement to pick up modules from the this location. To prevent errors if the directory does not exist (C modules), this module use is guarded by a check.

@hpcugentbot
Copy link
Contributor

Automatic reply from Jenkins: Can I test this?

@boegel
Copy link
Member

boegel commented Nov 13, 2015

Jenkins: ok to test

@boegel boegel modified the milestone: v2.5.0 Nov 13, 2015
@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2329/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

user_modpath_exts = ActiveMNS().det_user_modpath_extensions(self.cfg)
self.log.debug("Including user module path extensions returned by naming scheme: %s"
% user_modpath_exts)
quoted_user_modpath_exts = [quote_str(os.path.join(user_modpath, ext)) for ext in user_modpath_exts]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should be quoting in module_generator.use instead, if it's so important to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think quoting only gets important if the path contains some crazy characters that are evaluated by Tcl/Lua. That is, the quotes are more a "safety net" rather than required. AFAIR, I did the quoting outside of module_generator.use to allow both prefix and path to be expressions that are evaluated when loading the modulefile. But I agree that this may be over-engineered.

@boegel
Copy link
Member

boegel commented Dec 14, 2015

@geimer: sorry it took me so long to give this a proper review, you should have pinged me earlier

There's a couple of loose ends here, so too late to make it in for EB v2.5.0 I'm afraid, so I'm going to put 2.6.0 as milestone on this...

Next to the remarks inline:

  • a test should be added for the --subdir-user-modules option somewhere, to verify that it's actually picked up
  • a test should be added for module_generator.det_home, or actually for get_env (see other remarks)

Other than that, this looks really reasonable to me, you have to stop pretending to not know Python. ;-)

Let me know if you need any help in tackling the remarks (or just throw your hands up and let me take over, that's fine by me).

@boegel boegel modified the milestones: v2.5.0, ASAP Dec 14, 2015
@geimer
Copy link
Contributor Author

geimer commented Jan 18, 2016

@boegel: OK, I believe the culprit is in ModulesTool.modpath_extensions_for... Hard-coded regex to search for module use... I don't think this ever worked for Lua syntax... Shouldn't the regrex be better queried from module_generator? Also, the regex has to become more complicated to account for prefix, which can also be an expression. Urgh...

Seems like this is getting more and more unrelated to the original topic of this PR, just because it triggers already existing bugs in other areas of the code... What do you think?

@boegel
Copy link
Member

boegel commented Jan 18, 2016

@geimer: it sounds like you're hitting the problem that @t-brown was working on during the hackathon in Austin, cfr. PR #1474

@geimer
Copy link
Contributor Author

geimer commented Jan 18, 2016

@boegel: Well, not really. I just noticed as well that the Lua syntax was missing here.

The more difficult issue is that with user-local modules, the argument given to module use may be an expression in Lua/Tcl syntax that is evaluated at module load time. My current understanding of the code is that it assumes absolute paths in plain text, that it can then pass to path_matches, which implicitly calls os.path.exists and os.path.samefile. This won't work for the Lua/Tcl expression...

@boegel
Copy link
Member

boegel commented Jan 18, 2016

@geimer: it sounds like we'll need to rework that to parse the output of module use rather than parsing the module file themselves?

@geimer
Copy link
Contributor Author

geimer commented Jan 18, 2016

@boegel: Something like this, yes. But since module use doesn't output anything, you probably need to load the module, see whether it extends $MODULEPATH, and unload it again if not. But I'm still trying to figure out what exactly modpath_extensions_for and path_to_top_of_module_tree are doing...

@boegel
Copy link
Member

boegel commented Jan 18, 2016

@geimer: I meant to say module show rather than module use; the output of module show should always include the modifications to $MODULEPATH (unless the directory is not there in $HOME, in which case we don't care, I think)

@geimer
Copy link
Contributor Author

geimer commented Jan 18, 2016

@boegel: Looks like module show doesn't seem to work for Tcl modules... :-(

@boegel
Copy link
Member

boegel commented Jan 18, 2016

@geimer In what sense, exactly? And do you mean with module files written in Tcl, or using modulecmd.tcl?

@geimer
Copy link
Contributor Author

geimer commented Jan 18, 2016

@boegel: I tried module show on a modulefile including a module use statement using modulecmd.tcl, and the output didn't include anything w.r.t. $MODULEPATH. May be a non-issue, though, as module hierarchies only work reasonably well with Lmod. But still...

@boegel
Copy link
Member

boegel commented Jan 18, 2016

@geimer: it doesn't even include the module use as is?

@boegel
Copy link
Member

boegel commented Jan 18, 2016

Seems like you're right, urgh.

test module:

$ cat ~/.local/easybuild/modules/all/toy/0.0 
#%Module
module use /tmp

'show' outputs shows no clue of use or $MODULEPATH

$ tclsh tcl-modulecmd/tcl/modulecmd.tcl bash show toy/0.0                           
-------------------------------------------------------------------
/Users/kehoste/.local/easybuild/modules/all/toy/0.0:

-------------------------------------------------------------------

'load' does what it should though:

$ tclsh tcl-modulecmd/tcl/modulecmd.tcl bash load toy/0.0
_LMFILES__modshare=/Users/kehoste/.local/easybuild/modules/all/toy/0.0:1; export _LMFILES__modshare;
LOADEDMODULES_modshare=toy/0.0:1; export LOADEDMODULES_modshare;
MODULEPATH_modshare=/tmp:1:/Users/kehoste/.local/easybuild/modules/all:1; export MODULEPATH_modshare;
_LMFILES_=/Users/kehoste/.local/easybuild/modules/all/toy/0.0; export _LMFILES_;
LOADEDMODULES=:toy/0.0; export LOADEDMODULES;
MODULEPATH=/tmp:/Users/kehoste/.local/easybuild/modules/all; export MODULEPATH;

'show' output with Lmod:

$ lmod bash show toy/0.0
LMOD_DEFAULT_MODULEPATH="/Users/kehoste/.local/easybuild/modules/all";
export LMOD_DEFAULT_MODULEPATH;
MODULEPATH="/Users/kehoste/.local/easybuild/modules/all";
export MODULEPATH;
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
   /Users/kehoste/.local/easybuild/modules/all/toy/0.0:
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
prepend_path("MODULEPATH","/tmp")

LMOD_DEFAULT_MODULEPATH="/Users/kehoste/.local/easybuild/modules/all";
export LMOD_DEFAULT_MODULEPATH;
MODULEPATH="/Users/kehoste/.local/easybuild/modules/all";
export MODULEPATH;
_ModuleTable001_="X01vZHVsZVRhYmxlXz17WyJhY3RpdmVTaXplIl09MCxiYXNlTXBhdGhBPXsiL1VzZXJzL2tlaG9zdGUvLmxvY2FsL2Vhc3lidWlsZC9tb2R1bGVzL2FsbCIsfSxbImNfcmVidWlsZFRpbWUiXT1mYWxzZSxbImNfc2hvcnRUaW1lIl09ZmFsc2UsZmFtaWx5PXt9LGluYWN0aXZlPXt9LG1UPXt9LG1wYXRoQT17Ii9Vc2Vycy9rZWhvc3RlLy5sb2NhbC9lYXN5YnVpbGQvbW9kdWxlcy9hbGwiLH0sWyJzeXN0ZW1CYXNlTVBBVEgiXT0iL1VzZXJzL2tlaG9zdGUvLmxvY2FsL2Vhc3lidWlsZC9tb2R1bGVzL2FsbCIsWyJ2ZXJzaW9uIl09Mix9";
export _ModuleTable001_;
_ModuleTable_Sz_="1";
export _ModuleTable_Sz_;

'show' output with Tcl/C env mods:

$ modulecmd bash show toy/0.0
-------------------------------------------------------------------
/Users/kehoste/.local/easybuild/modules/all/toy/0.0:

module       use /tmp 
-------------------------------------------------------------------

@geimer
Copy link
Contributor Author

geimer commented Jan 20, 2016

@boegel: I've adjusted the regex in ModulesTool.modpath_extensions_for to account for quoted module use statements. But this is obviously not the full solution. I suggest to fix the modpath_extensions_for routine and its unit test (which doesn't check Lua module syntax) in a separate PR.

W.r.t. ''.join(...) vs. '\n'.join(...): The latter feels more natural, agreed. But it introduces a number of extra newlines as, for example, conditional_statement already includes a trailing newline. As fixing this is again something that can have wide-spread impacts, addressing this in a separate PR would IMHO be preferred.

Apart from this, I believe all your comments have been addressed. Please take another look.

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2523/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@@ -588,7 +588,7 @@ def modpath_extensions_for(self, mod_names):
modpath_exts = {}
for mod_name in mod_names:
modtxt = self.read_module_file(mod_name)
useregex = re.compile(r"^\s*module\s+use\s+(\S+)", re.M)
useregex = re.compile(r'^\s*module\s+use\s+"?([^" \t\n\r\f\v]+)"?', re.M)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geimer: please use \s instead of \t\n\r\f\v, cfr. https://docs.python.org/2/library/re.html

\s
When the UNICODE flag is not specified, it matches any whitespace character,
this is equivalent to the set [ \t\n\r\f\v]. 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boegel: I wasn't aware that one can use character classes within character classes. But makes sense.

@boegel
Copy link
Member

boegel commented Jan 20, 2016

@geimer: one single small style remark, looks excellent otherwise!

W.r.t. modpath_extensions_for: can you clarify what's left to fix there? Just that it's blissfully unaware of Lua syntax (which is basically what #1474 is about), or is there more to it?

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2528/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member

boegel commented Jan 20, 2016

tests failed due to a temporary issue with GitHub, it seems, let's try again

Jenkins: test this please

@hpcugentbot
Copy link
Contributor

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2529/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@geimer
Copy link
Contributor Author

geimer commented Jan 20, 2016

@boegel: W.r.t. modpath_extensions_for: With the current usage model, I believe things should be fine. However, if at some point in the future someone uses the prefix argument of the use method also for module paths that need to be considered in the modpath_extensions_for routine, things will break (for a potential use case, see below). Thus it's more about being future-proof.

And now the potential use case: Assume that we have a system-wide software installation using EB in <prefix>, using a hierarchical module naming scheme. In the long run, I would like to be able to extend it with my own local software/module collection in $HOME (this is what this PR is good for), but also use EB for installing this stuff (e.g., using --installpath=$HOME/.local). One example might be to install a special debugging version or a version with a specific patch of an MPI library, and then some software on top of it. Then, the module paths in my $HOME and the modpath extensions of modules installed there need to be taken into account in the modpath_extensions_for, right?

I guess a bunch of other things may need to be resolved before the use case above will work with EB. But it is something I would like to see at some point (and I know at least one other guy in our group who already asked whether this would be possible).

@boegel
Copy link
Member

boegel commented Jan 20, 2016

@geimer: OK, I see what you're saying.

The current implementation of modpath_extensions_for only looks for module use "...", so it will miss the module use [ file join ... ] entries we have there for user-local module path extensions.

For your use case to work, we will need a way of dealing with those too.
I'm not sure how we can do that since module show doesn't seem to be an option that always works, but we can figure something out (e.g. push what we get through Tcl first to evaluate it, or whatever, or just look at how $MODULEPATH changes if we fake-load the module).

Can you please open a separate issue on this with largely a copy-paste of your last comment, and a pointer to this PR?

This is good to go btw, thanks a lot for your work and dedicated follow-up on this, much appreciated.

boegel added a commit that referenced this pull request Jan 20, 2016
Allow user-local modules with hierarchical naming schemes
@boegel boegel merged commit 69cfe84 into easybuilders:develop Jan 20, 2016
@geimer geimer deleted the user_modules branch January 25, 2016 17:47
@@ -126,6 +126,18 @@ def det_modpath_extensions(self, ec):
# by default: an empty list of subdirectories to extend $MODULEPATH with
return []

def det_user_modpath_extensions(self, ec):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geimer @boegel This does not seem to be respecting EASYBUILD_SUFFIX_MODULES_PATH (which defaults to all):

conflict("GCCcore")
prepend_path("MODULEPATH", "/usr/local/software/jureca/Stages/Devel-2017a/modules/all/Compiler/GCCcore/5.4.0")
if isDir(pathJoin(os.getenv("HOME"), ".local/EasyBuild/modules/Compiler/GCCcore/5.4.0")) then
    prepend_path("MODULEPATH", pathJoin(os.getenv("HOME"), ".local/EasyBuild/modules/Compiler/GCCcore/5.4.0"))
end

Notice the missing all in the local path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ocaisa what was the full EasyBuild configuration here (eb --show-config)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-bash-4.2$ eb --show-config
#
# Current EasyBuild configuration
# (C: command line argument, D: default value, E: environment variable, F: configuration file)
#
buildpath                      (E) = /dev/shm/ocaisa
experimental                   (E) = True
fixed-installdir-naming-scheme (E) = True
group-writable-installdir      (E) = True
hide-deps                      (E) = ANTLR, APR, APR-util, AT-SPI2-ATK, AT-SPI2-core, ATK, Autoconf, Automake, adwaita-icon-theme, ant, assimp, Bison, babl, binutils, byacc, bzip2, CUSP, Coreutils, cairo, configurable-http-proxy, DB, DBus, DocBook-XML, Dyninst, dbus-glib, damageproto, ETSF_IO, Exiv2, eudev, expat, FFmpeg, FLTK, FTGL, fixesproto, fontsproto, fontconfig, freeglut, freetype, GCCcore, GDAL, GEGL, GL2PS, GLEW, GLib, GLPK, GPC, GObject-Introspection, GTI, GTK+, GTS, Gdk-Pixbuf, Ghostscript, GraphicsMagick, GtkSourceView, g2clib, g2lib, gc, gexiv2, glproto, gperf, guile, grib_api, gsettings-desktop-schemas, gettext, HarfBuzz, icc, ifort, inputproto, intltool, itstool, JUnit, JSON-C, JSON-GLib, JasPer, jhbuild, kbproto, LZO, LibTIFF, LibUUID, Libint, LittleCMS, libGLU, libICE, libSM, libX11, libXau, libXaw, libXcursor, libXdamage, libXdmcp, libXext, libXfixes, libXfont, libXft, libXi, libXinerama, libXmu, libXpm, libXrandr, libXrender, libXt, libXtst, libcerf, libcroco, libctl, libdap, libdrm, libdwarf, libelf, libepoxy, libevent, libffi, libfontenc, libgd, libgeotiff, libglade, libidn, libjpeg-turbo, libmatheval, libmypaint, libpng, libpciaccess, libpthread-stubs, libreadline, librsvg, libtool, libunistring, libunwind, libyaml, libxcb, libxkbcommon, libxml2, libxslt, libyuv, M4, Mesa, makedepend, motif, msgpack-c, NASM, ncurses, nettle, nodejs, nvenc_sdk, nvidia, OPARI2, OTF2, PCRE, PDT, PROJ, Pango, Pmw, PnMPI, PyCairo, PyGObject, Python-Xpra, pixman, pkg-config, pkgconfig, popt, pscom, Qhull, Qt, Qt5, qrupdate, randrproto, recordproto, renderproto, S-Lang, SCons, SIP, SQLite, SWIG, Serf, Szip, scrollkeeper, Tcl, Tk, texinfo, UDUNITS, util-linux, vpx, wxPropertyGrid, wxWidgets, XML-Parser, XZ, XKeyboardConfig, x264, x265, xbitmaps, xcb-proto, xcb-util, xcb-util-image, xcb-util-keysyms, xcb-util-renderutil, xcb-util-wm, xextproto, xineramaproto, xorg-macros, xprop, xproto, xtrans, YAXT, Yasm, zlib
hide-toolchains                (E) = GCCcore
include-easyblocks             (E) = /work/zam/swmanage/EasyBuild/Custom_EasyBlocks/Devel-2017a/*.py, /work/zam/swmanage/EasyBuild/Custom_EasyBlocks/Devel-2017a/generic/*.py
include-module-naming-schemes  (E) = /work/zam/swmanage/EasyBuild/Custom_MNS/Devel-2017a/*.py
include-toolchains             (E) = /work/zam/swmanage/EasyBuild/Custom_Toolchains/Devel-2017a/*.py
installpath                    (E) = /usr/local/software/jureca/Stages/Devel-2017a
job-backend-config             (E) = /usr/local/software/FZJ/gc3pie.cfg
job-cores                      (E) = 48
job-max-walltime               (E) = 1
minimal-toolchains             (E) = True
module-naming-scheme           (E) = CustomHierarchicalMNS
packagepath                    (E) = /usr/local/software/jureca/Stages/Devel-2017a/packages
prefix                         (E) = /usr/local/software/jureca/Stages/Devel-2017a
repositorypath                 (E) = /usr/local/software/jureca/Stages/Devel-2017a/eb_repo
robot                          (E) = /work/zam/swmanage/EasyBuild/Golden_Repo/Devel-2017a, /usr/local/software/jureca/Stages/Devel-2017a/eb_repo, /work/zam/swmanage/EasyBuild/Golden_Repo/Devel-2017a
robot-paths                    (E) = /work/zam/swmanage/EasyBuild/Golden_Repo/Devel-2017a, /usr/local/software/jureca/Stages/Devel-2017a/eb_repo, /work/zam/swmanage/EasyBuild/Golden_Repo/Devel-2017a
set-gid-bit                    (E) = True
sourcepath                     (E) = /work/zam/swmanage/EasyBuild/sources
subdir-user-modules            (E) = .local/EasyBuild/modules
umask                          (E) = 002
use-existing-modules           (E) = True

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the same effect with using HierarchicalMNS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This occurs with a vanilla instance and the command line:

eb --module-naming-scheme=HierarchicalMNS --subdir-user-modules=.local/EasyBuild/modules GCCcore-5.4.0.eb -x

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix implemented in #2250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants