[test] Add generic checks for the cscs-supported apps into `hpctestlib/apps` #2084

hurricane642 · 2021-07-20T09:12:08Z

This PR contains a base for the tests and their implementation for cscs-checks with updated syntax and unified style. I'll be happy with your review and suggestions!

update reframe to 3.7.1 version

pep8speaks · 2021-07-20T09:12:11Z

Hello @hurricane642, Thank you for updating!

In the file cscs-checks/apps/amber/amber_check.py:

Line 15:13: E231 missing whitespace after ':'
Line 65:1: E302 expected 2 blank lines, found 1
Line 86:80: E501 line too long (81 > 79 characters)

Do see the ReFrame Coding Style Guide

Comment last updated at 2021-09-01 12:12:53 UTC

jenkins-cscs · 2021-07-20T09:15:02Z

Can I test this patch?

jjotero · 2021-07-20T16:00:27Z

ok to test

codecov-commenter · 2021-07-20T16:01:37Z

Codecov Report

Merging #2084 (0ea8823) into master (78e7ce2) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 0ea8823 differs from pull request most recent head 817802b. Consider uploading reports for the commit 817802b to get more accurate results

@@            Coverage Diff             @@
##           master    #2084      +/-   ##
==========================================
- Coverage   86.29%   86.26%   -0.03%     
==========================================
  Files          53       53              
  Lines        9571     9313     -258     
==========================================
- Hits         8259     8034     -225     
+ Misses       1312     1279      -33

Impacted Files	Coverage Δ
reframe/frontend/autodetect.py	`57.48% <0.00%> (-4.40%)`	⬇️
reframe/frontend/loader.py	`92.12% <0.00%> (-1.84%)`	⬇️
reframe/utility/typecheck.py	`95.45% <0.00%> (-1.69%)`	⬇️
reframe/core/namespaces.py	`95.08% <0.00%> (-1.64%)`	⬇️
reframe/frontend/cli.py	`75.95% <0.00%> (-0.37%)`	⬇️
reframe/core/pipeline.py	`91.90% <0.00%> (-0.35%)`	⬇️
reframe/core/meta.py	`98.92% <0.00%> (-0.23%)`	⬇️
reframe/core/deferrable.py	`98.04% <0.00%> (-0.17%)`	⬇️
reframe/utility/sanity.py	`97.79% <0.00%> (-0.03%)`	⬇️
reframe/utility/osext.py	`84.71% <0.00%> (ø)`
... and 6 more

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 78e7ce2...817802b. Read the comment docs.

jjotero · 2021-07-20T16:05:36Z

Hi @hurricane642! One comment regarding the file structure. The base tests are meant to go into hpctestlib/apps and there you can have you Amber tests in hpctestlib/apps/amber/__init__.py. Then, there is no need to prepend the derived file with the _der.py and you can just leave the names as amber there (and the same for all the other apps).

So when you import your base, the python import should look as from hpctestlib.apps.amber import AmberBase.

jjotero

A few more comments on the structure before delving into the test-specifics:

Rename the cscs-checks/apps/lammps/lammps to cscs-checks/apps/lammps/lammps_check.py and then use git mv if you want to. Otherwise, we'd be losing all the git history.
Delete the __init__ files that just have a version ID in there.
For each test, just replace the __init__ with the *_base.py files. That intermediate step is not needed. Have a look into the other tests in the library to see how it's done there.

cscs-checks/apps/amber/amber.py

jjotero

I think it's better if each app is under its own directory instead of having all the python files in the same place. The reason behind this is that each test might have their own additional files that one might need for the tests, and then you don't want to have all those files mixed up in the same directory (please, have a look into hpctestlib/microbenchmarks/gpu and see how it's done in there). So in short, I would rename hpctestlib/apps/amber.py to hpctestlib/apps/amber/__init__.py (and the same for all the other apps).

jjotero

I have just briefly gone through the base classes for now.

I think there are a few conceptual changes to be made to the base classes. Things like the self.benchmark parameter (which referenced, but not declared in the base classes) should not be enforced from the base class. If someone wants to parameterise all these tests, they're free to do so in the base class. But if someone doesn't they should not need to define anything such as self.benchmark, because there is only one of them. Also, (and this is for the derived classes) perhaps the name variant makes more sense than benchmark.

hpctestlib/apps/lammps/__init__.py

hpctestlib/apps/amber/__init__.py

hpctestlib/apps/vasp/__init__.py

jjotero · 2021-09-01T08:20:42Z

cscs-checks/apps/amber/amber_check.py

+        for key in list(self.REFERENCE_DICT):
+            if self.current_partition.fullname in key:
+                d = {self.current_partition.fullname:
+                     self.REFERENCE_DICT[key]}
+                self.reference = d


I would say that we make this set_perf_reference a different hook for both the GPU and CPU cases, simply because their references above are compacted in a different format. So move this hook into the GPU-specific class, and then create a similar one in the GPU class where you also deal with the small & large scopes of the dictionary.

This also avoids having to store another reference to this REFERENCE_DICT in the class, which is a bit confusing.

Suggested change

for key in list(self.REFERENCE_DICT):

if self.current_partition.fullname in key:

d = {self.current_partition.fullname:

self.REFERENCE_DICT[key]}

self.reference = d

for key, val in REFERENCE_GPU_PERFORMANCE.items():

if self.current_partition.fullname in key:

self.reference = {'*': val}

break

else:

raise ValueError(

f'could not find a reference for the current '

f'partition {self.current_partition.fullname!r}'

)

jjotero · 2021-09-01T08:29:47Z

cscs-checks/apps/amber/amber_check.py

+    },
 }

 REFERENCE_CPU_PERFORMANCE_LARGE = {


The keys for this dictionary also must be made tuples.

jjotero · 2021-09-01T08:30:09Z

cscs-checks/apps/amber/amber_check.py

+    @run_after('setup')
+    def set_reference_dict(self):
+        self.REFERENCE_DICT = REFERENCE_CPU_PERFORMANCE[self.scale]


Suggested change

@run_after('setup')

def set_reference_dict(self):

self.REFERENCE_DICT = REFERENCE_CPU_PERFORMANCE[self.scale]

@run_before('performance')

def set_perf_reference(self):

for key, val in REFERENCE_CPU_PERFORMANCE[self.scale].items():

if self.current_partition.fullname in key:

self.reference = {'*': val}

break

else:

raise ValueError(

f'could not find a reference for the current '

f'partition {self.current_partition.fullname!r}'

)

Note that the keys of REFERENCE_CPU_PERFORMANCE_LARGE also must be made tuples in order for this to work.

Why? Thank to string if self.current_partition.fullname in key: we check either substring in the string (as in case REFERENCE_CPU_PERFORMANCE_LARGE) or element in a tuple

vkarak · 2021-10-29T10:05:40Z

Replaced by individual PRs.

hurricane642 added 6 commits July 15, 2021 01:40

add amber tests

3a1b41e

moved the download function

e494af4

Merge pull request #1 from eth-cscs/master

abe0a88

update reframe to 3.7.1 version

add lammps tests

dd62c74

update lammps test

4b5e94c

update lammps test

b38a912

sanity update

ec16d43

jjotero requested review from jjotero, victorusu and vkarak July 20, 2021 15:59

jjotero assigned hurricane642 Jul 20, 2021

jjotero added prio: normal regression test labels Jul 20, 2021

hurricane642 added 3 commits July 20, 2021 20:01

sanity checks

862e319

refactor the code organization and their position in the repo

7311311

sanity commit

90466eb

jjotero suggested changes Jul 23, 2021

View reviewed changes

cscs-checks/apps/amber/amber.py Outdated Show resolved Hide resolved

jjotero suggested changes Jul 26, 2021

View reviewed changes

reformat the test organization

adee473

hurricane642 force-pushed the master branch from 9f05926 to adee473 Compare July 26, 2021 13:00

sanity commit

60d0216

jjotero suggested changes Jul 26, 2021

View reviewed changes

change the base test structure

411da80

hurricane642 force-pushed the master branch from 3790a46 to 411da80 Compare July 26, 2021 18:28

sanity commit

73d0223

hurricane642 added 18 commits August 26, 2021 10:55

fix indentation in the amber_base

c6bc455

fix indentation in the amber_base

4f1af80

fix indentation in the amber_base

337ffbc

fix indentation in the amber_base

60db205

fix indentation in the amber_base

2cc73aa

fix indentation in the cp2k_base

8c8d510

fix indentation in the cp2k_base

52e94da

fix indentation in the cp2k_base

89c246d

fix indentation in the cp2k_base

06e2f2d

fix indentation in the cp2k_base

5d7f3cb

fix indentation in the cpmd_base

ade2344

fix indentation in the cpmd_base

d2dd340

fix indentation in the lammps_base

c9d567f

fix indentation in the quantumespresso_base

9be57da

fix indentation in the vasp_base

817802b

minor changes in order of variables and comments

a0a2d76

fix the number of blank lines

1589e9f

change the format of reference dicts in order to compress them

d48ca44

jjotero suggested changes Sep 1, 2021

View reviewed changes

another version of work wirh performance reference

e23bdd5

hurricane642 mentioned this pull request Sep 1, 2021

[test] Add new Amber NVE library test and refactor the CSCS Amber check #2172

Merged

jjotero removed this from the ReFrame Sprint 21.08.2 milestone Sep 8, 2021

vkarak closed this Oct 29, 2021

-    @run_after('setup')
-    def set_reference_dict(self):
-        self.REFERENCE_DICT = REFERENCE_CPU_PERFORMANCE[self.scale]
+    @run_before('performance')
+    def set_perf_reference(self):
+        for key, val in REFERENCE_CPU_PERFORMANCE[self.scale].items():
+            if self.current_partition.fullname in key:
+                self.reference = {'*': val}
+                break
+        else:
+            raise ValueError(
+                f'could not find a reference for the current '
+                f'partition {self.current_partition.fullname!r}'
+            )

[test] Add generic checks for the cscs-supported apps into hpctestlib/apps #2084

[test] Add generic checks for the cscs-supported apps into hpctestlib/apps #2084

Uh oh!

Conversation

hurricane642 commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-09-01 12:12:53 UTC

Uh oh!

jenkins-cscs commented Jul 20, 2021

Uh oh!

jjotero commented Jul 20, 2021

Uh oh!

codecov-commenter commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jjotero commented Jul 20, 2021

Uh oh!

jjotero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jjotero left a comment

Choose a reason for hiding this comment

Uh oh!

jjotero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjotero Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

jjotero Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

jjotero Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

jjotero Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

hurricane642 Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

vkarak commented Oct 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[test] Add generic checks for the cscs-supported apps into `hpctestlib/apps` #2084

[test] Add generic checks for the cscs-supported apps into `hpctestlib/apps` #2084

hurricane642 commented Jul 20, 2021 •

edited

Loading

pep8speaks commented Jul 20, 2021 •

edited

Loading

codecov-commenter commented Jul 20, 2021 •

edited

Loading