use mixin class for QuantumESPRESSO #212
Conversation
| @@ -435,7 +434,7 @@ def _set_or_append_valid_systems(test: rfm.RegressionTest, valid_systems: str): | |||
| warn_msg = f"valid_systems has multiple ({len(test.valid_systems)}) items," | |||
| warn_msg += " which is not supported by this hook." | |||
| warn_msg += " Make sure to handle filtering yourself." | |||
| warnings.warn(warn_msg) | |||
There was a problem hiding this comment.
Just to double check: these changes were not accidental, right?
There was a problem hiding this comment.
I don't mind too much, but you could set bench_name=None in the class body. Then, with the current logic of setting self.banch_name=self.bench_name_ci in that one particular case in the init hook, that should work, right? I.e. it's essentially only one extra line in the test (a default bench_name).
There was a problem hiding this comment.
you're right, i readded the bench_name check in the mixin class.
i now also set bench_name_ci only for the CI test to avoid that bench_name always has to be set to something.
There was a problem hiding this comment.
and yes, those warning changes are again not accidental :)
There was a problem hiding this comment.
@smoors if you can just double check that the changes to the hooks.py were intentional and check if you'd still like to chance anything regarding the bench_name approach, I'm ok with this PR. Even if you don't change the bench_name approach, I'm ok.
Tested the CI case on all core counts: works like a charm.
notes:
- the mixin class no longer raises an error ifbench_nameis not defined as it's not really needed, and it helps making the QE test a bit simpler.