Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion easybuild/framework/easyblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,8 @@ def make_devel_module(self, create_in_builddir=False):
# capture all the EBDEVEL vars
# these should be all the dependencies and we should load them
recursive_unload = self.cfg['recursive_module_unload']
if self.cfg['recursive_module_unload_depends_on']:
recursive_unload = 'depends_on'
for key in os.environ:
# legacy support
if key.startswith(DEVEL_ENV_VAR_NAME_PREFIX):
Expand Down Expand Up @@ -1042,12 +1044,15 @@ def make_module_dep(self, unload_info=None):
self.log.debug("List of retained deps to load in generated module: %s", deps)

# include load statements for retained dependencies
recursive_unload = self.cfg['recursive_module_unload']
if self.cfg['recursive_module_unload_depends_on']:
recursive_unload = 'depends_on'
Copy link
Member

Choose a reason for hiding this comment

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

rather than introducing a magic value for the recursive_unload named argument of load_module, why not add a new named argument use_depends_on?

Copy link
Member

Choose a reason for hiding this comment

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

Also, you should take the build_option into account here (rather than in load_module)?

loads = []
for dep in deps:
unload_modules = []
if dep in unload_info:
unload_modules.append(unload_info[dep])
loads.append(self.module_generator.load_module(dep, recursive_unload=self.cfg['recursive_module_unload'],
loads.append(self.module_generator.load_module(dep, recursive_unload=recursive_unload,
unload_modules=unload_modules))

# Force unloading any other modules
Expand Down
1 change: 1 addition & 0 deletions easybuild/framework/easyconfig/default.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@
'moduleforceunload': [False, 'Force unload of all modules when loading the extension', MODULES],
'moduleloadnoconflict': [False, "Don't check for conflicts, unload other versions instead ", MODULES],
'recursive_module_unload': [False, 'Recursive unload of all dependencies when unloading module', MODULES],
'recursive_module_unload_depends_on' : [False, 'Use depends_on (Lmod 7.6.1+) for dependencies in generated module', MODULES],
Copy link
Member

Choose a reason for hiding this comment

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

@bartoldeman Do you think we need this both as configuration option (--recursive-module-unload-depends-on) as well as an easyconfig parameter (recursive_module_unload_depends_on)?

What use case do you see for latter, i.e. selectively enabling the use of depends_on rather than setting it in the global configuration?

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 will check... this simply mirrors recursive_module_unload.
If I check the history it looks like the people in Juelich wanted to only have recursive unloading enabled for toolchain modules, and not for other modules, to avoid loadstorms. I will double check how depends_on behaves for us on toolchain modules (we hide them but obviously easybuild uses them itself).


# MODULES documentation easyconfig parameters
# (docurls is part of MANDATORY)
Expand Down
2 changes: 2 additions & 0 deletions easybuild/tools/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX):
False: [
'dry_run',
'recursive_mod_unload',
'recursive_mod_unload_depends_on',
'retain_all_deps',
'silent',
'try_to_generate',
Expand Down Expand Up @@ -386,6 +387,7 @@ def init_build_options(build_options=None, cmdline_options=None):
'check_osdeps': not cmdline_options.ignore_osdeps,
'dry_run': cmdline_options.dry_run or cmdline_options.dry_run_short,
'recursive_mod_unload': cmdline_options.recursive_module_unload,
'recursive_mod_unload_depends_on': cmdline_options.recursive_module_unload_depends_on,
'retain_all_deps': retain_all_deps,
'validate': not cmdline_options.force,
'valid_module_classes': module_classes(),
Expand Down
51 changes: 34 additions & 17 deletions easybuild/tools/module_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,9 @@ class ModuleGeneratorTcl(ModuleGenerator):
MODULE_SHEBANG = '#%Module'
CHARS_TO_ESCAPE = ['$']

LOAD_REGEX = r"^\s*module\s+load\s+(\S+)"
LOAD_REGEX = r"^\s*module\s+(?:load|depends-on)\s+(\S+)"
LOAD_TEMPLATE = "module load %(mod_name)s"
LOAD_TEMPLATE_DEPENDS_ON = "depends-on %(mod_name)s"

def check_group(self, group, error_msg=None):
"""
Expand Down Expand Up @@ -505,9 +506,15 @@ def load_module(self, mod_name, recursive_unload=False, unload_modules=None):
body = []
if unload_modules:
body.extend([self.unload_module(m).strip() for m in unload_modules])
body.append(self.LOAD_TEMPLATE)

if build_option('recursive_mod_unload') or recursive_unload:
load_template = self.LOAD_TEMPLATE
# Lmod 7.6.1+ supports depends-on which does this most nicely:
if (build_option('recursive_mod_unload_depends_on') or
recursive_unload == 'depends_on') and modules_tool().has_depends_on:
Copy link
Member

Choose a reason for hiding this comment

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

if depends_on is requested but not supported (supports_depends_on is False), then we should get an error rather than silently ignoring that request?

Copy link
Member

Choose a reason for hiding this comment

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

Taking into account the above, this should become something like:

load_template = self.LOAD_TEMPLATE
if use_depends_on:
    if modules_tool().supports_depends_on:
        load_template = self.LOAD_TEMPLATE_DEPENDS_ON
    else:
        raise EasyBuildError("depends_on statements in generated module are not supported by modules tool")

load_template = self.LOAD_TEMPLATE_DEPENDS_ON
body.append(load_template)

if (build_option('recursive_mod_unload') or recursive_unload or
load_template == self.LOAD_TEMPLATE_DEPENDS_ON):
Copy link
Member

Choose a reason for hiding this comment

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

no need to wrap this across two lines? looks confusing imho...

# not wrapping the 'module load' with an is-loaded guard ensures recursive unloading;
# when "module unload" is called on the module in which the dependency "module load" is present,
# it will get translated to "module unload"
Expand Down Expand Up @@ -668,8 +675,9 @@ class ModuleGeneratorLua(ModuleGenerator):
MODULE_SHEBANG = '' # no 'shebang' in Lua module files
CHARS_TO_ESCAPE = []

LOAD_REGEX = r'^\s*load\("(\S+)"'
LOAD_REGEX = r'^\s*(?:load|depends_on)\("(\S+)"'
LOAD_TEMPLATE = 'load("%(mod_name)s")'
LOAD_TEMPLATE_DEPENDS_ON = 'depends_on("%(mod_name)s")'

PATH_JOIN_TEMPLATE = 'pathJoin(root, "%s")'
UPDATE_PATH_TEMPLATE = '%s_path("%s", %s)'
Expand Down Expand Up @@ -795,19 +803,28 @@ def load_module(self, mod_name, recursive_unload=False, unload_modules=None):
body = []
if unload_modules:
body.extend([self.unload_module(m).strip() for m in unload_modules])
body.append(self.LOAD_TEMPLATE)

if build_option('recursive_mod_unload') or recursive_unload:
# wrapping the 'module load' with an 'is-loaded or mode == unload'
# guard ensures recursive unloading while avoiding load storms,
# when "module unload" is called on the module in which the
# depedency "module load" is present, it will get translated
# to "module unload"
# see also http://lmod.readthedocs.io/en/latest/210_load_storms.html
load_guard = 'isloaded("%(mod_name)s") or mode() == "unload"'

load_template = self.LOAD_TEMPLATE
# Lmod 7.6+ supports depends_on which does this most nicely:
if (build_option('recursive_mod_unload_depends_on') or
recursive_unload == 'depends_on') and modules_tool().has_depends_on:
load_template = self.LOAD_TEMPLATE_DEPENDS_ON

body.append(load_template)
if load_template == self.LOAD_TEMPLATE_DEPENDS_ON:
load_statement = body + ['']
else:
load_guard = 'isloaded("%(mod_name)s")'
load_statement = [self.conditional_statement(load_guard, '\n'.join(body), negative=True)]
if build_option('recursive_mod_unload') or recursive_unload:
Copy link
Member

Choose a reason for hiding this comment

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

make this elif?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it really needs to be else: because of the common load_statement = statement at the bottom.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I overlooked that indeed, thanks or clarifying

# wrapping the 'module load' with an 'is-loaded or mode == unload'
# guard ensures recursive unloading while avoiding load storms,
# when "module unload" is called on the module in which the
# depedency "module load" is present, it will get translated
# to "module unload"
# see also http://lmod.readthedocs.io/en/latest/210_load_storms.html
load_guard = 'isloaded("%(mod_name)s") or mode() == "unload"'
else:
load_guard = 'isloaded("%(mod_name)s")'
load_statement = [self.conditional_statement(load_guard, '\n'.join(body), negative=True)]

return '\n'.join([''] + load_statement) % {'mod_name': mod_name}

Expand Down
5 changes: 5 additions & 0 deletions easybuild/tools/modules.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,11 @@ def modules(self):

def set_and_check_version(self):
"""Get the module version, and check any requirements"""
lmod_depends_on_min = '7.6.1'
if self.COMMAND in MODULE_VERSION_CACHE:
self.version = MODULE_VERSION_CACHE[self.COMMAND]
self.has_depends_on = (isinstance(self, Lmod) and
Copy link
Member

Choose a reason for hiding this comment

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

I'd go with supports_depends_on rather than has_depends_on

supports_depends_on should be set to False in ModulesTool.__init__

and then in Lmod.__init__ it should be set based on the Lmod version after the super call (don't use isinstance(self, Lmod))

StrictVersion(self.version) >= StrictVersion(lmod_depends_on_min))
self.log.debug("Found cached version for %s: %s", self.COMMAND, self.version)
return

Expand Down Expand Up @@ -259,6 +262,8 @@ def set_and_check_version(self):
self.log.debug('Version %s matches requirement <= %s', self.version, self.MAX_VERSION)

MODULE_VERSION_CACHE[self.COMMAND] = self.version
self.has_depends_on = (isinstance(self, Lmod) and
StrictVersion(self.version) >= StrictVersion(lmod_depends_on_min))
Copy link
Member

Choose a reason for hiding this comment

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

You're setting this twice (but it should be moved to the Lmod class anyway)


def check_cmd_avail(self):
"""Check whether modules tool command is available."""
Expand Down
2 changes: 2 additions & 0 deletions easybuild/tools/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,8 @@ def config_options(self):
None, 'store', None),
'recursive-module-unload': ("Enable generating of modules that unload recursively.",
None, 'store_true', False),
'recursive-module-unload-depends-on': ("Use depends_on (Lmod 7.6.1+) for dependencies in all generated modules.",
Copy link
Member

Choose a reason for hiding this comment

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

Should this just be use-depends-on, and then have the help message clarify that this implies that it makes unloading of modules recursive?

None, 'store_true', False),
'repository': ("Repository type, using repositorypath",
'choice', 'store', DEFAULT_REPOSITORY, sorted(avail_repositories().keys())),
'repositorypath': (("Repository path, used by repository "
Expand Down
22 changes: 22 additions & 0 deletions test/framework/module_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,17 @@ def test_load(self):

init_config(build_options={'recursive_mod_unload': True})
self.assertEqual(expected, self.modgen.load_module("mod_name"))

# Lmod 7.6+ depends-on
if self.modtool.has_depends_on:
expected = '\n'.join([
'',
"depends-on mod_name",
'',
])
self.assertEqual(expected, self.modgen.load_module("mod_name", recursive_unload="depends_on"))
init_config(build_options={'recursive_mod_unload_depends_on': 'True'})
self.assertEqual(expected, self.modgen.load_module("mod_name"))
Copy link
Member

Choose a reason for hiding this comment

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

also check the other case, with self.assertErrorRegex?

else:
# default: guarded module load (which implies no recursive unloading)
expected = '\n'.join([
Expand All @@ -283,6 +294,17 @@ def test_load(self):
init_config(build_options={'recursive_mod_unload': True})
self.assertEqual(expected, self.modgen.load_module("mod_name"))

# Lmod 7.6+ depends_on
if self.modtool.has_depends_on:
expected = '\n'.join([
'',
'depends_on("mod_name")',
'',
])
self.assertEqual(expected, self.modgen.load_module("mod_name", recursive_unload="depends_on"))
init_config(build_options={'recursive_mod_unload_depends_on': 'True'})
self.assertEqual(expected, self.modgen.load_module("mod_name"))

def test_unload(self):
"""Test unload part in generated module file."""

Expand Down
38 changes: 21 additions & 17 deletions test/framework/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -1156,23 +1156,27 @@ def test_recursive_module_unload(self):
eb_file = os.path.join(os.path.dirname(__file__), 'easyconfigs', 'test_ecs', 't', 'toy', 'toy-0.0-deps.eb')

# check log message with --skip for existing module
args = [
eb_file,
'--sourcepath=%s' % self.test_sourcepath,
'--buildpath=%s' % self.test_buildpath,
'--installpath=%s' % self.test_installpath,
'--debug',
'--force',
'--recursive-module-unload',
]
self.eb_main(args, do_build=True, verbose=True)

toy_module = os.path.join(self.test_installpath, 'modules', 'all', 'toy', '0.0-deps')
if get_module_syntax() == 'Lua':
toy_module += '.lua'
toy_module_txt = read_file(toy_module)
is_loaded_regex = re.compile(r"if { !\[is-loaded gompi/1.3.12\] }", re.M)
self.assertFalse(is_loaded_regex.search(toy_module_txt), "Recursive unloading is used: %s" % toy_module_txt)
lastargs = ['--recursive-module-unload']
if self.modtool.has_depends_on:
lastargs.append(lastargs[0]+'-depends-on')
Copy link
Member

Choose a reason for hiding this comment

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

We should not stop testing --recursive-module-unload with recent Lmod versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change it tests both, but I renamed the option now.

for lastarg in lastargs:
args = [
eb_file,
'--sourcepath=%s' % self.test_sourcepath,
'--buildpath=%s' % self.test_buildpath,
'--installpath=%s' % self.test_installpath,
'--debug',
'--force',
lastarg,
]
self.eb_main(args, do_build=True, verbose=True)

toy_module = os.path.join(self.test_installpath, 'modules', 'all', 'toy', '0.0-deps')
if get_module_syntax() == 'Lua':
toy_module += '.lua'
toy_module_txt = read_file(toy_module)
is_loaded_regex = re.compile(r"if { !\[is-loaded gompi/1.3.12\] }", re.M)
self.assertFalse(is_loaded_regex.search(toy_module_txt), "Recursive unloading is used: %s" % toy_module_txt)

def test_tmpdir(self):
"""Test setting temporary directory to use by EasyBuild."""
Expand Down