add support for enhancing existing sanity check by boegel · Pull Request #3288 · easybuilders/easybuild-framework

boegel · 2020-04-17T18:05:12Z

Following the issue that was raised by @ocaisa in easybuilders/easybuild-easyconfigs#10418, where defining sanity_check_commands in a LAMMPS easyconfig causes other checks that are run by the easyblock to be skipped, I think it's time to add support for specifying that the sanity_check_commands and sanity_check_paths specified in the easyconfig file should be run in addition to what the easyblock already checks, rather than overriding them.

This PR adds support for doing so, via:

enhance_sanity_check = True

…er tests by doing proper cleanup w.r.t. included toy easyblock

lexming

This works very well. I tested the enhance_sanity_check option with a stripped down version of LAMMPS-3Mar2020-intel-2019b-Python-3.7.4-kokkos.eb from PR easybuilders/easybuild-easyconfigs#10418 and it worked as intended in the following cases

enhance_sanity_check = False: sanity checks included the files from the easyblock and the command from the EC (ignoring commands from easyblock)
enhance_sanity_check = True: sanity checks included the files from the easyblock and the commands from both the EC and the easyblock
enhance_sanity_check = True and removing the sanity check command from the EC: sanity checks included the files from the easyblock and it did perform the commands from the easyblock
enhance_sanity_check = True, add sanity_check_files to EC and remove all sanity check command from the EC: sanity checks added the files from the EC to those set by the easyblock and it did perform the commands set in the easyblock

The unit test test.framework.toy_build also went fine and it successfully finished.

I only have one remark, I think that it is counter-intuitive having to forcefully add and empty dirs to sanity_check_files when we only want to add files to the sanity check with enhance_sanity_check = True. Adding an empty dirs does not break the sanity checks, it just gets added to the list of paths, but I would make it mandatory only if enhance_sanity_check = False.

…n enhance_sanity_check_paths is enabled + bug fix under dry run for tuple entries in sanity_check_paths

boegel · 2020-04-20T08:19:47Z

I only have one remark, I think that it is counter-intuitive having to forcefully add and empty dirs to sanity_check_files when we only want to add files to the sanity check with enhance_sanity_check = True. Adding an empty dirs does not break the sanity checks, it just gets added to the list of paths, but I would make it mandatory only if enhance_sanity_check = False.

@lexming That restriction is actually lifted implicitly when enhanced_sanity_check is enabled, because either the sanity_check_paths from the easyblock or the default one (bin + lib/lib64) will be enhanced, so both files and dirs will already be there, implying that only specifying files or keys is fine as long as enhanced_sanity_check = True is used...

I've covered that in the test added in 4ee9d25, and also fixed two minor bugs I ran into because of adding that test in 23167d5...

lexming · 2020-04-20T17:52:41Z

@boegel this is not the case on my side. For instance, if I add to LAMMPS-3Mar2020-intel-2019b-Python-3.7.4-kokkos.eb the following sanity_check_paths

enhance_sanity_check = True
 
sanity_check_paths = {                                                                                                                                                   
    'files': ['log.lammps'],
}

I get the following error

$ ebt LAMMPS-3Mar2020-intel-2019b-Python-3.7.4-kokkos.eb
== temporary log file in case of crash /local/3036178.master01.hydra.brussel.vsc/eb-RENhlb/easybuild-DDvnmA.log
== resolving dependencies ...
== processing EasyBuild easyconfig /theia/home/brussel/101/vsc10122/src/easybuild-easyconfigs/easybuild/easyconfigs/LAMMPS-3Mar2020-intel-2019b-Python-3.7.4-kokkos.eb
== FAILED: Installation ended unsuccessfully (build directory: /user/brussel/101/vsc10122/.local/easybuild-ivybridge/build/LAMMPS/3Mar2020/intel-2019b-Python-3.7.4-kokkos): build failed (first 300 chars): Missing mandatory key 'dirs' in sanity_check_paths. (took 0 sec)
== Results of the build can be found in the log file(s) /local/3036178.master01.hydra.brussel.vsc/eb-RENhlb/easybuild-LAMMPS-3Mar2020-20200420.194814.YSvyG.log
ERROR: Build of /theia/home/brussel/101/vsc10122/src/easybuild-easyconfigs/easybuild/easyconfigs/LAMMPS-3Mar2020-intel-2019b-Python-3.7.4-kokkos.eb failed (err: "build failed (first 300 chars): Missing mandatory key 'dirs' in sanity_check_paths.")

edit: I forgot to mention that I tested it with the last commits from this PR

…FormatZeroOne._reformat_line

boegel · 2020-04-20T18:55:48Z

@lexming Ah, that's failing in an entirely different part of the code. Apparently we have that same restriction in two places...

I like to keep it there though, for the case where enhance_sanity_check is not enabled.

I've reproduced your issue in a test and fixed the problem in 96ec4e9 .

lexming

LGTM

sanity_check_paths = {'files': ['foo.bar']} in test EC
- enhance_sanity_check = False FAIL (as intended). Sanity checks only include paths from EC and these do not fulfil the required keys in sanity_check_paths. Adding {'dirs': ''} to sanity_check_paths makes the checks to pass.
- enhance_sanity_check = True PASS. Sanity checks include paths set by easyblock/default and the single file added from EC
sanity_check_commands = ["touch foo.bar"] in test EC
- enhance_sanity_check = False PASS. Sanity checks only include the touch command from EC
- enhance_sanity_check = True PASS. Sanity checks include commands set by easyblock/default and taht from EC

Any combination of the above sanity_check_paths and sanity_check_commands works in the same manner, as expected.

I also tested PyGTK-2.24.0-foss-2018b-Python-2.7.15.eb, which has sanity_check_commands with multiple tuple elements. It works as expected in both cases of enhance_sanity_check
- enhance_sanity_check = False PASS.
== sanity checking...
file 'lib/pkgconfig/pygtk-2.0.pc' found: OK
(non-empty) directory 'lib/pygtk' found: OK
running command 'python -c 'import gtk'': OK
running command 'python -c 'import gtk.glade'': OK
- enhance_sanity_check = True PASS.
== sanity checking...
file 'lib/pkgconfig/pygtk-2.0.pc' found: OK
(non-empty) directory 'bin' found: OK
(non-empty) directory 'lib' or 'lib64' found: OK
(non-empty) directory 'lib/pygtk' found: OK
running command 'python -c 'import gtk'': OK
running command 'python -c 'import gtk.glade'': OK
Unit tests
- test.framework.toy_build: PASS
- test.framework.easyblock: PASS

… also if test fails

lexming

LGTM

lexming · 2020-04-21T08:51:31Z

Going in, thanks @boegel !

…easybuild-framework#3288)

boegel added the enhancement label Apr 17, 2020

boegel added this to the next release (4.2.1?) milestone Apr 17, 2020

boegel mentioned this pull request Apr 17, 2020

disable USER-INTEL package in LAMMPS easyconfigs using intel/2019b, since it results in an installation that produces incorrect results easybuilders/easybuild-easyconfigs#10418

Merged

add support for enhancing existing sanity check

f8844f7

boegel force-pushed the enhance_sanity_check branch from 73f7993 to f8844f7 Compare April 17, 2020 18:07

easybuilders deleted a comment from boegelbot Apr 18, 2020

avoid that test_toy_build_enhanced_sanity_check causes trouble in oth…

184791d

…er tests by doing proper cleanup w.r.t. included toy easyblock

boegel force-pushed the enhance_sanity_check branch from 365b675 to 184791d Compare April 18, 2020 16:13

lexming reviewed Apr 19, 2020

View reviewed changes

boegel added 2 commits April 20, 2020 10:16

take into account that sanity_check_paths values may not be lists whe…

23167d5

…n enhance_sanity_check_paths is enabled + bug fix under dry run for tuple entries in sanity_check_paths

add test for verification of sanity_check_paths

4ee9d25

don't strictly require both files/dirs keys in sanity_check_paths in …

96ec4e9

…FormatZeroOne._reformat_line

lexming previously approved these changes Apr 20, 2020

View reviewed changes

boegel added 2 commits April 21, 2020 07:50

ensure sorted output in sanity check step under dry run

4130980

move cleanup of toy easyblock to tearDown, to ensure it's always run,…

b89b8f4

… also if test fails

boegel dismissed lexming’s stale review via b89b8f4 April 21, 2020 06:19

also clean up toytoy easyblock in tearDown of toy_build tests

8844dde

easybuilders deleted a comment from boegelbot Apr 21, 2020

lexming approved these changes Apr 21, 2020

View reviewed changes

lexming merged commit 7c1c288 into easybuilders:develop Apr 21, 2020

boegel deleted the enhance_sanity_check branch April 21, 2020 15:33

boegel added a commit to boegel/easybuild-easyconfigs that referenced this pull request Apr 22, 2020

enable enhance_sanity_check in LAMMPS easyconfigs (cfr. easybuilders/…

4394844

…easybuild-framework#3288)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for enhancing existing sanity check#3288

add support for enhancing existing sanity check#3288
lexming merged 8 commits intoeasybuilders:developfrom
boegel:enhance_sanity_check

boegel commented Apr 17, 2020

Uh oh!

lexming left a comment

Uh oh!

boegel commented Apr 20, 2020

Uh oh!

lexming commented Apr 20, 2020 •

edited

Loading

Uh oh!

boegel commented Apr 20, 2020

Uh oh!

lexming left a comment

Uh oh!

lexming left a comment

Uh oh!

lexming commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

boegel commented Apr 17, 2020

Uh oh!

lexming left a comment

Choose a reason for hiding this comment

Uh oh!

boegel commented Apr 20, 2020

Uh oh!

lexming commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boegel commented Apr 20, 2020

Uh oh!

lexming left a comment

Choose a reason for hiding this comment

Uh oh!

lexming left a comment

Choose a reason for hiding this comment

Uh oh!

lexming commented Apr 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lexming commented Apr 20, 2020 •

edited

Loading