Skip to content

Conversation

@boegel
Copy link
Member

@boegel boegel commented Apr 17, 2020

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

@boegel boegel force-pushed the enhance_sanity_check branch from 73f7993 to f8844f7 Compare April 17, 2020 18:07
@easybuilders easybuilders deleted a comment from boegelbot Apr 18, 2020
…er tests by doing proper cleanup w.r.t. included toy easyblock
@boegel boegel force-pushed the enhance_sanity_check branch from 365b675 to 184791d Compare April 18, 2020 16:13
Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

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.

boegel added 2 commits April 20, 2020 10:16
…n enhance_sanity_check_paths is enabled + bug fix under dry run for tuple entries in sanity_check_paths
@boegel
Copy link
Member Author

boegel commented Apr 20, 2020

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
Copy link
Contributor

lexming commented Apr 20, 2020

@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

@boegel
Copy link
Member Author

boegel commented Apr 20, 2020

@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
lexming previously approved these changes Apr 20, 2020
Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

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

@easybuilders easybuilders deleted a comment from boegelbot Apr 21, 2020
Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

LGTM

@lexming
Copy link
Contributor

lexming commented Apr 21, 2020

Going in, thanks @boegel !

@lexming lexming merged commit 7c1c288 into easybuilders:develop Apr 21, 2020
@boegel 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants