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

Make generic EasyBlock usable to install software #4531

Open
wants to merge 30 commits into
base: 5.0.x
Choose a base branch
from

Conversation

lexming
Copy link
Contributor

@lexming lexming commented May 13, 2024

This is quite a big change, it makes the base EasyBlock class usable by adding generic implementations of the missing configure, build and install step.

This is how this works:

  • easyconfigs that define an easyblock will use that one
  • easyconfigs with a name that matches an existing easyblock will use that one (i.e. EB_name)
  • easyconfigs without any specific easyblock will use EasyBlock

Framework will define for each step of the installation the following parameters:

  • pre_[step]_cmds: list of commands to execute before the main command of the step (analogue in mechanics to postinstallcmds)
  • pre[step]opts: string to be prepended to main command of the step
  • [step]_cmd: main command of the step
  • [step]opts: string to append to main command of the step

Easyconfigs can then define any of the [step]_cmd and then EasyBlock will carry out that step. Otherwise the step is skipped.

Motivation for this change is two-fold:

  • It becomes much easier to make easyconfigs for non-standard software without their own easyblock:
    Basically EasyBlock becomes a CmdCmdCmd easyblock. For instance, I recently had to install something with Bazel that needed a custom ./configure.py for the configure step and bazel build in the build step. With this approach I can do that installation without having to write a new easyblock or hacking other easyblocks not meant for this task.
  • Provides a standard implementation on how easyblock should carry out each build step:
    I initially only implemented the addition of pre[step]cmds parameters to execute commands at the beginning of each step. However, it is extremely weird to define those parameters in framework without any implementation. Custom easyblocks are then free to do whatever they like with those parameters. This PR adds a run_main_step_action method that serves as a reference on how all the parameters should be combined and executed.

Changelog:

  • add easyconfig parameters: pre_configure_cmds, pre_build_cmds, pre_test_cmds, pre_install_cmds
  • add easyconfig parameters: configure_cmd, build_cmd, test_cmd, install_cmd
  • add method easyblock._run_command_stack()
  • implement easyblock.configure_step() using easyblock._run_command_stack()
  • implement easyblock.build_step() using easyblock._run_command_stack()
  • implement easyblock.install_step() using easyblock._run_command_stack()
  • update easyblock.test_step() using easyblock._run_command_stack()
  • update easyblock.run_post_install_commands() using easyblock._run_command_stack()
  • update easyblock.get_easyblock_class() behaviour as follows:
    • easyconfigs that define an easyblock will use that one
    • easyconfigs with a name that matches an existing easyblock will use that one (i.e. EB_name)
    • easyconfigs without any specific easyblock will use EasyBlock
  • remove unused error_on_failed_import attribute from easyblock.get_easyblock_class()
  • deprecate easyconfig parameter runtest in favor of test_cmd
  • standardize format of help strings of all easyconfig parameters

@lexming lexming added the EasyBuild-5.0 EasyBuild 5.0 label May 13, 2024
@boegel boegel added the change label May 21, 2024
@boegel boegel added this to the 5.0 milestone May 21, 2024
@boegel boegel added enhancement and removed change labels May 22, 2024
@boegel
Copy link
Member

boegel commented May 22, 2024

I need to give this a detailed review, but in general I quite like this idea to support "easyblock-less" installations.

It lowers the bar to entry for people since this may avoid the need to implement a custom easyblock for simple stuff.

It does perhaps also introduce a risk to encourage producing messier easyconfigs, but it's not like we don't have those already...

@lexming
Copy link
Contributor Author

lexming commented Jun 7, 2024

A perfect use case for this PR is easybuilders/easybuild-easyconfigs#20693

@lexming lexming changed the title WIP: Make generic EasyBlock usable Make generic EasyBlock usable to install software Jun 12, 2024
@lexming
Copy link
Contributor Author

lexming commented Jun 12, 2024

Example easyconfig using EasyBlock:

name = 'CORSIKA'
version = '77550'

homepage = "https://www.iap.kit.edu/corsika"
description = """CORSIKA (COsmic Ray SImulations for KAscade) is a program for detailed
simulation of extensive air showers initiated by high energy cosmic ray
particles. Protons, light nuclei up to iron, photons, and many other particles
may be treated as primaries."""

toolchain = {'name': 'foss', 'version': '2023a'}
toolchainopts = {'usempi': True}

download_instructions = "Sources have to be requested to the developers"
sources = [SOURCELOWER_TAR_GZ]
checksums = ['fed74c144e22deb5a7c1d2dc1f04f0100eb2732cb48665a3da49ce471a3775ee']

dependencies = [
    ("ROOT", "6.30.06"),
]

# custom coconut script does not recognize -j
parallel = False

# execute ./coconut manually with your own options and extract configure command from top of config.log
configure_cmd = "./configure"
configopts = "CORDETECTOR=HORIZONTAL CORTIMELIB=TIMEAUTO CORHEMODEL=QGSJETII CORLEMODEL=URQMD "
configopts += "--enable-UPWARD --enable-SLANT --enable-THIN --enable-COREAS "
configopts += "--enable-PARALLEL --with-mpirunner-lib --enable-PARALLELIB "

build_cmd = "./coconut"
buildopts = "--batch"

preinstallopts = "mkdir -p %(installdir)s/bin && "
install_cmd = "cp"
installopts = "%(builddir)s/%(namelower)s-%(version)s/run/%(namelower)s%(version)s* %(installdir)s/bin/"

sanity_check_paths = {
    'files': ['bin/corsika77550Linux_QGSII_urqmd_thin_coreas_parallel'],
    'dirs': []
}

moduleclass = "phys"

Test report for this PR with this example easyconfig:
https://gist.github.com/lexming/776d52d61b8bdce7aa47c068fe092af4

self.log.debug(f"Trying to execute {unit_test_cmd} as a command for running unit tests...")
res = run_shell_cmd(unit_test_cmd)
if self.cfg['runtest'] is not None:
self.log.deprecated("Easyconfig parameter 'runtest' is deprecated, use 'test_cmd' instead.", "6.0")
Copy link
Member

Choose a reason for hiding this comment

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

@lexming This implies updating all easyblocks/easyconfigs to use test_cmd instead of runtest before we release EasyBuild 5.0, so please open an issue on that in both repos and tag it for EasyBuild 5.0

Copy link
Member

Choose a reason for hiding this comment

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

Also, what if both test_cmd and runtest are set?

Either we error out, or we let one overrule the other (with logging).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I suggest to not error out and just ignore runtest if both are defined (with a warning ofc). Fixed in 22d9648

easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
Comment on lines 2750 to 2753
"configure",
self.cfg['pre_configure_cmds'],
self.cfg['preconfigopts'],
self.cfg['configopts'],
Copy link
Member

Choose a reason for hiding this comment

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

This looks kind of weird without using named options, since pre_configure_cmd is specified after configure_cmd, so at the very least this should become:

Suggested change
"configure",
self.cfg['pre_configure_cmds'],
self.cfg['preconfigopts'],
self.cfg['configopts'],
'configure',
pre_cmds=self.cfg['pre_configure_cmds'],
pre_opts=self.cfg['preconfigopts'],
post_opts=self.cfg['configopts'],

Even better would be to use only named options, so also use action='self.cfg['configure_cmd']' and action='configure' (and making those both None by default in run_step_main_action and mandatory with an is None check in there.

Also, maybe rename run_step_main_action to run_core_step, and only pass a string like configure, let run_core_step figure out the rest (to use configure_cmd, preconfigopts, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, it is indeed easy to construct the commands from just the step name. Done in e05a985

self.log.debug(f"Trying to execute {unit_test_cmd} as a command for running unit tests...")
res = run_shell_cmd(unit_test_cmd)
if self.cfg['runtest'] is not None:
self.log.deprecated("Easyconfig parameter 'runtest' is deprecated, use 'test_cmd' instead.", "6.0")
Copy link
Member

Choose a reason for hiding this comment

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

Also, what if both test_cmd and runtest are set?

Either we error out, or we let one overrule the other (with logging).

easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
@@ -1858,83 +1860,72 @@ def det_installversion(version, toolchain_name, toolchain_version, prefix, suffi
_log.nosupport('Use det_full_ec_version from easybuild.tools.module_generator instead of %s' % old_fn, '2.0')


def get_easyblock_class(easyblock, name=None, error_on_failed_import=True, error_on_missing_easyblock=True, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove error_on_failed_import, that's API breakage.

At best deprecate it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we can actually break the API between major versions 😉 I re-added it with a deprecation warning nonetheless. Done in eacd6e3

raise EasyBuildError(f"Software-specific easyblock '{class_name}' not found for {name}")
else:
_log.debug(f"Easyblock for {class_name} not found, but ignoring it: {err}")
else:
Copy link
Member

Choose a reason for hiding this comment

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

keep the elif error_on_failed_import here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in eacd6e3

lexming and others added 5 commits June 28, 2024 14:59
@lexming
Copy link
Contributor Author

lexming commented Jul 3, 2024

Test report with last commit and experimental easyconfig for CORSIKA in #4531 (comment)
https://gist.github.com/lexming/13af2faf00baa7f34b6923620b2015fe

== 2024-07-03 09:18:52,337 easyconfig.py:1906 INFO Using generic EasyBlock module for software 'CORSIKA'
== 2024-07-03 09:18:52,338 easyconfig.py:1935 INFO Successfully obtained class 'EasyBlock' for easyblock 'None' (software name 'CORSIKA')
== 2024-07-03 09:18:52,338 easyconfig.py:688 INFO Parsing easyconfig file /rhea/scratch/brussel/101/vsc10122/easybuild/easybuild-easyconfigs/easybuild/easyconfigs/CORSIKA-77550-foss-2023a.eb with rawcontent: name = 'CORSIKA'

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@lexming Ideally we also have a dedicated for this new feature, using the toy software we use in the tests for example, with an easyconfig where we define configure_cmd & co and we make sure that EasyBlock class is used

pre_step_cmds = self.cfg[step_cmd_cfgs[step][0]]
if pre_step_cmds:
try:
self._run_cmds(pre_step_cmds)
Copy link
Member

Choose a reason for hiding this comment

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

Don't we limit the usefulness of pre_configure_cmds by running those commands separately?

Something like export FOO=bar in pre_configure_cmds won't work as expected because it's run in a different shell than configure_cmd, which limits the usefulness of pre_configure_cmds quite a bit I think?

I can see the value though (better errors when one of the commands in pre_configure_cmds fails, for example), but we should try and make it very clear that these commands are running in a different subshell?

if self.cfg['test_cmd'] is None:
self.cfg['test_cmd'] = self.cfg['runtest']
else:
self.log.warning(
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we print a warning here (too), to avoid that this is overlooked easily?

@@ -88,9 +88,11 @@
"to be linked in any installed binary/library", BUILD],
'bitbucket_account': ['%(namelower)s', "Bitbucket account name to be used to resolve template values in source"
" URLs", BUILD],
'buildopts': ['', 'Extra options passed to make step (default already has -j X)', BUILD],
'buildopts': ['', 'Extra options appended to build command', BUILD],
'build_cmd': [None, "Main shell command used in the build step", BUILD],
Copy link
Member

Choose a reason for hiding this comment

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

This should also mention something like "(only used by default build step implemented by EasyBlock class)", or something like that?
As soon as a generic/custom easyblock is used, build_cmd will be totally ignored.
Same goes for configure, test, install

'postinstallcmds': [[], 'Commands to run after the install step.', BUILD],
'postinstallpatches': [[], 'Patch files to apply after running the install step.', BUILD],
'postinstallmsgs': [[], 'Messages to print after running the install step.', BUILD],
'pre_build_cmds': ['', 'Extra commands executed before main build command', BUILD],
Copy link
Member

Choose a reason for hiding this comment

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

Mention that these are run in a different subshell than the build command itself (no sharing of environment), and only when there's no generic/custom easyblock is used

Same for configure, test, install

Comment on lines +1922 to +1923
elif error_on_failed_import:
raise EasyBuildError(f"Failed to import '{class_name}' easyblock: {err}")
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't belong here, in this case the easyblock doesn't exist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants