Skip to content

Conversation

@teojgo
Copy link
Contributor

@teojgo teojgo commented Jul 24, 2018

  • Fix bug which led to accumulation of module unloading.

  • Use raw string to avoid warnings.

Fixes #59

* Fix bug which led to accumulation of module unloading.

* Use raw string to avoid warnings.
@codecov-io
Copy link

codecov-io commented Jul 24, 2018

Codecov Report

Merging #405 into master will increase coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
+ Coverage   91.23%   91.23%   +<.01%     
==========================================
  Files          70       70              
  Lines        8581     8582       +1     
==========================================
+ Hits         7829     7830       +1     
  Misses        752      752
Impacted Files Coverage Δ
reframe/core/environments.py 89.93% <80%> (+0.06%) ⬆️
unittests/test_policies.py 99.54% <0%> (ø) ⬆️

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 b5cad8f...f338b26. Read the comment docs.

@vkarak vkarak self-requested a review July 24, 2018 18:41
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.

Can you explain what was exactly the bug and how it was triggered precisely?

@teojgo
Copy link
Contributor Author

teojgo commented Jul 25, 2018

Imagine that module A conflicts with module C and module B conflicts with D. If we wanted to load modules A, B, looping through them would do the following:

  1. self._conflicted = []
  2. after checking the conflicts of A: self._conflicted = ['C']
  3. Therefore before loading A, module C would be unloaded
  4. after checking the conflicts of B: self._conflicted = ['C', 'D']
  5. Since self._conflicted belongs to the instance, before loading B, modules C, D would be unloaded although C was unloaded in 3.

@vkarak
Copy link
Contributor

vkarak commented Jul 25, 2018

@teojgo Good, I get it now.

for conflict in self._conflicted:
module_conflicts = rt.modules_system.load_module(m, force=True)
self._conflicted += module_conflicts
for conflict in module_conflicts:
Copy link
Contributor

Choose a reason for hiding this comment

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

The loop variable should be simply m.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its a nested loop and the outer one already uses m

self._conflicted += rt.modules_system.load_module(m, force=True)
for conflict in self._conflicted:
module_conflicts = rt.modules_system.load_module(m, force=True)
self._conflicted += module_conflicts
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this assignment after the loop. Placed here, it kind of lets you expect that self._conflicted will be used just right after.


self._conflicted += rt.modules_system.load_module(m, force=True)
for conflict in self._conflicted:
module_conflicts = rt.modules_system.load_module(m, force=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd name this simply conflicted.

@vkarak vkarak merged commit 128b916 into master Jul 25, 2018
@vkarak vkarak deleted the bugfix/module_unload_accumulation branch July 25, 2018 11:51
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