Merge pull request #1611 from vkarak/bugfix/treat-conflicts-module-mappings

[bugfix] Treat conflicts in module mappings properly

Author: Vasileios Karakasis

reframe/core/modules.py

@@ -82,7 +82,7 @@ def __eq__(self, other):
             return self.name == other.name and self.version == other.version
 
     def __repr__(self):
-        return '%s(%s)' % (type(self).__name__, self.fullname)
+        return f'{type(self).__name__}({self.fullname}, {self.collection})'
 
     def __str__(self):
         return self.fullname
@@ -222,15 +222,20 @@ def load_module(self, name, force=False, collection=False):
             conflicting modules currently loaded. If module ``name`` refers to
             multiple real modules, all of the target modules will be loaded.
         :arg collection: The module is a "module collection" (TMod4 only)
-        :returns: the list of unloaded modules as strings.
+        :returns: A list of two-element tuples, where each tuple contains the
+            module that was loaded and the list of modules that had to be
+            unloaded first due to conflicts. This list will be normally of
+            size one, but it can be longer if there is mapping that maps
+            module ``name`` to multiple other modules.
 
         .. versionchanged:: 3.3
-           The ``collection`` argument was added.
+           - The ``collection`` argument was added.
+           - This function now returns a list of tuples.
 
         '''
         ret = []
         for m in self.resolve_module(name):
-            ret += self._load_module(m, force, collection)
+            ret.append((m, self._load_module(m, force, collection)))
 
         return ret
 
@@ -349,37 +354,41 @@ def searchpath_remove(self, *dirs):
         return self._backend.searchpath_remove(*dirs)
 
     def emit_load_commands(self, name, collection=False):
-        '''Return the appropriate shell command for loading module ``name``.
+        '''Return the appropriate shell commands for loading a module.
+
+        Module mappings are not taken into account by this function.
 
         :arg name: The name of the module to load.
         :arg collection: The module is a "module collection" (TMod4 only)
         :returns: A list of shell commands.
 
         .. versionchanged:: 3.3
-           The ``collection`` argument was added.
+           The ``collection`` argument was added and module mappings are no
+           more taken into account by this function.
 
         '''
-        ret = []
-        for name in self.resolve_module(name):
-            cmds = self._backend.emit_load_instr(Module(name, collection))
-            if cmds:
-                ret.append(cmds)
 
-        return ret
+        # We don't consider module mappings here, because we cannot treat
+        # correctly possible conflicts
+        return [self._backend.emit_load_instr(Module(name, collection))]
 
     def emit_unload_commands(self, name, collection=False):
-        '''Return the appropriate shell command for unloading module
-        ``name``.
+        '''Return the appropriate shell commands for unloading a module.
+
+        Module mappings are not taken into account by this function.
+
+        :arg name: The name of the module to unload.
+        :arg collection: The module is a "module collection" (TMod4 only)
+        :returns: A list of shell commands.
+
+        .. versionchanged:: 3.3
+           The ``collection`` argument was added and module mappings are no
+           more taken into account by this function.
 
-        :rtype: List[str]
         '''
-        ret = []
-        for name in self.resolve_module(name):
-            cmds = self._backend.emit_unload_instr(Module(name, collection))
-            if cmds:
-                ret.append(cmds)
 
-        return ret
+        # See comment in emit_load_commands()
+        return [self._backend.emit_unload_instr(Module(name, collection))]
 
     def __str__(self):
         return str(self._backend)

reframe/core/runtime.py

@@ -212,16 +212,19 @@ def loadenv(*environs):
     env_snapshot = snapshot()
     commands = []
     for env in environs:
-        for m in env.modules_detailed:
-            conflicted = modules_system.load_module(**m, force=True)
-            for c in conflicted:
-                commands += modules_system.emit_unload_commands(c)
+        for mod in env.modules_detailed:
+            load_seq = modules_system.load_module(**mod, force=True)
+            for m, conflicted in load_seq:
+                for c in conflicted:
+                    commands += modules_system.emit_unload_commands(c)
 
-            commands += modules_system.emit_load_commands(**m)
+                commands += modules_system.emit_load_commands(
+                    m, mod['collection']
+                )
 
         for k, v in env.variables.items():
             os.environ[k] = osext.expandvars(v)
-            commands.append('export %s=%s' % (k, v))
+            commands.append(f'export {k}={v}')
 
     return env_snapshot, commands
 
@@ -284,7 +287,7 @@ def __init__(self, config_file, sysname=None, options=None):
             _runtime_context = None
         else:
             site_config = config.load_config(config_file)
-            site_config.select_subconfig(sysname)
+            site_config.select_subconfig(sysname, ignore_resolve_errors=True)
             for opt, value in options.items():
                 site_config.add_sticky_option(opt, value)
 

unittests/modules/testmod_bar

@@ -1,6 +1,6 @@
 #%Module
 proc ModulesHelp { } {
-    Helper module for PyRegression unit tests
+    Helper module for ReFrame unit tests
 }
 
 module-whatis { Helper module for ReFrame unit tests }

unittests/modules/testmod_boo

@@ -1,6 +1,6 @@
 #%Module
 proc ModulesHelp { } {
-    Helper module for PyRegression unit tests
+    Helper module for ReFrame unit tests
 }
 
 module-whatis { Helper module for ReFrame unit tests }

unittests/modules/testmod_ext

@@ -0,0 +1,9 @@
+#%Module
+proc ModulesHelp { } {
+    Helper module for ReFrame unit tests
+}
+
+module-whatis { Helper module for ReFrame unit tests }
+
+module load testmod_base
+module load testmod_bar

unittests/modules/testmod_foo

@@ -1,6 +1,6 @@
 #%Module
 proc ModulesHelp { } {
-    Helper module for PyRegression unit tests
+    Helper module for ReFrame unit tests
 }
 
 module-whatis { Helper module for ReFrame unit tests }

unittests/test_environments.py

@@ -252,6 +252,23 @@ def test_emit_loadenv_commands_with_confict(base_environ, user_runtime,
     assert expected_commands == rt.emit_loadenv_commands(env0)
 
 
+def test_emit_loadenv_commands_mapping_with_conflict(base_environ,
+                                                     user_runtime,
+                                                     modules_system):
+    if modules_system.name == 'tmod4':
+        pytest.skip('test scenario not valid for tmod4')
+
+    e0 = env.Environment(name='e0', modules=['testmod_ext'])
+    ms = rt.runtime().modules_system
+    ms.load_mapping('testmod_ext: testmod_ext testmod_foo')
+    expected_commands = [
+        ms.emit_load_commands('testmod_ext')[0],
+        ms.emit_unload_commands('testmod_bar')[0],
+        ms.emit_load_commands('testmod_foo')[0],
+    ]
+    assert expected_commands == rt.emit_loadenv_commands(e0)
+
+
 def test_emit_loadenv_failure(user_runtime):
     snap = rt.snapshot()
     environ = env.Environment('test', modules=['testmod_foo', 'testmod_xxx'])

unittests/test_modules.py

@@ -112,13 +112,13 @@ def test_module_load_force(modules_system):
         modules_system.load_module('testmod_foo')
 
         unloaded = modules_system.load_module('testmod_foo', force=True)
-        assert 0 == len(unloaded)
+        assert unloaded == [('testmod_foo', [])]
         assert modules_system.is_module_loaded('testmod_foo')
 
         unloaded = modules_system.load_module('testmod_bar', force=True)
         assert modules_system.is_module_loaded('testmod_bar')
         assert not modules_system.is_module_loaded('testmod_foo')
-        assert 'testmod_foo' in unloaded
+        assert unloaded == [('testmod_bar', ['testmod_foo'])]
         assert 'TESTMOD_BAR' in os.environ
 
 
@@ -127,7 +127,7 @@ def test_module_load_force_collection(modules_system, module_collection):
     modules_system.load_module('testmod_bar')
     unloaded = modules_system.load_module(module_collection,
                                           force=True, collection=True)
-    assert unloaded == []
+    assert unloaded == [('test_collection', [])]
     assert modules_system.is_module_loaded('testmod_base')
     assert modules_system.is_module_loaded('testmod_foo')
 
@@ -165,7 +165,7 @@ def test_module_available_all(modules_system):
         assert modules == []
     else:
         assert (modules == ['testmod_bar', 'testmod_base',
-                            'testmod_boo', 'testmod_foo'])
+                            'testmod_boo', 'testmod_ext', 'testmod_foo'])
 
 
 def test_module_available_substr(modules_system):

unittests/test_utility.py

@@ -1429,8 +1429,8 @@ def test_find_modules(modules_system):
     if modules_system.name == 'nomod':
         assert found_modules == []
     else:
-        assert found_modules == ['testmod_bar', 'testmod_base',
-                                 'testmod_boo', 'testmod_foo']*ntimes
+        assert found_modules == ['testmod_bar', 'testmod_base', 'testmod_boo',
+                                 'testmod_ext', 'testmod_foo']*ntimes
 
 
 def test_find_modules_env_mapping(modules_system):