easybuilders / easybuild-easyblocks

Collection of easyblocks that implement support for building and installing software with EasyBuild.
https://easybuild.io
GNU General Public License v2.0
106 stars 285 forks source link

[5.0x] run pip check only once for PythonBundle #3432

Open Flamefire opened 2 months ago

Flamefire commented 2 months ago

(created using eb --new-pr)

We have 2 checks in PythonPackage:

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

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

boegel commented 1 month ago

@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...

Flamefire commented 1 month ago

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:

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)

Flamefire commented 1 month ago

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

Flamefire commented 1 month ago

Test report by @Flamefire

Overview of tested easyconfigs (in order)

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.

Micket commented 1 month ago

This needs rebasing on #3475 now

Flamefire commented 1 month ago

This needs rebasing on #3475 now

Done and split the pyver fix commit into #3478