-
Notifications
You must be signed in to change notification settings - Fork 117
[test] Add MPI version of the eat memory check #1613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1613 +/- ##
==========================================
- Coverage 87.54% 87.54% -0.01%
==========================================
Files 44 44
Lines 7292 7288 -4
==========================================
- Hits 6384 6380 -4
Misses 908 908
Continue to review full report at Codecov.
|
ekouts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though none of the tests fail there is an error printed in the end:
[ PASSED ] Ran 15 test case(s) from 10 check(s) (0 failure(s))
[==========] Finished on Fri Nov 20 15:11:26 2020
./bin/reframe: rfm_MemoryMpiCheck_job.out: No such file or directory
It is coming from these lines, but I am not sure how you should write the test to avoid it
regex_mem = r'^Currently avail memory: (\d+)'
self.reference_meminfo = \
sn.extractsingle(regex_mem, self.stdout, 1,
conv=lambda x: int(int(x) / 1024**3))
Also this part doesn't need to be in a hook, since you use the deferable, right?
|
👍 I moved it out of the hook. |
vkarak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I cannot understand very well is the value of this test. Why running the single node eat memory test on multiple nodes wouldn't do the same?
It's an extension of the MemoryOverconsumptionCheck: it tests a slurm flag with a fixed value: |
|
@jgphpc I did some minor enhancements to your test, but on Dom it fails to meet some references. Can you check? And then we can merge. |
Changes done and we need to merge it today.
thanks @ekouts