-
Notifications
You must be signed in to change notification settings - Fork 283
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
enhance Python easyblock: Add option to install pip with core Python, tweak defaults, create unversioned pip symlink #2388
Conversation
e01821a
to
e7b6ddd
Compare
5b102a7
to
125dcb5
Compare
@Flamefire These changes seem to at least break I also have a test report coming up from |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 0 out of 1 (1 easyconfigs in total) |
I may have an explanation here: That EasyConfig installs setuptools without pip. This may lead to the same issue as with pip itself: It will install the new setuptools without removing the old one and then the newer pip might use the older setuptools which leads to that issue. Reading pypa/pip#6264 and your comments there suggests that indeed the older setuptools (installed by the bootstrap/ensurepip) are used Not sure how this can be solved if the EC tries to install pip/setuptools without pip while Python has already a pip and setuptools. I'm running out of ideas how to fix the issues AND have it backward compat. E.g. current ECs install pip without pip but use pip for setuptools: https://github.com/easybuilders/easybuild-easyconfigs/blob/91e0e3809cc56ae6e95db8cef902093d2f806be0/easybuild/easyconfigs/p/Python/Python-3.8.2-GCCcore-9.3.0.eb#L53 So either we use ensurepip in Python base and use pip to install/upgrade BOTH setuptools and pip, or don't use ensurepip and install setuptools/pip via setup.py. Everything else is trouble... And we can't have that without breaking something Idea: Force using pip for the pip/setuptools extensions. In a quick test the only thing that might be required is installing |
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 11 out of 13 (13 easyconfigs in total) |
@Flamefire We're fixing bugs here, which means things don't have to be fully backwards-compatible. If some of the existing easyconfigs we have centrally need changes in combination with the changes being made here to ensure they still work, then so be it. |
Ok then my suggestion would be:
This means for older pythons (e.g. Python 2) setuptools&pip will still be installed as eggs, which has worked so far. Real issues occur only when we want to update those 2 which was not (AFAIK) the case, until we used ensurepip. We can update recentish Python 2 ECs to set this option Installing/upgrading setuptools via pip was likely never a problem (checked some versions, and if we used ensurepip of course), but installing pip via pip requires wheel for newer versions of pip (I think 20+ according to some quick experiments) The error option would mean we need to fix a lot of ECs. So maybe add that option but make it off by default. Then we only need to fix ECs which we have changed to rely on pip being there, which are far less, and maybe backport that for a few older ones (2019a+ I'd say) --> Add option I'll be back for further discussion via slack on Monday if required. |
This allows easier installing of packages and avoids the fallback to using setup.py and egg files which can't be easily upgraded
Enabling the default sanity_pip_check can only be done if we also install a recent pip (i.e. >= 9). I added 6c7a190 for that. |
Enabling the download_dep_fail option by default does make sense to me but it shows "bugs" in older ECs, e.g. Python-2.7.13-foss-2017a.eb does not include ipaddress but that is a dependency for cryptography. Due to enabling the "disconnected" option of pip if "download_dep_fail" is set, with this change it will NOT download ipaddress from PyPi anymore and fail to install the EC. @boegel I can PR the fix to the EC and keep the |
Looks like fixing the EC(s) is more work and likely some more are also affected. I choose to not set download_dep_fail for "old" Python versions. The remaining ones should already set that in the EC so it's safe there, I guess. Could still remove this and wait for EB 5.x with that change |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 15 out of 17 (9 easyconfigs in total) |
Ok, it seems the old ECs can no longer be installed. I tested the failing 2 Python ECs without this patch and e.g. Python/3.6.1-foss-2017a/ does fail too with:
So it seems we missed a required package and PyPi has changed something so old pips can't use it anymore. But I'd also be fine with leaving this as-is and get this in rather sooner than later |
@Flamefire That failure is not related to the changes made in #2390, right? |
@boegel Yes they are not related. The failing command:
I.e. it is using setup.py not pip, hence this is unaffected by the change in #2390 which only affects pip |
easybuild/easyblocks/p/python.py
Outdated
# Since Python 2.7.14 (and all Python 3 versions) we usually enable download_dep_fail in the ECs | ||
# already so this should not lead to failures | ||
if LooseVersion(self.version) >= LooseVersion('2.7.14'): | ||
ext_defaults['download_dep_fail'] = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Flamefire I don't think we should automatically enable this at this point, that's a rather intrusive change in the default behavior, which we should only make in EasyBuild v5.0 (cfr. #2127).
I fully agree with you that we should do this, which is why the easyconfig test suite has been requiring this for a while now, and I'm looking forward to not having to worry about this anymore and enabling it by default, but we shouldn't for the time being.
We're not enabling download_dep_fail
in the oldest Python 3 easyconfigs (and it's not worth spending time in trying to enable that there). Those easyconfigs are old enough that they'll be archived in EasyBuild v5.0, so they won't block enabling download_dep_fail
by default.
If I understand correctly, just removing this part won't affect the rest of this PR, correct me if I'm wrong...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Postponed to EB 5.x where this will be the default in the PythonPackage EasyBlock
easybuild/easyblocks/p/python.py
Outdated
# Python installations must be clean. Requires pip >= 9 | ||
'sanity_pip_check': LooseVersion(self._get_pip_ext_version() or '0') >= LooseVersion('9'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be clarified a bit I think, and I would use 9.0
rather than 9
(just for clarity sake):
# Python installations must be clean. Requires pip >= 9 | |
'sanity_pip_check': LooseVersion(self._get_pip_ext_version() or '0') >= LooseVersion('9'), | |
# enable "pip check" in sanity check by default, to ensure that Python installations are clean; | |
# requires pip v9.0 or newer | |
'sanity_pip_check': LooseVersion(self._get_pip_ext_version() or '0') >= LooseVersion('9.0'), |
That said, I don't think we should be enabling sanity_pip_check
by default just yet. It'll result in broken Python
easyconfigs that worked just fine up until now, and changes that break backwards compability should only be made in the next major EasyBuild release (v5.0), cfr. #2127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Removed and will open a follow-up PR for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up: #2423
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 31 out of 36 (7 easyconfigs in total) |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 44 out of 46 (46 easyconfigs in total) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python
easyconfigs that didn't work are due to problems not related to the changes in this PR (they're broken in the same way with the current Python
easyblock), so I'll go ahead and merge this and follow up on those specific easyconfigs in a separate (easyconfigs) PR (with low priority though, since they're ancient...).
Thanks for your efforts on this @Flamefire!
Installing pip from the core Python allows easier installing of packages and avoids the fallback to using setup.py and egg files which can't be easily upgraded
I added an option
install_pip
for installing an initial pip+setuptools which may become the default in EB 5.0. However when that is used, thenpip
MUST be used to upgrade those 2 later in the EC or otherwise the initial version is not properly removed/updated but merely half-way patched (or with pip even kept and silently used)Hence an additional check is added to catch this case.
This is a breaking change, however I see no viable alternative to fix the current bug. See #2388 (comment) for more details.
Current ECs are fixed in easybuilders/easybuild-easyconfigs#12650
The remaining changes are closely related:
pip
topip2/3
spares the ECs to do that ininstallopts
pip check
doesn't succeed for the core Python package something is wrong...The last one is critical as
--ignore-installed
does not properly update but only add new files, not changing existing files leads to a mix of versions and issues such as easybuilders/easybuild-easyconfigs#12757