Hierarchical module naming scheme categorizing modulefiles by 'moduleclass' #1176

geimer · 2015-02-12T17:04:02Z

NOTE: This PR depends on #1163

This is an enhanced hierarchical module naming scheme which uses the moduleclass easyconfig parameter to further categorize modulefiles on each level of the hierarchy.

@fgeorgatos, @pforai: here you go :-)

by 'moduleclass' easyconfig parameter on each level of the hierarchy

hpcugentbot · 2015-02-12T17:04:03Z

Automatic reply from Jenkins: Can I test this?

fgeorgatos · 2015-02-12T23:26:46Z

hooray! nice contribution Markus, thanks;

My assumption is that most phys/chem HPC shops would much prefer a namespace of this type.

(and now we need to convince @ocaisa for sharing the other alternative toolchain namespace, so that we can cook hybrids of these ideas)

fgeorgatos · 2015-02-12T23:31:16Z

easybuild/tools/module_naming_scheme/categorized_hmns.py

+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+##


as I see it, this is 3-clause BSD, no? http://en.wikipedia.org/wiki/BSD_licenses

It would be nice if it was stated as such somewhere (just me pipedreaming here that all devs would do the same)

@fgeorgatos: Yes, it is 3-clause BSD. I'm so used to it that I thought it was obvious ;-) Not sure where to best add such a statement, though.

just add an extra comment line, mentioning something like:

# License: 3-clause BSD # Author: Markus Geimer (JSC)

ok, you already have the author bit below, so just an extra comment for the license part is sufficient

fgeorgatos · 2015-02-12T23:35:11Z

visual review, all looks good. TBC.

boegel · 2015-02-13T08:51:40Z

We need to get this integrated in the unit tests somehow, to make sure we don't break it in the future.

There are several test modules that now test HierachicalMNS:

test/framework//easyblock.py:520:        Make sure that modules under the HierarchicalMNS are correct,
test/framework//easyblock.py:532:        os.environ['EASYBUILD_MODULE_NAMING_SCHEME'] = 'HierarchicalMNS'
test/framework//easyblock.py:543:        # example: for imkl on top of iimpi toolchain with HierarchicalMNS, no module load statements should be included
test/framework//module_generator.py:399:        os.environ['EASYBUILD_MODULE_NAMING_SCHEME'] = 'HierarchicalMNS'
test/framework//modules.py:258:        os.environ['EASYBUILD_MODULE_NAMING_SCHEME'] = 'HierarchicalMNS'
test/framework//options.py:439:            'module-naming-schemes': ['EasyBuildMNS', 'HierarchicalMNS'],
test/framework//options.py:723:            '--module-naming-scheme=HierarchicalMNS',
test/framework//options.py:1219:        for extra_args in [[], ['--module-naming-scheme=HierarchicalMNS']]:
test/framework//toy_build.py:524:            '--module-naming-scheme=HierarchicalMNS',
test/framework//utilities.py:215:        # a flat scheme like EasyBuildMNS and a hierarhical one like HierarchicalMNS doesn't work

Not all of these need to also test this new MNS, but it should be tested in one or two places at least.

@geimer: Are you up for that? If not, I can look into it (not sure how soon though).

boegel · 2015-02-13T08:51:55Z

Jenkins: ok to test

boegel · 2015-02-13T08:54:39Z

easybuild/tools/module_naming_scheme/categorized_hmns.py

+        paths = []
+        for path in basepaths:
+            for moduleclass in known_module_classes:
+                paths.extend([os.path.join(path, moduleclass)])


this is pretty much copy-paste of det_modpath_extensions, so the body should be fleshed out in another method or function like categorise_paths(paths), which is simply called twice

we already have enough LoC to take care of ;)

or, alternatively, collapse this into a single line using a Python list comprehension:

paths = [os.path.join(p, mc) for p in basepaths for mc in known_module_classes]

and then you don't even need paths or a separate categorize_paths() anymore, just return the list straight away :)

Lovely meeting of Python with the functional languages world ;-)

To be honest, your suggestion looks more like perl than Python... looks awful...

Since I'm in favor of readable code, I opt for the separate method (which in fact makes sense -- and I don't know why I didn't think of it myself).

this is a pretty clean list comprehension, we have many others... ;-)

but sure, go ahead and do it via a separate method then, I don't feel strongly about making it a list comprehension, since I won't have to maintain this bit ;)

ocaisa · 2015-02-13T09:02:28Z

@fgeorgatos It's sitting there waiting for your attention ;) #1168

boegel · 2015-02-13T09:10:48Z

@ocaisa: I've just reviewed #1168

hpcugentbot · 2015-02-13T09:19:01Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1336/
Test PASSed.

hpcugentbot · 2015-02-13T13:38:24Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1342/
Test PASSed.

boegel · 2015-02-13T13:40:15Z

@geimer: the best location for a test is probably test/framework/module_generator.py, see test_hierarchical_mns

geimer · 2015-02-13T13:45:54Z

@boegel: That's what I thought, too. Just staring at it and trying to understand. The first part is quite clear, but what is the final part after the

os.environ['EASYBUILD_MODULE_NAMING_SCHEME'] = self.orig_module_naming_scheme

good for?

boegel · 2015-02-13T13:46:49Z

@geimer: leave that in, it restores whatever module naming scheme was active before running the tests

geimer · 2015-02-13T15:06:27Z

That's understood. I was actually wondering why the test is verifying some things again afterwards, assuming that the original naming scheme is the "traditional" one.

boegel · 2015-02-13T16:19:58Z

@geimer: it's checking whether the default naming scheme setting is being honored, which is why the tests shouldn't be picking up any user config files :)

geimer · 2015-02-13T16:32:52Z

Shouldn't this be in a separate test method then? I mean, test_hierarchical_mns somehow implies that the test is testing the hierarchical module naming scheme. But apparently it's doing more. So why not have a separate test_easybuild_mns? The nested test_ec should then probably be moved outside to avoid code duplication.

boegel · 2015-02-13T16:58:37Z

@geimer: yes, you're probably right, that could be cleaned up, indeed; I guess it's also checking whether restoring the default MNS actually works

geimer · 2015-02-14T11:37:25Z

@boegel: I now integrated a test into the existing method. Assuming that @ocaisa adds a test there too, refactoring should probably be done in a separate PR afterwards to simplify merging.

hpcugentbot · 2015-02-14T11:59:16Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1346/
Test PASSed.

hpcugentbot · 2015-02-15T10:47:28Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1350/
Test FAILed.

hpcugentbot · 2015-02-15T11:40:45Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1352/
Test PASSed.

boegel · 2015-02-15T13:18:03Z

@geimer: just gave this another review, looks great.

The only thing, as discussed off-PR: let's isolate the test modules specific to CategorizedHMNS in a subdirectory of test/framework/modules?

You can enhance setup_hierarchical_modules in test/framework/utilities.py as follows:

def setup_hierarchical_modules(self, subdir=None):
    """Setup hierarchical modules to run tests on."""
    if subdir is not None:
        mod_prefix = os.path.join(self.test_installpath, 'modules', 'all', subdir)
    else:
        mod_prefix = os.path.join(self.test_installpath, 'modules', 'all')

and then call setup_hierarchical_modules(subdir='CategorizedHMNS') before testing your MNS

geimer · 2015-02-15T15:38:54Z

@boegel: There is a little bit more to be done than just modifying mod_prefix. Thus, creating a setup_categorized_modules method.

boegel · 2015-02-15T15:47:16Z

@geimer: that should be setup_categorized_hmns_modules?

geimer · 2015-02-15T15:54:13Z

@boegel: Then setup_hierarchical_modules should have been setup_hierarchical_mns_modules, no? ;-)

I'm happy to rename setup_categorized_modules as suggested. Though renaming setup_hierarchical_modules should be done in a separate PR.

boegel · 2015-02-15T15:56:01Z

I'm just saying you're using a hierarchical MNS too, so only calling it categorised is misleading.

So, setup_categorized_hierarchical_modules is another option. ;-)

geimer · 2015-02-15T16:01:03Z

Ah, got your point. Makes sense.

as suggested

hpcugentbot · 2015-02-15T16:06:10Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1358/
Test PASSed.

…tegorized_hmns

hpcugentbot · 2015-02-15T17:21:35Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1360/
Test FAILed.

hpcugentbot · 2015-02-16T10:13:59Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1363/
Test PASSed.

geimer · 2015-02-16T10:21:18Z

@boegel: Unless you see a need for enhancing even more unit tests for the CategorizedHMNS, I'd say this PR is good for review.

boegel · 2015-02-16T12:05:51Z

@geimer: this looks great, thanks for all the effort! Going in!

@ocaisa: if you need inspiration for adding tests for your PR #1168, checkout the diff here

Hierarchical module naming scheme categorizing modulefiles by 'moduleclass'

Add hierarchical module naming scheme which categorizes modulefiles

f4d2b6a

by 'moduleclass' easyconfig parameter on each level of the hierarchy

fgeorgatos reviewed Feb 12, 2015
View reviewed changes

boegel reviewed Feb 13, 2015
View reviewed changes

ocaisa mentioned this pull request Feb 13, 2015

Add initial support for a toolchain based module naming scheme #1168

Merged

Add note on license and some code refactoring

4563a77

Add unit test for module_generator

de2c619

geimer added 2 commits February 15, 2015 10:12

Adjusted affiliation to make it consistent with other places

2d684b2

Add unit tests for command-line options

f89eb14

geimer added 2 commits February 15, 2015 12:12

Adjust number of modulefiles

ccd9110

Modulefile cleanup

235635a

Move test module files for CategorizedHMNS to subdir

c0da353

Renamed setup_categorized_modules to setup_categorized_hmns_modules

b3dfce9

as suggested

geimer added 2 commits February 15, 2015 17:13

Merge branch 'develop' of github:hpcugent/easybuild-framework into ca…

925ca51

…tegorized_hmns

Add unit test for path_to_top_of_module_tree under CategorizedHMNS

fbf73d6

Create empty module file directory to make C/Tcl modules happy

e3afeab

boegel added a commit that referenced this pull request Feb 16, 2015

Merge pull request #1176 from geimer/categorized_hmns

da5762f

Hierarchical module naming scheme categorizing modulefiles by 'moduleclass'

boegel merged commit da5762f into easybuilders:develop Feb 16, 2015

geimer deleted the categorized_hmns branch February 16, 2015 12:53

Hierarchical module naming scheme categorizing modulefiles by 'moduleclass' #1176

Hierarchical module naming scheme categorizing modulefiles by 'moduleclass' #1176

Conversation

geimer commented Feb 12, 2015

hpcugentbot commented Feb 12, 2015

fgeorgatos commented Feb 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgeorgatos commented Feb 12, 2015

boegel commented Feb 13, 2015

boegel commented Feb 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocaisa commented Feb 13, 2015

boegel commented Feb 13, 2015

hpcugentbot commented Feb 13, 2015

hpcugentbot commented Feb 13, 2015

boegel commented Feb 13, 2015

geimer commented Feb 13, 2015

boegel commented Feb 13, 2015

geimer commented Feb 13, 2015

boegel commented Feb 13, 2015

geimer commented Feb 13, 2015

boegel commented Feb 13, 2015

geimer commented Feb 14, 2015

hpcugentbot commented Feb 14, 2015

hpcugentbot commented Feb 15, 2015

hpcugentbot commented Feb 15, 2015

boegel commented Feb 15, 2015

geimer commented Feb 15, 2015

boegel commented Feb 15, 2015

geimer commented Feb 15, 2015

boegel commented Feb 15, 2015

geimer commented Feb 15, 2015

hpcugentbot commented Feb 15, 2015

hpcugentbot commented Feb 15, 2015

hpcugentbot commented Feb 16, 2015

geimer commented Feb 16, 2015

boegel commented Feb 16, 2015