[wip] [feat] Module helper

[davidedelvento]

Contex
Per discussion on #1159
Not adding [feat] to the PR title since there is no feature just a helper file which (hopefully) will later contribute to the feature implementation described in #1159

Actual PR description
This is what NCAR uses in house, with minimal cleaning, for the very reason of #1159. Before spending more time on it to clean it up and integrating into ReFrame, I wanted to get some feedback.

Note that there is one skipped test (empty, mostly a reminder of something we might want to add later) and a failed one. The one that fails is there to demonstrate the assumptions that the method makes which may cause unexpected behavior. I think that rather than fixing the parsing for all possible edge cases, it'd be better, easier and more robust if the users avoid combinations of modules which cause them (see the test case for detail, in my opinion is very clear what is happening and why). The failing test is there just to grab your attention and talk about options. I am pretty sure before merging this test needs at least to be marked as skipped.

Besides that, not sure what you'd prefer to do, since this is a standalone file dropped into the tree, which at the moment has no connection with the rest of the framework. Perhaps if there's a branch were #1159 is being worked on, it may be better merged there?

[pep8speaks]

Hello @davidedelvento, Thank you for submitting the Pull Request!

• In the file reframe/utility/lmod_helper.py:

Line 3:1: E302 expected 2 blank lines, found 1
Line 5:13: E225 missing whitespace around operator
Line 6:36: E251 unexpected spaces around keyword / parameter equals
Line 6:38: E251 unexpected spaces around keyword / parameter equals
Line 9:42: E261 at least two spaces before inline comment
Line 14:15: E222 multiple spaces after operator
Line 21:1: E302 expected 2 blank lines, found 1
Line 36:1: E302 expected 2 blank lines, found 1
Line 47:1: E302 expected 2 blank lines, found 1
Line 55:1: E305 expected 2 blank lines after class or function definition, found 1
Line 56:1: E302 expected 2 blank lines, found 0
Line 63:12: E713 test for membership should be 'not in'
Line 66:80: E501 line too long (80 > 79 characters)

• In the file unittests/test_lmod_helper.py:

Line 15:80: E501 line too long (235 > 79 characters)
Line 18:80: E501 line too long (235 > 79 characters)
Line 25:80: E501 line too long (97 > 79 characters)
Line 29:80: E501 line too long (100 > 79 characters)
Line 45:80: E501 line too long (235 > 79 characters)
Line 52:80: E501 line too long (97 > 79 characters)
Line 56:1: E302 expected 2 blank lines, found 1
Line 67:1: E302 expected 2 blank lines, found 1
Line 71:1: E302 expected 2 blank lines, found 1
Line 75:1: E302 expected 2 blank lines, found 1
Line 79:1: E302 expected 2 blank lines, found 1
Line 82:15: E222 multiple spaces after operator
Line 88:80: E501 line too long (82 > 79 characters)
Line 90:1: E302 expected 2 blank lines, found 1
Line 96:19: E261 at least two spaces before inline comment
Line 97:80: E501 line too long (80 > 79 characters)
Line 98:80: E501 line too long (106 > 79 characters)
Line 99:80: E501 line too long (107 > 79 characters)
Line 99:106: E202 whitespace before '}'
Line 101:15: E222 multiple spaces after operator
Line 109:69: E202 whitespace before ']'
Line 110:49: E251 unexpected spaces around keyword / parameter equals
Line 110:51: E251 unexpected spaces around keyword / parameter equals
Line 111:80: E501 line too long (82 > 79 characters)
Line 113:1: E302 expected 2 blank lines, found 1
Line 114:80: E501 line too long (101 > 79 characters)
Line 118:1: E302 expected 2 blank lines, found 1
Line 120:80: E501 line too long (134 > 79 characters)
Line 122:80: E501 line too long (141 > 79 characters)
Line 125:80: E501 line too long (109 > 79 characters)
Line 126:1: E302 expected 2 blank lines, found 1
Line 127:19: E201 whitespace after '{'
Line 127:80: E501 line too long (111 > 79 characters)
Line 128:80: E501 line too long (94 > 79 characters)
Line 129:80: E501 line too long (120 > 79 characters)
Line 130:80: E501 line too long (121 > 79 characters)
Line 130:120: E202 whitespace before '}'
Line 131:49: E251 unexpected spaces around keyword / parameter equals
Line 131:51: E251 unexpected spaces around keyword / parameter equals
Line 132:80: E501 line too long (92 > 79 characters)
Line 133:15: E222 multiple spaces after operator
Line 141:74: E202 whitespace before ']'

Do see the ReFrame Coding Style Guide

[jenkins-cscs]

Can I test this patch?

[victorusu]

@davidedelvento, thanks for the PR.
I think it is a great initiative, we should have that inside ReFrame.
I am just concerned about the integration with the current code, since some of the helper functions or classes that you implement are not in sync with the way modules are implemented in ReFrame or have already been implemented inside ReFrame.

[victorusu]

It would be great if this code could have a tighter integration with ReFrame.
The PopenCommunicate() should not be needed since ReFrame provides methods to run external processes.
Having said that, I am in the opinion that one needs to use the Python bindings for Lmod, which is the way that all module systems are implemented in ReFrame (https://github.com/eth-cscs/reframe/blob/master/reframe/core/modules.py). So, this class should be dropped.

[davidedelvento]

Hi @victorusu
Thanks for the suggestions. I see your points. In fact, I pushed here what I had done in another context, so it's no surprise that it looks a bit like duct-taped ;-)

I'll try to get to your suggestions as soon as I have time.
Cheers,
Davide

[victorusu]

I think an extended version of this function should go to the Lmod backend. But it does require an extension of the Modules' System.

[victorusu]

I also think this should go to the Lmod backend.
The python bindings for Lmod are preferred over calling ml avail.
And the function name should be slightly improved to better explain what it does.

[davidedelvento]

I haven't forgotten about this, quite the contrary. Just gotten caught in more urgent things. Hope to find the time for this (and overall reframe work) in the not-too-far future...

[vkarak]

@davidedelvento Nice. It will be much easier to merge this if it targets specifically #1159. For that we'd need simply to extend the module systems API of ReFrame (and accordingly its backends) to obtain a list of module names based on a given pattern. Each backend may implement that in its own way. We'd also need a utility function (pretty simple) that would expose that functionality in a convenient way to be used in a paremeterized test. Such a PR would be much simpler than this one and would make it easier to merge upstream.

[vkarak]

This is replaced by #1417.