Skip to content

Conversation

@boegel
Copy link
Contributor

@boegel boegel commented May 29, 2018

When running ./test_reframe.py -v, one test was failing on our system:

FAIL: test_module_unload_all (unittests.test_modules.TestLModModulesSystem)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/user/scratchphanpy/gent/gvo000/gvo00002/vsc40023/easybuild_REGTEST/CO7/sandybridge/software/ReFrame/2.12-foss-2018a-Python-3.6.4/unittests/test_modules.py", line 58, in test_module_unload_all
    self.assertEqual(0, len(self.modules_system.loaded_modules()))
AssertionError: 0 != 1

----------------------------------------------------------------------
Ran 404 tests in 771.699s

FAILED (SKIP=24, failures=1)

This occurs because we are using Lmod and we have one "sticky" module (cfr.https://lmod.readthedocs.io/en/latest/240_sticky_modules.html) loaded, which only gets unloaded with module --force purge, see below.

I'm not sure whether this is the correct fix though, it depends for what purpose unload_all is actually being used... The sticky cluster module we have sets up $MODULEPATH for example, so if it's unloaded the modules we provide centrally will no longer be available...

Another way to fix this would be to check differently in the test whether unload_all unloads the previously loaded modules...

$ module list

Currently Loaded Modules:
  1) cluster/delcatty (S)

  Where:
   S:  Module is Sticky, requires --force to unload or purge

$ module purge
The following modules were not unloaded:
  (Use "module --force purge" to unload all):

  1) cluster/delcatty

$ module list

Currently Loaded Modules:
  1) cluster/delcatty (S)

  Where:
   S:  Module is Sticky, requires --force to unload or purge

$ module --force purge
$ module list
No modules loaded

@jenkins-cscs
Copy link
Collaborator

Can I test this patch?

@vkarak
Copy link
Contributor

vkarak commented May 29, 2018

Good catch, @boegel! We don't run Lmod in production, so our testing uses mostly the basic setup. Thanks for submitting the fix.

@vkarak
Copy link
Contributor

vkarak commented May 29, 2018

@jenkins-cscs retry all

@boegel
Copy link
Contributor Author

boegel commented May 29, 2018

@vkarak Can you clarify what unload_all is used for? I suspect doing a forced purge may not actually be what you want to do...

@vkarak
Copy link
Contributor

vkarak commented May 29, 2018

@boegel Indeed, the purpose of unload_all() is to abstract the purge command. We don't use it explicitly in the framework, but we have some unit tests testing it. I guess we could add another one exposing this problem. I'll have a look at the details of your PR later on.

@vkarak vkarak self-assigned this May 29, 2018
@vkarak vkarak added this to the Upcoming sprint milestone Jun 4, 2018
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

@boegel I've done a PR into your branch adding a small comment for your fix.

Sorry for the delay in approving your PR, but I was quite busy last week and I wanted also to check whether it made sense to extend our internal API with a force option, but eventually, I don't think there is such need, cos we are not using unload_all in any case.

Add note about force purge in Lmod backend
@codecov-io
Copy link

codecov-io commented Jun 7, 2018

Codecov Report

Merging #307 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
+ Coverage   91.46%   91.46%   +<.01%     
==========================================
  Files          66       66              
  Lines        7805     7807       +2     
==========================================
+ Hits         7139     7141       +2     
  Misses        666      666
Impacted Files Coverage Δ
reframe/core/modules.py 67.9% <50%> (-0.12%) ⬇️
unittests/test_policies.py 99.54% <0%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d199ba4...1969397. Read the comment docs.

@vkarak vkarak merged commit 3bd86b2 into reframe-hpc:master Jun 7, 2018
@boegel boegel deleted the force_purge branch December 6, 2019 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants