easybuild-easyblocks
easybuild-easyblocks copied to clipboard
[5.0x] run pip check only once for PythonBundle
(created using eb --new-pr)
We have 2 checks in PythonPackage:
pip checkpip list-> Check for "0.0.0" versions
In PythonBundle those are run for every extension after the build of the whole EC even though running it once is enough because the result will always be the same.
This PR uses the following logic:
sanity_pip_check should be set at the top of PythonBundle and not for the individual extensions. Currently if any extension has it enabled the check will be run so it does not make sense to disable/enable it for individual extensions. PythonBundle passes its value for this to every extension as a default so a deprecation is added in case it gets changed in an extension.
Similar reasoning applies to unversioned_packages: Only a single value for the whole bundle is useful and hence should be set at the top. For kind of backwards compatibility during the deprecation an union of all those values is used in the check.
PythonPackage does no longer do the pip checks if it is an extension and the parent EC (e.g. PythonBundle) has a value for sanity_pip_check set.
PythonBundle does the pip check if itself or any extension has requested it issuing a deprecation if something differs.
Refactoring
To make this possible some refactoring was required.
This makes the diff look large although it is mostly moved code. Explanation follows to help navigate the changes
run_pip_checkis moved out ofsanity_check_stepofPythonPackagesuch that it can be used byPythonBundle- This required moving the dependent method
det_installed_python_packagesout of the class too, the originalPythonPackage.get_installed_python_packagesneeds to stay for backwards compatibility which prevents giving the same name to the free function. Maybe in EB 5 we can remove it and useget_installed_python_packagesfor the global method?det_-prefix is chosen similar todet_py_libdirs PythonBundle.sanity_check_stepnow requirespython_cmdto be available which was only set in theprepare_stepthat is skipped in--sanity-check-only--> Factor outprepare_pythonfromprepare_stepsimilar toPythonPackage- There was a mismatch in the code to detect the
pythoncommand to use although I see no reason for that. I factored outfind_python_cmdfromPythonPackage.prepare_pythonand call it fromPythonBundle. I left the check for a loadedPythonmodule inPythonBundleas I don't know the reason for that check. IMO it should either be in both or neither
Fixes #3418
I overwrite _sanity_check_step_extensions now for this. This also ensures that the extensions are initialized. Related PR: https://github.com/easybuilders/easybuild-framework/pull/4620
@Flamefire Can you look into fixing the merge conflicts?
I'm keen on getting this merged soon, but there's a lot of code shuffling going on here that makes the review a bit tough...
Ok, the merge conflict mostly originated from the addition of a max-Python version. I added that to the moved code.
I split up the change into one commit that should only be a refactoring without any effective changes, then the actual change(s)
While doing the refactoring I noticed some weirdness with specifying the required Python version in ECs using the system Python dependency:
- If only the
req_py_majveris set, the minor version will be set to the minor version of the used python which doesn't make sense - The check for the max version when the minor version is missing fails almost always
I fixed both in separate commits to avoid having to test this code again.
I can split this into 3 PRs though if preferred (refactoring, pip-check, pyver fixes)
I copied the refactoring to https://github.com/easybuilders/easybuild-easyblocks/pull/3475 for easier review
Both tested with a random selection of recent-ish PythonBundle and PythonPackage ECs
Test report by @Flamefire
Overview of tested easyconfigs (in order)
- SUCCESS Flask-3.0.3-GCCcore-13.3.0.eb
- SUCCESS flit-3.9.0-GCCcore-13.2.0.eb
- SUCCESS Mako-1.2.4-GCCcore-13.2.0.eb
- SUCCESS pytest-workflow-2.1.0-GCCcore-13.3.0.eb
- SUCCESS poetry-1.8.3-GCCcore-13.3.0.eb
- SUCCESS pydantic-2.6.4-GCCcore-13.2.0.eb
- SUCCESS scikit-build-0.17.6-GCCcore-13.2.0.eb
- SUCCESS Z3-4.13.0-GCCcore-13.2.0.eb
- SUCCESS lxml-5.3.0-GCCcore-13.3.0.eb
- SUCCESS cryptography-41.0.5-GCCcore-13.2.0.eb
- SUCCESS Cython-3.0.10-GCCcore-13.2.0.eb
- SUCCESS Pillow-10.2.0-GCCcore-13.2.0.eb
- SUCCESS archspec-0.2.2-GCCcore-13.2.0.eb
Build succeeded for 13 out of 13 (13 easyconfigs in total) i7139 - Linux Rocky Linux 8.9 (Green Obsidian), x86_64, AMD EPYC 7702 64-Core Processor, Python 3.8.17 See https://gist.github.com/Flamefire/a4a6e8292ac9c977a15360f7828db932 for a full test report.
This needs rebasing on #3475 now
This needs rebasing on #3475 now
Done and split the pyver fix commit into #3478
@Flamefire I changed to target branch in this PR from 5.0.x to develop, you should synchronize your PR branch with current develop branch (which has received a massive update after the release of EasyBuild v5.0.0, see https://github.com/easybuilders/easybuild-easyblocks/pull/3670)
Should i look into getting this PR merged or #3428 Sorry I lost track of this before the 5.0 release.
Should i look into getting this PR merged or #3428 Sorry I lost track of this before the 5.0 release.
This used to be for 5.x while the other was for develop. I updated both and compared them so I could combine outstanding changes.
So this is the most current version now and I closed the other one.
Should i look into getting this PR merged or #3428 Sorry I lost track of this before the 5.0 release.
There's a lot of extra code in here, and since this doesn't require any backwards incompatible changes, we opted not to block the release of EasyBuild 5.0.0 over this.
We should get back to this indeed though, I would definitely like to see this fixed, but given the easyblocks this is touching, we'll need to tread carefully here...
Test report by @boegel
Overview of tested easyconfigs (in order)
- SUCCESS Python-3.10.8-GCCcore-12.2.0.eb
- SUCCESS Python-bundle-PyPI-2024.06-GCCcore-13.3.0.eb
- SUCCESS SciPy-bundle-2023.11-gfbf-2023b.eb
- SUCCESS numexpr-2.9.0-foss-2023a.eb
- SUCCESS cryptography-41.0.1-GCCcore-12.3.0.eb
- SUCCESS matplotlib-3.9.2-gfbf-2024a.eb
Build succeeded for 6 out of 6 (6 easyconfigs in total) node3625.doduo.os - Linux RHEL 9.4, x86_64, AMD EPYC 7552 48-Core Processor (zen2), Python 3.9.18 See https://gist.github.com/boegel/b8c8ecfd5f3b7be340379bef46fa1619 for a full test report.