Skip to content
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

use mixin class for cp2k #193

Merged
merged 3 commits into from
Nov 13, 2024
Merged

use mixin class for cp2k #193

merged 3 commits into from
Nov 13, 2024

Conversation

smoors
Copy link
Collaborator

@smoors smoors commented Oct 12, 2024

additional improvements to the mixin class:

  • add hook set_tag_ci() to the mixin class, which is used if both bench_name_ci and bench_name are set. we could also make this a mandatory attribute, to be discussed.
  • make measure_memory_usage a reframe variable that can be set on the cmd line with reframe -S measure_memory_usage=True

cp2k.py file reduced from 114 lines to 76 \o/

@@ -113,6 +115,16 @@ def measure_mem_usage(self):
# instead of the @performance_function decorator
self.perf_variables['memory'] = make_performance_function(hooks.extract_memory_usage, 'MiB', self)

@run_after('init', always_last=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I find this only mildly useful, since bench_name is not a standard reframe test property. I.e. essentially you replaced defining a pipeline hook after init to set the tag with two property definitions. I'm not against it, but the gain is relatively minor. It does make the child test more 'configuration-only', and less programming, that's true.

One potential issue is if someone wants to do more complex stuff, e.g. set one test case to CI for CPU runs and another for GPU runs. I mean, it's still possible, they can then define the self.bench_name_ci conditionally on the CPU vs GPU part.

The one thing this does make sure is that the tag is standardized (though we also did that by making it a constant).

Ok, I'm convinced, this is useful enough (but does need to be documented - I realize I didn't document the use of the CI tag at all in my portable-test-instructions)

Copy link
Collaborator

@casparvl casparvl left a comment

Choose a reason for hiding this comment

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

Looks good to me. We should adapt the docs to document the self.bench_name and self.bench_name_ci.

@casparvl casparvl merged commit d7800c7 into EESSI:main Nov 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants