-
Notifications
You must be signed in to change notification settings - Fork 283
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
Enhance generic Bundle
easyblock to transfer module requirements of components
#3472
Enhance generic Bundle
easyblock to transfer module requirements of components
#3472
Conversation
I've only tested the changes with easybuilders/easybuild-easyconfigs#21582 so far. I'll try to gather the EasyConfigs using Bundle in some way to make sure that I did not break them, since this is a change on a widely used EasyBlock. Unfortunately, I can only test GCC 12.3.0 and newer easily and are somewhat limited with my compute resources. So this might take a while.. |
Example with the changed EasyBlock: |
Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) |
Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 25 out of 26 (7 easyconfigs in total) The failed one doesn't look related:
The tested ones include normal |
Changed PR so that the EasyBlock for each component is stored and not initialized in multiple steps. |
Based on https://docs.easybuild.io/version-specific/easyblocks/?h=bundle, this change should affect the following EasyBlocks:
I will not be able to test all of them (e.g. CrayToolchain), but I'll try to get one EasyConfig for all of these EasyBlocks to make sure that nothing broke and compare their modules. |
I've chosen a few EasyConfigs to compare with this MR and the current develop.
Building will take quite some time. I'll post the resulting module diffs later. |
Now that all modules are finally installed (but I had to partially select other modules due to general installation issues), I found one bug with the proposed changes: When checking crypt4gh-1.7-GCC-12.3.0.eb, the module does a sanity check for an executable in help([==[
Description
===========
crypt4gh is a Python tool to encrypt, decrypt or re-encrypt files,
according to the GA4GH encryption file format.
More information
================
- Homepage: https://github.com/EGA-archive/crypt4gh
Included extensions
===================
crypt4gh-1.7
]==])
whatis([==[Description: crypt4gh is a Python tool to encrypt, decrypt or re-encrypt files,
according to the GA4GH encryption file format.]==])
whatis([==[Homepage: https://github.com/EGA-archive/crypt4gh]==])
whatis([==[URL: https://github.com/EGA-archive/crypt4gh]==])
whatis([==[Extensions: crypt4gh-1.7]==])
local root = "/tank/Programs/Linux/EasyBuild/datenlager/Software/software/crypt4gh/1.7-GCC-12.3.0"
conflict("crypt4gh")
if not ( isloaded("GCC/12.3.0") ) then
load("GCC/12.3.0")
end
if not ( isloaded("Python/3.11.3-GCCcore-12.3.0") ) then
load("Python/3.11.3-GCCcore-12.3.0")
end
if not ( isloaded("Python-bundle-PyPI/2023.06-GCCcore-12.3.0") ) then
load("Python-bundle-PyPI/2023.06-GCCcore-12.3.0")
end
if not ( isloaded("PyYAML/6.0-GCCcore-12.3.0") ) then
load("PyYAML/6.0-GCCcore-12.3.0")
end
if not ( isloaded("cryptography/41.0.1-GCCcore-12.3.0") ) then
load("cryptography/41.0.1-GCCcore-12.3.0")
end
if not ( isloaded("bcrypt/4.0.1-GCCcore-12.3.0") ) then
load("bcrypt/4.0.1-GCCcore-12.3.0")
end
prepend_path("CMAKE_PREFIX_PATH", root)
prepend_path("LIBRARY_PATH", pathJoin(root, "lib"))
prepend_path("PATH", pathJoin(root, "bin"))
setenv("EBROOTCRYPT4GH", root)
setenv("EBVERSIONCRYPT4GH", "1.7")
setenv("EBDEVELCRYPT4GH", pathJoin(root, "easybuild/crypt4gh-1.7-GCC-12.3.0-easybuild-devel"))
prepend_path("PYTHONPATH", pathJoin(root, "lib/python3.11/site-packages"))
-- Built with EasyBuild version 4.9.5.dev0
setenv("EBEXTSLISTCRYPT4GH", "crypt4gh-1.7") but this fails with the proposed changes. |
07b4490
to
24cec43
Compare
Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 9 out of 9 (9 easyconfigs in total) I'm not able to test jiter-0.4.1-GCCcore-12.3.0.eb due to the following error caused by
I've also checked if the module files of the EasyConfigs above changed. They did not. |
24cec43
to
5e3d7e4
Compare
Full (re-)installation Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 9 out of 9 (9 easyconfigs in total) |
Module only Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 9 out of 9 (9 easyconfigs in total) |
Testing a few of the Stages 2025 EasyConfigs using Bundle/PythonBundle/... of the JSC repo Test report by @Thyre Overview of tested easyconfigs (in order)
Build succeeded for 5 out of 5 (5 easyconfigs in total) |
Bundle
easyblock to transfer module requirements of components
Bi-weekly EasyBuild conf call 2024-11-06: While these changes continue to work with EasyBuild 5.0.x, the My current idea would be to continue with this PR as it is. Once easybuilders/easybuild-framework#4653 is merged, I'll work on porting these changes to the new method for EasyBuild 5.0.x. |
@boegelbot please test @ jsc-zen3 |
@Thyre: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2464270943 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 9 out of 9 (9 easyconfigs in total) |
@boegelbot please test @ jsc-zen3 |
@Thyre: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2464420612 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 9 out of 9 (9 easyconfigs in total) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I only have a few comments to improve the code a bit. See below
Thanks a lot for the suggestions. I'll add them to the commit in a moment 😄 |
…omponents This commit implements the `make_module_req_guess` method for the generic Bundle EasyBlock. With this, all the requirements of the components in a bundle are transferred correctly to the final module. Previously, this could lead to missing environment variables, letting the build succeed but still resulting in a broken module, for example because `PATH` is not set. Signed-off-by: Jan André Reuter <[email protected]>
8247d63
to
957ee53
Compare
Instead of zipping bundles and configs every time, use a single instance list. Also, do not check for the type when building the module requirements and catch the exception instead. Signed-off-by: Jan André Reuter <[email protected]>
957ee53
to
eda3ca7
Compare
@boegelbot please test @ jsc-zen3 |
@Thyre: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2464855921 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 9 out of 9 (9 easyconfigs in total) |
@Thyre thanks for the quick update! do you want to add your name to the list of contributors to this easyblock? If you do, there is no need to send new tests again 🙂 |
Signed-off-by: Jan André Reuter <[email protected]>
Done. Took longer than I want to admit to find the authors comment 😅
Agreed, changing the comment shouldn't break anything. Just wanted to make sure nothing broke during the last changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merging, thanks @Thyre ! |
# list of EasyConfig instances for components | ||
self.comp_cfgs = [] | ||
# list of EasyConfig instances and their EasyBlocks for components | ||
self.comp_instances = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Thyre renaming this will have some fallout, see the clang_aomp.py
where self.comp_cfgs
is used...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we should keep the old one, yeah. With the PR reverted, I revise the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I missed that EasyBlock since it is unused. However, there is only a single usage of self.comp_cfgs
.
When I open a new PR for this, I'll update the clang_aomp.py
EasyBlock as well. Haven't found other usages.
|
||
for cfg, comp in self.comp_instances: | ||
self.log.info("Gathering module paths for component %s v%s", cfg['name'], cfg['version']) | ||
reqs = comp.make_module_req_guess() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if one of the component itself is a Bundle
instance, that may result in an infinite loop I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... a Bundle
instance should gather the requirements of the components. If one of the components is also a Bundle
, we should collect all of them recursively.
An infinite loop can be achieved if one EasyBlock tries to call this function via super
. Can we prevent that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boegel I don't see why there would be an infinite loop here, the list of components is finite and equal to whatever the easyconfig defines. Even if there is a bundle in the component list, it is also defined in the easyconfig.
This is not like looping over dependencies, with multiple layers of depth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a Bundle
as component of another Bundle
is explicitly forbidden. See
raise EasyBuildError("The Bundle easyblock can not be used to install components in a bundle") |
So this is not the cause of the issue (unless some unit test does some weird stuff and hijacks that check in
Bundle
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently there is a general bug in this check, as it does not cover easyblocks based on Bundle
, like PythonBundle
or OpenSSL_wrapper
. So that is one thing that can improve.
However, it is already impossible to make a nested bundle in practice because the resulting construct always errors out. If the bundle component has sources, it results in error about "bundles cannot have sources"; and if it does not have sources
it results in error about "missing sources".
…nfinite loop in easyconfigs test suite?
I've opened #3504 to revert the changes that were done here. |
revert changes from PR #3472, seems to be leading to an infinite loop in easyconfigs test suite?
@@ -198,7 +199,7 @@ def __init__(self, *args, **kwargs): | |||
if comp_cfg['patches']: | |||
self.cfg.update('patches', comp_cfg['patches']) | |||
|
|||
self.comp_cfgs.append(comp_cfg) | |||
self.comp_instances.append((comp_cfg, comp_cfg.easyblock(comp_cfg))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test suite is now stuck for QuantumESPRESSO. That's a huge progress (since we were previously stuck even for the first few EasyConfigs with A), but still not a complete fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The QuantumESPRESSO check is not stuck. Instead, the logging files are not closed and all (?) tests after that are logged as well, causing around 11GB logs on my system. This also happens with develop
as far as I can tell.
total 11G
drwx------ 2 jreuter jreuter 800 12. Nov 20:44 .
drwxrwxrwt 24 root root 540 12. Nov 20:36 ..
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-5.3.0-20241112.203827.pyJFM.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-5.4.0-20241112.203827.FyduX.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-5.4.0-20241112.203827.hzkuy.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.0-20241112.203827.FzleV.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.1-20241112.203827.FZgWV.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.2.1-20241112.203827.xMxaO.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.2-20241112.203827.fABEs.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.3-20241112.203827.kaPJh.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.3-20241112.203827.nZhTm.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.4.1-20241112.203827.juhTP.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.5-20241112.203827.pfDnY.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.5-20241112.203827.VBbnI.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.6-20241112.203827.bRTCU.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.6-20241112.203827.ewZNc.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.6-20241112.203827.IzdWy.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.6-20241112.203827.PgGZc.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.6-20241112.203827.vwIEO.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.7-20241112.203827.KUUtU.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.7-20241112.203827.piJmd.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.7-20241112.203827.rQAjF.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.7-20241112.203827.sbJUP.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.7-20241112.203827.ZBRpm.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.7-20241112.203827.ZSOpP.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.8-20241112.203827.cKErR.log
-rw-r--r-- 1 jreuter jreuter 298M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.8-20241112.203827.wwDpe.log
-rw-r--r-- 1 jreuter jreuter 299M 12. Nov 20:45 easybuild-QuantumESPRESSO-6.8-20241112.203827.XATBJ.log
-rw-r--r-- 1 jreuter jreuter 298M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.0-20241112.203827.KuImR.log
-rw-r--r-- 1 jreuter jreuter 298M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.0-20241112.203827.SXBiL.log
-rw-r--r-- 1 jreuter jreuter 298M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.1-20241112.203827.BWcVK.log
-rw-r--r-- 1 jreuter jreuter 298M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.1-20241112.203827.tpQDP.log
-rw-r--r-- 1 jreuter jreuter 298M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.2-20241112.203827.GrtNk.log
-rw-r--r-- 1 jreuter jreuter 298M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.2-20241112.203827.MArZe.log
-rw-r--r-- 1 jreuter jreuter 283M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.2-20241112.203828.qrsQj.log
-rw-r--r-- 1 jreuter jreuter 283M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.3.1-20241112.203828.gMpKu.log
-rw-r--r-- 1 jreuter jreuter 283M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.3.1-20241112.203828.kpFyU.log
-rw-r--r-- 1 jreuter jreuter 283M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.3.1-20241112.203828.lrPYN.log
-rw-r--r-- 1 jreuter jreuter 283M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.3-20241112.203828.TOcVA.log
-rw-r--r-- 1 jreuter jreuter 283M 12. Nov 20:45 easybuild-QuantumESPRESSO-7.3-20241112.203828.wnnQg.log
Could this be related to the way the EasyBlock is written?
@Thyre the changes in this PR are still alive in |
@lexming We found the issue, see easybuilders/easybuild-easyconfigs#21841. It basically just boils down to loggers not being closed during the EasyConfig tests. I'll prepare an updated PR with a fix for this (and the comment regarding |
Signed-off-by: Jan André Reuter <[email protected]>
This PR implements the
make_module_req_guess
method for the generic Bundle EasyBlock. With this, all the requirements of the components in a bundle are transferred correctly to the final module. Previously, this could lead to missing environment variables, letting the build succeed but still resulting in a broken module, for example becausePATH
is not set.Fixes #2733