support depends_on "load" statements in generated modules via --module-depends-on and module_depends_on easyconfig parameter by bartoldeman · Pull Request #2391 · easybuilders/easybuild-framework

bartoldeman · 2018-02-01T14:21:43Z

edit: see http://lmod.readthedocs.io/en/latest/098_dependent_modules.html

Also clean up the Lua generator for this a bit and fix the ignores of the unloads.

We need to bump the minimum Lmod with TCL depends-on to 7.6.1. It's then easiest to just not care about 7.6.0, and make 7.6.1 the minimum supported Lmod for depends_on.

bartoldeman · 2018-03-01T03:31:01Z

Ah Travis can't download env modules 4.0:
Resolving prdownloads.sourceforge.net (prdownloads.sourceforge.net)... 52.85.131.153, 52.85.131.22, 52.85.131.98, ...
Connecting to prdownloads.sourceforge.net (prdownloads.sourceforge.net)|52.85.131.153|:80... connected.
HTTP request sent, awaiting response... 403 Forbidden
2018-03-01 02:04:34 ERROR 403: Forbidden.

we'll try again later then...

boegel · 2018-03-01T07:42:17Z

@bartoldeman Seems like SourceForge was having trouble for a while, seems to be fixed now, I've re-triggered the tests.

boegel · 2018-03-14T19:04:29Z

@bartoldeman Please sync with current develop and fix the merge conflict on .travis.yml (should be trivial to resolve)?

…pends_on

boegel · 2018-03-23T10:16:23Z

    '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],


@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?

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).

boegel · 2018-03-23T10:17:13Z

                       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.",


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

boegel · 2018-03-23T10:19:48Z

+        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


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))

boegel · 2018-03-23T10:20:01Z


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


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

boegel · 2018-03-23T10:22:04Z

        # 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'


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?

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

boegel · 2018-03-23T10:23:41Z

+        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:


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?

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")

boegel · 2018-03-23T10:30:50Z

-        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')


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

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

@boegel

Now initialized to False by default and only set depending on version in Lmod.__init__, as suggested by @boegel.

@boegel

Decouple module-use-depends-on completely from recursive-module-unload, and change recursive-module-unload-depends-on to module-use-depends-on as suggested by @boegel.

bartoldeman · 2018-03-26T01:13:57Z

I addressed all comments I believe, except for the easyconfig parameter. I'll double-check depends_on behaviour in toolchain modules before I remove that (see my comment in the review).

bartoldeman · 2018-03-26T02:29:11Z

One could argue that since #2316 the recursive_module_unload easyconfig parameter is no longer needed since the global config no longer causes load storms.

depends_on() seems to work ok. I get funny module lists if I load foss after iomkl but that could be fixed by using e.g. family('linalg') in the OpenBLAS and imkl modules, and family('toolchain') in the toolchain modules. This is not a very common use case though since EB itself doesn't do it, and module swap works ok.

boegel · 2018-03-30T12:33:04Z

    'moduleclass': ['base', 'Module class to be used for this software', MODULES],
    'moduleforceunload': [False, 'Force unload of all modules when loading the extension', MODULES],
    'moduleloadnoconflict': [False, "Don't check for conflicts, unload other versions instead ", MODULES],
+    'module_use_depends_on' : [False, 'Use depends_on (Lmod 7.6.1+) for dependencies in generated module '


@bartoldeman I would avoid module_use, since that has a particular meaning (not at all related to depends_on).

How about module_depends_on or use_depends_on?

boegel · 2018-03-30T12:33:34Z

            '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,
+            'mod_use_depends_on': cmdline_options.module_use_depends_on,


same here, avoid mod(ule)_use, since it has a very particular meaning

boegel · 2018-03-30T12:34:36Z

+        body.append(load_template)
+
+        if (build_option('recursive_mod_unload') or recursive_unload or
+            load_template == self.LOAD_TEMPLATE_DEPENDS_ON):


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

boegel · 2018-03-30T12:36:04Z

        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:


make this elif?

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

OK, I overlooked that indeed, thanks or clarifying

boegel · 2018-03-30T12:37:38Z

            'module-naming-scheme': ("Module naming scheme to use", None, 'store', DEFAULT_MNS),
            'module-syntax': ("Syntax to be used for module files", 'choice', 'store', DEFAULT_MODULE_SYNTAX,
                              sorted(avail_module_generators().keys())),
+            'module-use-depends-on': ("Use depends_on (Lmod 7.6.1+) for dependencies in all generated modules "


same here, I'd go with either --module-depends-on or --use-depends-on

boegel · 2018-03-30T12:38:36Z

+                ])
+                self.assertEqual(expected, self.modgen.load_module("mod_name", use_depends_on=True))
+                init_config(build_options={'mod_use_depends_on': 'True'})
+                self.assertEqual(expected, self.modgen.load_module("mod_name"))


also check the other case, with self.assertErrorRegex?

boegel · 2018-03-31T07:49:41Z

lgtm, thanks for the effort @bartoldeman!

bartoldeman added 5 commits March 1, 2018 03:02

Support Lmod 7.6+ depends_on via recursive-module-unload-depends-on.

6f21c8f

Bump Lmod version used for Travis testing from 7.4 to 7.6.

fcb0e21

Use depends_on (syntax: "depends-on modname") TCL modulefiles too.

800bd90

Also clean up the Lua generator for this a bit and fix the ignores of the unloads.

Centralize Lmod version checks to modules_tool().has_depends_on/bump.

63bda19

We need to bump the minimum Lmod with TCL depends-on to 7.6.1. It's then easiest to just not care about 7.6.0, and make 7.6.1 the minimum supported Lmod for depends_on.

Fix regular expression for module dependencies to match depends_on.

acf8cd1

bartoldeman force-pushed the module_depends_on branch from 47f3fa3 to acf8cd1 Compare March 1, 2018 03:03

easybuilders deleted a comment from boegelbot Mar 1, 2018

boegel added this to the 3.6.0 milestone Mar 14, 2018

boegel added the enhancement label Mar 14, 2018

Merge remote-tracking branch 'github_hpcugent/develop' into module_de…

2d9166a

…pends_on

boegel requested changes Mar 23, 2018

View reviewed changes

bartoldeman added 2 commits March 25, 2018 01:31

Rename has_depends_on to supports_depends_on and init properly.

8e02128

Now initialized to False by default and only set depending on version in Lmod.__init__, as suggested by @boegel.

Rename recursive-module-unload-depends-on to module-use-depends-on.

5e29702

Decouple module-use-depends-on completely from recursive-module-unload, and change recursive-module-unload-depends-on to module-use-depends-on as suggested by @boegel.

Fix string typo in module_use_depends_on help.

279e4a3

easybuilders deleted a comment from boegelbot Mar 30, 2018

boegel requested changes Mar 30, 2018

View reviewed changes

bartoldeman added 3 commits March 31, 2018 02:07

Replace *use_depends_on by *depends_on as suggested by @boegel.

631810d

Undo wrapping for clarity.

4b1c65e

Test if module-depends-on gives an error with unsupported module tools.

84094e8

boegel changed the title ~~Support Lmod 7.6+ depends_on via recursive-module-unload-depends-on.~~ support depends_on "load" statements in generated modules via --module-depends-on and module_depends_on easyconfig parameter Mar 31, 2018

boegel approved these changes Mar 31, 2018

View reviewed changes

boegel merged commit 23ebd9f into easybuilders:develop Mar 31, 2018

boegel mentioned this pull request Jul 31, 2024

add support for using Lmod's depends_on in generated modules #2280

Closed

Conversation

bartoldeman commented Feb 1, 2018 • edited by boegel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartoldeman commented Mar 1, 2018

Uh oh!

boegel commented Mar 1, 2018

Uh oh!

boegel commented Mar 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartoldeman commented Mar 26, 2018

Uh oh!

bartoldeman commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Mar 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bartoldeman commented Feb 1, 2018 •

edited by boegel

Loading