Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding ESPResSo test PR #144
Adding ESPResSo test PR #144
Changes from 2 commits
e882e6d
e6dc707
6e429af
e4321a8
c06e32f
93ec7aa
eea3538
a2c660f
cbe7fe9
f434c48
bd04f83
2b1640c
ef21ed5
140b9e4
abee145
e523458
c5e0245
df8873c
aebfdc1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Please use
test-suite/eessi/testsuite/hooks.py
Line 386 in b0c91e4
Also, I assume the memory requirement isn't a fixed 50GB, but depends on the scale at which this is run (i.e. number of tasks)? Or doesn't it? If it does, please define an approximate function to compute the memory requirement as a function of task count. It's fine if it is somewhat conservative (i.e. asks for too much), but be aware that the test will be skipped on systems where insufficient memory is available (so don't over-do it).
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.
This was the problem causing OOM and I have fixed it. I am yet to push it into the PR as I am making some more incremental changes to it. I am also observing some crashes on
zen4
such as:https://gitlab.com/eessi/support/-/issues/37#note_1927317164
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.
The sanity checks have actually already executed at lines 111 to 116, and they will interrupt the Python interpreter with an exception if any check fails, so I would suspect the
else
branch is unreachable. I would also recommend against re-expressing the assertions asnp.allclose
in the conditional to avoid redundancy and prevent the risk that the assertions and conditional diverge over time, for example due to changes to tolerance values.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.
Hi @jngrad ,
I have used the same values for tolerances in the both the assertions so the values should not diverge between the assertions and the conditional.
Do these assertions also end the python executions? In that case, I will move the conditional above your original assertions so that the execution can also reach the else part of the code.
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.
They don't diverge today. But they might in a month's time if multiple people contribute to this file, or if you forget that the conditional block must exactly mirror the assertion block above it.
np.testing.assert_allclose()
raises anAssertionError
which halts the Python interpreter with a non-zero exit code.Is the
else
branch truly needed?np.testing.assert_allclose()
already generates a clear error message:I do not totally understand the purpose of this
if/else
statement. If you need to log the tolerances tostdout
, you can do so independently of the success or failure of the assertions, since they are constants. If you need to report the success of failure of the assertions, that is already done by numpy.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.
Yeah, the assertion reports a failure in the manner that you have pointed out but how can I extract a success? Or printing a message right below it would suffice? Since it exits the program anyways.
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.
You can
print("Success")
or check if Python returned exit code 0.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.
This write operation is superfluous if the ReFrame runner captures stdout.
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.
Yes, I have to remove this and is a part of my TODO. :)