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

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

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

Conversation

Flamefire
Copy link
Contributor

(created using eb --new-pr)

We have 2 checks in PythonPackage:

  • pip check
  • pip 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_check is moved out of sanity_check_step of PythonPackage such that it can be used by PythonBundle
  • This required moving the dependent method det_installed_python_packages out of the class too, the original PythonPackage.get_installed_python_packages needs to stay for backwards compatibility which prevents giving the same name to the free function. Maybe in EB 5 we can remove it and use get_installed_python_packages for the global method? det_-prefix is chosen similar to det_py_libdirs
  • PythonBundle.sanity_check_step now requires python_cmd to be available which was only set in the prepare_step that is skipped in --sanity-check-only --> Factor out prepare_python from prepare_step similar to PythonPackage
  • There was a mismatch in the code to detect the python command to use although I see no reason for that. I factored out find_python_cmd from PythonPackage.prepare_python and call it from PythonBundle. I left the check for a loaded Python module in PythonBundle as 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: easybuilders/easybuild-framework#4620

@Flamefire Flamefire changed the title Single pip check 5.x [5.0x] run pip check only once for PythonBundle Sep 4, 2024
@boegel boegel added this to the 5.0 milestone Sep 7, 2024
@boegel boegel added the bug fix label Sep 25, 2024
@boegel
Copy link
Member

boegel commented Oct 8, 2024

@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 Flamefire force-pushed the single-pip-check-5.x branch 2 times, most recently from 19789d6 to f78c6f6 Compare October 9, 2024 14:23
@Flamefire
Copy link
Contributor Author

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_majver is 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)

@Flamefire Flamefire force-pushed the single-pip-check-5.x branch 3 times, most recently from cde58b6 to 286d365 Compare October 10, 2024 08:32
@Flamefire
Copy link
Contributor Author

Flamefire commented Oct 10, 2024

I copied the refactoring to #3475 for easier review

Both tested with a random selection of recent-ish PythonBundle and PythonPackage ECs

@Flamefire
Copy link
Contributor Author

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.

@Micket
Copy link
Contributor

Micket commented Oct 10, 2024

This needs rebasing on #3475 now

@Flamefire Flamefire force-pushed the single-pip-check-5.x branch 3 times, most recently from e3f4293 to 06ab742 Compare October 11, 2024 09:19
@Flamefire
Copy link
Contributor Author

This needs rebasing on #3475 now

Done and split the pyver fix commit into #3478

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Nice-to-have
Development

Successfully merging this pull request may close these issues.

3 participants