-
Notifications
You must be signed in to change notification settings - Fork 285
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
run pip check
only once for PythonBundle
#3428
base: develop
Are you sure you want to change the base?
run pip check
only once for PythonBundle
#3428
Conversation
02fa4ce
to
f9fb262
Compare
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 14 out of 14 (12 easyconfigs in total)
--> Works |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 12 out of 12 (12 easyconfigs in total)
--> Works |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 0 out of 1 (1 easyconfigs in total) |
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: I overwrite |
It is easier in the top-level and might cause confusion with easybuilders/easybuild-easyblocks#3428
It is easier in the top-level and might cause confusion with easybuilders/easybuild-easyblocks#3428
@Flamefire Although this is only a bug fix, please re-target this to the 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. |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 19 out of 19 (13 easyconfigs in total) |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 13 out of 13 (13 easyconfigs in total) |
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. |
It is easier in the top-level and might cause confusion with easybuilders/easybuild-easyblocks#3428
2b1b21e
to
0463256
Compare
Opened a PR against 5.0x: #3432 |
(created using
eb --new-pr
)We have 2 checks in PythonPackage:
pip check
pip list
-> Check for "0.0.0" versionsIn
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 ofPythonBundle
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 forsanity_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 ofsanity_check_step
ofPythonPackage
such that it can be used byPythonBundle
det_installed_python_packages
out of the class too, the originalPythonPackage.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 useget_installed_python_packages
for the global method?det_
-prefix is chosen similar todet_py_libdirs
PythonBundle.sanity_check_step
now requirespython_cmd
to be available which was only set in theprepare_step
that is skipped in--sanity-check-only
--> Factor outprepare_python
fromprepare_step
similar toPythonPackage
python
command to use although I see no reason for that. I factored outfind_python_cmd
fromPythonPackage.prepare_python
and call it fromPythonBundle
. I left the check for a loadedPython
module inPythonBundle
as I don't know the reason for that check. IMO it should either be in both or neitherFixes #3418