Conversation
| set(WITH_PYTHON3 "3" CACHE STRING "build with specified python3 version") | ||
| if(NOT WITH_PYTHON3 STREQUAL "3") | ||
| # Please specify 3.[0-8] if you want to build with a certain version of python3. | ||
| set(WITH_PYTHON3 "3.8" CACHE STRING "build with specified python3 version") |
There was a problem hiding this comment.
I don't have 3.8 installed on my system. Is there a way to make exclude 3.5 without mandating versions that are not installed?
There was a problem hiding this comment.
i don't think wiring the default value to 3.8 helps unless we depends on 3.8 and up.
There was a problem hiding this comment.
I don't have 3.8 installed on my system. Is there a way to make exclude 3.5 without mandating versions that are not installed?
@sebastian-philipp but if we declare the 3.8 dep it'll become installed. Is there any specific reason not to want 3.8 installed?
There was a problem hiding this comment.
i don't think wiring the default value to 3.8 helps unless we depends on 3.8 and up.
@tchaikov just to mention a single change: f-strings are not supported in 3.5, so we should at least indicate that we require 3.6 or above (which, on the other hand, is nothing really adventurous given that 3.5 end of life finished almost a year ago and 3.6 EOL is Dec this year).
The reason to go with 3.8 is using improved typing constructs and other basic helpers as dataclasses.
There was a problem hiding this comment.
i don't think wiring the default value to 3.8 helps unless we depends on 3.8 and up.
@tchaikov just to mention a single change: f-strings are not supported in 3.5, so we should at least indicate that we require 3.6 or above (which, on the other hand, is nothing really adventurous given that 3.5 end of life finished almost a year ago and 3.6 EOL is Dec this year).
i agreed that depending on 3.6 is the way to go. that's actually one of the reasons why i pushed the xenial to bionic change.
please note, my concern is not why 3.6 but why 3.8.
There was a problem hiding this comment.
Personally I've found a few things that are of interest in python 3.8, but they mainly revolve around asyncio improvements. On top of that, I find having type annotation forward references is incredibly useful. And, as @epuertat mentioned, 3.6 is EOL at end of this year, and moving on to python 3.8 makes sense just because of that, should there be no particular constraint.
| set(WITH_PYTHON3 "3" CACHE STRING "build with specified python3 version") | ||
| if(NOT WITH_PYTHON3 STREQUAL "3") | ||
| # Please specify 3.[0-8] if you want to build with a certain version of python3. | ||
| set(WITH_PYTHON3 "3.8" CACHE STRING "build with specified python3 version") |
There was a problem hiding this comment.
i don't think wiring the default value to 3.8 helps unless we depends on 3.8 and up.
| BuildRequires: python%{python3_pkgversion}-devel | ||
| BuildRequires: python%{python3_pkgversion}-setuptools | ||
| BuildRequires: python%{python3_pkgversion}-Cython | ||
| BuildRequires: python%{python3_version_nodots} |
There was a problem hiding this comment.
i don't understand this change. and i cannot find the explanation in the commit message or in tracker ticket.
There was a problem hiding this comment.
@tchaikov sure, I'll update the PR to comment my changes. In this very case, the python3_pkgversion macro is not used in the RPM ecosystem, while the python3_version_nodots is a very well established macro.
There was a problem hiding this comment.
i was looking at https://fedoraproject.org/wiki/PackagingDrafts:Python3EPEL and was referencing lots of actively maintained python packages when working on the rpm recipe, for instance
- https://src.fedoraproject.org/rpms/python-requests/blob/rawhide/f/python-requests.spec
- https://src.fedoraproject.org/rpms/python-setuptools/blob/rawhide/f/python-setuptools.spec
so i am not convinced that python3_pkgversion is not used in the RPM ecosystem.
There was a problem hiding this comment.
and this change should be put into a separate commit.
There was a problem hiding this comment.
Thanks for the reference @tchaikov . My main concern with the python3_pkgversion is that the name is not very self-describing, ulinke python3_version, python3_version_nodots or python3_version_major (which follows the CMake convention).
Why should this be in a separate commit?
There was a problem hiding this comment.
Thanks for the reference @tchaikov . My main concern with the
python3_pkgversionis that the name is not very self-describing, ulinkepython3_version,python3_version_nodotsorpython3_version_major(which follows the CMake convention).
IMHO, pkgversion is a good name in this context, in which it specifies the python version of a python package.
i don't understand why we need to follow the CMake convention here. and strictly speaking, i don't think python3_version, python3_version_nodots and python3_version_major are following the find_package() conventions.
FindPython3 module follows the convention of find_package(), so find_package(Python3 ...) can work as expected.
Why should this be in a separate commit?
because i think it is orthogonal to "use Python 3.8".
There was a problem hiding this comment.
IMHO, pkgversion is a good name in this context, in which it specifies the python version of a python package.
@tchaikov my main concern with that name is that it doesn't specify whether that's the major number only (as it was so far), the complete version with dots or without (as it should be, at least for CentOS and OpenSUSE python38 and other ABI-bound packages).
because i think it is orthogonal to "use Python 3.8".
Do you mean replacing the python3_pkgversion with python3_version_nodots?
I don't think so: maybe it's not compulsory, but you still need 2 macros to cope with packages bound to the 3.8 ABI (e.g.: python38-Cython) and pure source code packages (e.g.: python3-cherrypy). If you prefer to keep python3_pkgversion, which one should be it? The major-version one or the full nodots? That's why I preferred having a self-descriptive macro. What's your preference?
There was a problem hiding this comment.
i don't think we should not differentiate the python packages which contains binaries from the ones which are implemtented in "pure python". as both of them are packaged and installed for a certain python x.y version, instead of a certain major version. take python3-cherrypy as an example
$ rpm -q --provides python3-cherrypy
python3-cherrypy = 18.4.0-1.el8
python3.6dist(cherrypy) = 18.4.0
python3dist(cherrypy) = 18.4.0the reasons why i think python3_pkgversion is the right choice are not completely based on by my personal preferences or taste:
-
to follow related package guide line: https://fedoraproject.org/wiki/PackagingDrafts:Python3EPEL
All references to python3 (except in macro names) need to be replaced with python%{python3_pkgversion}.
-
to follow examples. as it is used by lots of actively maintained python packages. see
in general, python package maintainers should not hardwire a python package to a certain python3 minor version. python3 means the python3.6 in EL8, and it is python3.9 in fedora33. python3_pkgversion is used to encapsulate this information, and i think it serves this purpose well. i am not an expert on the rpm packaging, and i don't feel comfortable commenting on if other macros are superior or better than python3_pkgversion . it is but my 2 cents.
Do you mean replacing the
python3_pkgversionwithpython3_version_nodots?
yes. and i don't recommend python3_version_nodots. but if you insist, i'd suggest do this in two steps:
- to replace the
python3_pkgversionwithpython3_version_nodots. and accompany it with explanations. so far the commit message still does not explain why we need this change. and your explanations in the comments have been changing. - to set the default values of the rpm macros from "3" to "3.8". please note, this does not change the dependencies from 3 to 3.8, as the behavior depends on the existence of
python-srpm-macros,python-rpm-macros,python36-rpm-macrosandpython38-rpm-macros.
instead i'd suggest:
- install
python38-rpm-macrosininstall-deps.sh. with which, we have:
$ rpm --eval '%{python3_pkgversion}'
38- replace
python3-develwithpython38-develininstall-deps.sh - update https://github.com/ceph/ceph-build/blob/master/scripts/build_utils.sh#L1396
- make sure readthedoc build does not break. see https://readthedocs.org/projects/ceph/builds/14110525/ for an example, where python3.7 instead of python3.8 is used, but
typing.Literalwas not available in python3.7.
| BuildRequires: python%{python3_pkgversion}-devel | ||
| BuildRequires: python%{python3_pkgversion}-setuptools | ||
| BuildRequires: python%{python3_pkgversion}-Cython | ||
| BuildRequires: python%{python3_version_nodots} |
There was a problem hiding this comment.
i was looking at https://fedoraproject.org/wiki/PackagingDrafts:Python3EPEL and was referencing lots of actively maintained python packages when working on the rpm recipe, for instance
- https://src.fedoraproject.org/rpms/python-requests/blob/rawhide/f/python-requests.spec
- https://src.fedoraproject.org/rpms/python-setuptools/blob/rawhide/f/python-setuptools.spec
so i am not convinced that python3_pkgversion is not used in the RPM ecosystem.
ceph.spec.in
Outdated
| Requires: parted | ||
| Requires: psmisc | ||
| Requires: python%{python3_pkgversion}-setuptools | ||
| Requires: python%{python3_version_nodots} |
There was a problem hiding this comment.
@tchaikov what do you think about this one?
I wasn't sure when I added it: it affects the base package. On one hand I didn't want to impose a Ceph-wide dependency on Python 3.8, but on the other hand, it also relies on python3-setuptools which will in turn depend on python3x, so this makes the dependency more explicit. I personally feel that it's more dangerous to allow coexisting versions of Python on a mono-repo (and assume part of our code base is still 3.6 compliant, without making it clear which ones).
There was a problem hiding this comment.
yeah, the only python tool packaged by ceph-base is the ceph cli tool once #41999 gets merged. but ceph-base in turn depends on ceph-common which packages a bunch of ceph python modules, like
- ceph-argparse
- ceph-common
and python bindings for librados, libcephfs and librbd. all of them are build and installed for a certain python version. so yes, it should depend on a certain python runtime versioned with minor version number.
when it comes to python3-setuptool, i moved it to ceph-osd in #41999 as a cleanup. not sure what maintainers of ceph-volume think about the python3.8 dependencies. if we can go further by extracting ceph-volume into its own package, we don't need to worry about this problem anymore. but since ceph-osd depends on ceph-base, it's non-trivial to make the dependency from ceph-volume to "python3" but not "python3.8" explicit. and as you put, it might not be a best practice to use multiple python runtime in a deployment. i concur with you on this, as it'd be a pain for testability and maintainability, even it's completely technically viable and supported by python.
in short,
- i'd avoid having both
python3-setuptoolsandpython38as runtime dependencies of a single package. - if ceph-volume is extracted into its own package, and it does not depend on other ceph python modules. it can depend on
python3-setuptoolsorpython38-setuptools. but if we want to usepython3-setuptools, we have to test bothpython36-setuptoolsandpython38-setuptools.
There was a problem hiding this comment.
BTW, we will have to build python bindings for both python3.8 and python3.6, so we don't break the existing users of these python bindings, and package them individually. so each python binding will be built and packaged twice for python3.6 (or the default python3 on the distro) and python3.8.
epuertat
force-pushed
the
wip-51319-master
branch
3 times, most recently
from
9694598 to
a4a393d
Compare
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
Other changes: - Added explicit dependency on git in ceph.spec (debian/control already had it). - Tentatively removed hardcoded python3-devel install in install-deps.sh Pros: - Making dependencies on Python minor version numbers explicit (PEP-20: explicit is better than implicit): Ceph packages don't explicitly require a minor Python version, but they actually rely on 3.6 features (f-strings just to name one). - Python 3.6 reaches End of Life this Dec '21 (3.8 EoL is Oct '24). - Python 3.8 provides new features that might help write more efficient and solid code: - Improved typing support - dataclasses - async - An statistics package - And other features [1] [2] Performance-wise: Python 3.7/3.8 brought dramatic performance improvements. Fixes: https://tracker.ceph.com/issues/51319 Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat
force-pushed
the
wip-51319-master
branch
from
a4a393d to
72d3445
Compare
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
|
@epuertat is this change still relevant ? i will help migrating to python 3.8 if you are fine with this. |
|
@epuertat any idea what's needed to move this along? |
@jecluis apart from rebasing... Based on this comment from @tchaikov, we'd need to modify our build pipeline to create 2 different set of packages for 3.8 and 3.6.... But to make things even funnier, I just tested with our 4 major target distros, and found that:
This is the detail on the testing with the following with docker images (
Summarizing, if we want to support the above distro matrix, we should better go for 3.9 :/ Good thing is that the 3.9 will give us an extra year (EoL by 2025) and adds some features, like |
|
Harsher than initially expected. I barely started with CentOS and the main issue is that Python packages are installed in a specific It can be easily hacked with a Tempted of doing this. |
|
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
Red Hat's Python team will continue to maintain Python 3.6 in RHEL 8 (main EOL is May 31, 2029) and CentOS Stream 8 (EOL is May 31 2024). We should consider dropping CentOS 8 support for Ceph in |
|
@epuertat we were also looking for python-3.8(.13 as latest) upgrade, for reasons you had listed in PR description, plus in #56945 issue. Additionally, python-3.6 also has some security (CVEs) issues which asks for this upgrade strongly, however not all would apply here as python mainly used in build infra.
After looking at this thread, saw much of the work and discussion has had happened already and was hopeful. Though after reading rest of the comments (below), seems additional packaging challenges would be added, which will be worth too i think, though would be more bulky, right?
Wouldn't this default version on distro can be bound too, to avoid breaking existing users python binding? @epuertat @tchaikov |
|
@cdeshmukh yes, there are plans, but also multiple interlocking issues which complicate this:
Are you targetting a specific distro? |
|
The openSUSE recent version is Leap 15.4, which has python3.6 base version, however in stock distro there is python3.9 and even python3.10, but there is no python3.8 at all. |
Thanks for chiming in, @kshtsk ! There's no strict requirement on the minor of the Python version except that it be >=8 (based on a recent analysis on the base Python versions for newer distro releases, there's no agreement on it as there was with Python 3.6: some distros bring 3.8, others 3.9 and Ubuntu is directly going all-out for 3.10). So could you please try to build Ceph on OpenSUSE with PYTHON RPM macros set to 3.9, install it and check if all build or install deps are met? Thanks! |
|
thanks @epuertat
We (cortx-rgw) are targeting RockyLinux 8.4 (CentOS 8 equivalent), which doesn't have python installed by default, though on explicit install of python3, 3.6.8 python version is installed from RockyLinux 8.4 repositories. So what are the plans for upgrade then for CentOS 8? And this upgrade would be targeted independently for all distros, in ceph build scripts, or need to be together, i believe its the latter case. |
The plan is that |
thanks for the insightful link for upcoming Reef release, very useful details in there. However, i should have been more specific. I meant to ask the plan for 'upgrade to python 3.8/3.9' on CentOS8 itself(until Reef is released & we can adopt), for which this PR was intended originally, along with to support other Distro versions. |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution! |
|
@epuertat @tchaikov it's been a while since this PR was closed. Checking the current supported versions I can see that both
WDYT? |
I see, thanks @tchaikov. @ktdreyer do you know how long should we support RHEL8? |
We are very close to moving over to centos 9 based containers (which have python 3.9 iirc) and that move is considered a blocker for the first squid RC build. On the other hand, we will still be doing quincy and reef releases on centos 8 containers. |
Hopefully those will move to use an el9 base image also. Like Kefu said, CentOS 8 Stream will be EOL May 31. |
|
@epuertat do we need to update default value for python3_pkgversion in ceph.spec.in file, since now the minimal supported version of python is 3.9? |
In some RPM docs they suggest that That might work for installing deps, as most Python packages (that excludes OpenSUSE ecosystem) also define |
10 participants
Reasons:
typingsupportdataclassesasyncstatisticspackageOther changes:
python3_versionandpython3_version_nodotsFedora/EPEL RPM macros. Added custompython3_version_major.Check distro support:
Fixes: https://tracker.ceph.com/issues/51319
Fixes: https://tracker.ceph.com/issues/56945
Signed-off-by: Ernesto Puerta epuertat@redhat.com
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox