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.

Sign up for GitHub

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

Jump to bottom

run pip check only once for PythonBundle #3428

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

run pip check only once for PythonBundle #3428

wants to merge 3 commits into from

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Aug 29, 2024 •
edited
Loading

(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

@Flamefire Flamefire force-pushed the 20240829134410_new_pr_pythonpackage branch from 02fa4ce to f9fb262 Compare August 29, 2024 12:00
@Flamefire
Copy link
Contributor Author

Flamefire commented Aug 29, 2024 •
edited
Loading

Test report by @Flamefire

Overview of tested easyconfigs (in order)

  • SUCCESS SciPy-bundle-2022.05-foss-2022a.eb
  • SUCCESS SciPy-bundle-2023.02-gfbf-2022b.eb
  • SUCCESS Python-bundle-PyPI-2023.06-GCCcore-12.3.0.eb
  • SUCCESS Python-bundle-PyPI-2023.10-GCCcore-13.2.0.eb
  • SUCCESS PyYAML-6.0.1-GCCcore-13.2.0.eb
  • SUCCESS bcrypt-4.1.3-GCCcore-13.2.0.eb
  • SUCCESS hypothesis-6.90.0-GCCcore-13.2.0.eb
  • SUCCESS expecttest-0.2.1-GCCcore-13.2.0.eb
  • SUCCESS Cython-3.0.10-GCCcore-13.2.0.eb
  • SUCCESS SciPy-bundle-2023.07-gfbf-2023a.eb
  • SUCCESS SciPy-bundle-2023.11-gfbf-2023b.eb
  • SUCCESS MPFR-4.2.1-GCCcore-13.2.0.eb (non-Python)
  • SUCCESS MPC-1.3.1-GCCcore-13.2.0.eb (non-Python)
  • SUCCESS gmpy2-2.1.5-GCC-13.2.0.eb

Build succeeded for 14 out of 14 (12 easyconfigs in total)
i7150 - Linux Rocky Linux 8.9 (Green Obsidian), x86_64, AMD EPYC 7702 64-Core Processor (zen2), Python 3.8.17
See https://gist.github.com/Flamefire/9b46c20a82f46a2b0ac8ac796c797ba0 for a full test report.

$ grep -F 'pip check` completed' /tmp/easybuild-tmplog/easybuild-9v3ppgb0.log | wc -l
12

--> Works

@Flamefire
Copy link
Contributor Author

Flamefire commented Aug 30, 2024 •
edited
Loading

--module-only build

Test report by @Flamefire

Overview of tested easyconfigs (in order)

  • SUCCESS SciPy-bundle-2022.05-foss-2022a.eb
  • SUCCESS SciPy-bundle-2023.02-gfbf-2022b.eb
  • SUCCESS Python-bundle-PyPI-2023.06-GCCcore-12.3.0.eb
  • SUCCESS Python-bundle-PyPI-2023.10-GCCcore-13.2.0.eb
  • SUCCESS PyYAML-6.0.1-GCCcore-13.2.0.eb
  • SUCCESS bcrypt-4.1.3-GCCcore-13.2.0.eb
  • SUCCESS gmpy2-2.1.5-GCC-13.2.0.eb
  • SUCCESS hypothesis-6.90.0-GCCcore-13.2.0.eb
  • SUCCESS expecttest-0.2.1-GCCcore-13.2.0.eb
  • SUCCESS Cython-3.0.10-GCCcore-13.2.0.eb
  • SUCCESS SciPy-bundle-2023.07-gfbf-2023a.eb
  • SUCCESS SciPy-bundle-2023.11-gfbf-2023b.eb

Build succeeded for 12 out of 12 (12 easyconfigs in total)
i7150 - Linux Rocky Linux 8.9 (Green Obsidian), x86_64, AMD EPYC 7702 64-Core Processor (zen2), Python 3.8.17
See https://gist.github.com/Flamefire/9f33337e3272a6195a6409513152ea14 for a full test report.

$ grep -F 'pip check` completed' /tmp/easybuild-tmplog/easybuild-ibyka26o.log | wc -l
12

--> Works

@Flamefire Flamefire mentioned this pull request Aug 30, 2024
Open
@Flamefire
Copy link
Contributor Author

Test report by @Flamefire

Overview of tested easyconfigs (in order)

Build succeeded for 0 out of 1 (1 easyconfigs in total)
n1604 - Linux RHEL 8.9 (Ootpa), x86_64, Intel(R) Xeon(R) Platinum 8470 (icelake), Python 3.8.17
See https://gist.github.com/Flamefire/eabd7daf1272cc47e34a7d2f03d623b2 for a full test report.

@Flamefire
Copy link
Contributor Author

Flamefire commented Aug 30, 2024 •
edited
Loading

For some reason the build dependencies are still loaded or get loaded in the sanity check after this change. Poetry depends on hatchling and has a builddep to scicit-build and that installs a packaging version (23.1) which is incompatible with hatchling 1.24.2 (requires packaging>=23.2)

Ok I found the issue: sanity_check_load_module must be called to also clean up the builddeps.

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 mentioned this pull request Aug 30, 2024
Merged
@boegel boegel added the bug fix label Aug 30, 2024
@boegel boegel added this to the 5.0 milestone Aug 30, 2024
@boegel boegel added the EasyBuild-5.0 EasyBuild 5.0 label Aug 30, 2024
@boegel
Copy link
Member

boegel commented Aug 30, 2024

@Flamefire Although this is only a bug fix, please re-target this to the 5.0.x to only include it with EasyBuild 5.0.

I think you're reasoning makes sense (haven't checked the diff in detail yet), but I want to err on the side of caution with this one.

That maybe also opens the door to not bother deprecating things, not sure at this point.

@Flamefire
Copy link
Contributor Author

Test report by @Flamefire

Overview of tested easyconfigs (in order)

  • SUCCESS SciPy-bundle-2022.05-foss-2022a.eb
  • SUCCESS SciPy-bundle-2023.02-gfbf-2022b.eb
  • SUCCESS Python-bundle-PyPI-2023.06-GCCcore-12.3.0.eb
  • SUCCESS Python-bundle-PyPI-2023.10-GCCcore-13.2.0.eb
  • SUCCESS PyYAML-6.0.1-GCCcore-13.2.0.eb
  • SUCCESS bcrypt-4.1.3-GCCcore-13.2.0.eb
  • SUCCESS gmpy2-2.1.5-GCC-13.2.0.eb
  • SUCCESS hypothesis-6.90.0-GCCcore-13.2.0.eb
  • SUCCESS expecttest-0.2.1-GCCcore-13.2.0.eb
  • SUCCESS Cython-3.0.10-GCCcore-13.2.0.eb
  • SUCCESS SciPy-bundle-2023.07-gfbf-2023a.eb
  • SUCCESS SciPy-bundle-2023.11-gfbf-2023b.eb
  • SUCCESS libarchive-3.7.4-GCCcore-13.3.0.eb
  • SUCCESS CMake-3.29.3-GCCcore-13.3.0.eb
  • SUCCESS Rust-1.78.0-GCCcore-13.3.0.eb
  • SUCCESS maturin-1.6.0-GCCcore-13.3.0.eb
  • SUCCESS cffi-1.16.0-GCCcore-13.3.0.eb
  • SUCCESS cryptography-42.0.8-GCCcore-13.3.0.eb
  • SUCCESS poetry-1.8.3-GCCcore-13.3.0.eb

Build succeeded for 19 out of 19 (13 easyconfigs in total)
i7078 - Linux Rocky Linux 8.9 (Green Obsidian), x86_64, AMD EPYC 7702 64-Core Processor (zen2), Python 3.8.17
See https://gist.github.com/Flamefire/b950ce066f48dfe5ff70e6286ef5604d for a full test report.

@Flamefire
Copy link
Contributor Author

Test report by @Flamefire

Overview of tested easyconfigs (in order)

  • SUCCESS SciPy-bundle-2022.05-foss-2022a.eb
  • SUCCESS SciPy-bundle-2023.02-gfbf-2022b.eb
  • SUCCESS Python-bundle-PyPI-2023.06-GCCcore-12.3.0.eb
  • SUCCESS Python-bundle-PyPI-2023.10-GCCcore-13.2.0.eb
  • SUCCESS PyYAML-6.0.1-GCCcore-13.2.0.eb
  • SUCCESS bcrypt-4.1.3-GCCcore-13.2.0.eb
  • SUCCESS gmpy2-2.1.5-GCC-13.2.0.eb
  • SUCCESS hypothesis-6.90.0-GCCcore-13.2.0.eb
  • SUCCESS expecttest-0.2.1-GCCcore-13.2.0.eb
  • SUCCESS Cython-3.0.10-GCCcore-13.2.0.eb
  • SUCCESS SciPy-bundle-2023.07-gfbf-2023a.eb
  • SUCCESS SciPy-bundle-2023.11-gfbf-2023b.eb
  • SUCCESS poetry-1.8.3-GCCcore-13.3.0.eb

Build succeeded for 13 out of 13 (13 easyconfigs in total)
i7012 - Linux Rocky Linux 8.9 (Green Obsidian), x86_64, AMD EPYC 7702 64-Core Processor (zen2), Python 3.8.17
See https://gist.github.com/Flamefire/55d0fd72b37f71a1fac528cca182621a for a full test report.

@Flamefire
Copy link
Contributor Author

That maybe also opens the door to not bother deprecating things, not sure at this point.

So what shall I do? I'd go with keeping the deprecation calls such that it will be an error immediately in 5.0x

I have some EC PRs to go with this:

Because I noticed that at some places ext_default_options is used instead of the PythonBundle options which triggers this check and just did a search for such ECs. I'd merge those once the test reports are in and then check the 5.0x branch too. If we don't we'll likely get new ECs created from those reintroducing the "issue" and the change is pretty straight forward and easy to even manually verify.

Flamefire added a commit to Flamefire/easybuild-easyconfigs that referenced this pull request Sep 4, 2024
@Flamefire
Move exts_default_options to top-level parameters forPythonBundle ECs
423f374 
It is easier in the top-level and might cause confusion with easybuilders/easybuild-easyblocks#3428
@Flamefire Flamefire force-pushed the 20240829134410_new_pr_pythonpackage branch from 2b1b21e to 0463256 Compare September 4, 2024 12:53
@Flamefire
Copy link
Contributor Author

Opened a PR against 5.0x: #3432
Or would you prefer to update this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug fix EasyBuild-5.0 EasyBuild 5.0 performance
Projects
EasyBuild v5.0
Status: Nice-to-have
Milestone
5.0
Development

Successfully merging this pull request may close these issues.

PythonBundle should perform 1 single pip check instead of each python package repeating it
2 participants
@Flamefire @boegel