feature: use ABI3 for cp36+ #2102
Conversation
mayeut
force-pushed
the
abi3-4
branch
2 times, most recently
from
e136aa3
to
8077259
Compare
mayeut
force-pushed
the
abi3-4
branch
2 times, most recently
from
ea146ae
to
e109f6b
Compare
mayeut
force-pushed
the
abi3-4
branch
2 times, most recently
from
41a91aa
to
ba617a4
Compare
mayeut
force-pushed
the
abi3-4
branch
5 times, most recently
from
097e654
to
64e67dc
Compare
mayeut
force-pushed
the
abi3-4
branch
4 times, most recently
from
1013132
to
b9b223e
Compare
Signed-off-by: mayeut <mayeut@users.noreply.github.com>
There was a problem hiding this comment.
This truly is an amazing work. Yet another thing I vaguely read about but never had the chance to delve into. Thanks.
I'm reading https://docs.python.org/3/c-api/stable.html. It poses some important considerations and suggests to test all minor Python versions. According to psutil's setup.py the most minor version we support is Python 3.4:
if setuptools is not None:
kwargs.update(
python_requires=">=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*",
extras_require=extras_require,
zip_safe=False,
)As such I wonder:
- would it make sense for
build.ymlto test 3.4? - what python versions are we testing right now? I see that windows has
["3.7", "3.8", "3.9", "3.10", "~3.11.0-0"]but I'm unclear about the other platforms.
Again, thanks for this great work!
setup.py
Outdated
| @@ -100,6 +104,14 @@ def get_version(): | |||
| VERSION = get_version() | |||
| macros.append(('PSUTIL_VERSION', int(VERSION.replace('.', '')))) | |||
|
|
|||
| PY36_PLUS = sys.version_info[:2] >= (3, 6) | |||
| CP36_PLUS = PY36_PLUS and sys.implementation.name == "cpython" | |||
There was a problem hiding this comment.
Please let's define this platform stuff at the top of the module, I would say right after PYPY = ....
Also I suggest to define a separate CPYTHON = ... constant so that later on it's clear that this stuff is executed only on macos|linux|windows + py>=36 + cpython.
There was a problem hiding this comment.
We can't use CPYTHON = sys.implementation.name == "cpython" because sys.implementation was introduced in Python 3.3 with PEP 421.
I'll move the definitions below PYPY = ....
| try: | ||
| from wheel.bdist_wheel import bdist_wheel | ||
| except ImportError: | ||
| bdist_wheel = None |
There was a problem hiding this comment.
I wonder: is it possible (or worth it) to raise in case we're explicitly running cbuildwheels tool? Something like (not tested, just to illustrate what I mean):
try:
from wheel.bdist_wheel import bdist_wheel
except ImportError:
if "RUNNING_CIBUILDWHEEL_CLI" in os.environ:
raise....or perhaps cbuildwheel command already fails if wheel package is not installed?
setup.py
Outdated
| class bdist_wheel_abi3(bdist_wheel): | ||
| def finalize_options(self): | ||
| bdist_wheel.finalize_options(self) | ||
| self.root_is_pure = False | ||
|
|
||
| def get_tag(self): | ||
| python, abi, plat = bdist_wheel.get_tag(self) | ||
| return python, "abi3", plat | ||
|
|
||
| cmdclass["bdist_wheel"] = bdist_wheel_abi3 |
There was a problem hiding this comment.
This looks like an hack. I guess this means bdist_wheel does not support Py_LIMITED_API natively? Should we keep an eye on this in the future? (e.g. the original class changes and this no longer works)
There was a problem hiding this comment.
Here's the example linked from the cibuildwheel docs:
https://github.com/joerick/python-abi3-package-sample/blob/main/setup.py
It also overrides get_tag, though it's limited to CPython, which you probably want to do too.
Is manually setting root_is_pure necessary? I would imagine it's automatically set according to ext_modules.
There was a problem hiding this comment.
I guess this means bdist_wheel does not support Py_LIMITED_API natively? Should we keep an eye on this in the future? (e.g. the original class changes and this no longer works)
wheel only has static support for Py_LIMITED_API through config file as far as I know.
It's probably worth keeping an eye on this in the future.
which you probably want to do too
Well, I can't remember where I saw a recommendation against building cp3x abi3 with a more recent CPython than 3.x but I'm pretty sure I saw it and there was a rationale behind it. It might not be important in this case but there's probably no harm letting this as implemented now.
Is manually setting root_is_pure necessary?
Indeed it's not.
There was a problem hiding this comment.
Well, I can't remember where I saw a recommendation against building cp3x abi3 with a more recent CPython than 3.x but I'm pretty sure I saw it and there was a rationale behind it. It might not be important in this case but there's probably no harm letting this as implemented now.
I meant checking the implementation before setting the tag as abi3 (i.e. if python.startswith("cp"):).
Now I see it's all wrapped with if py_limited_api: which is CPython-only, so this part of my comment is irrelevant. Whoops 😛
Indeed I think the official docs advise building cp3x ABI3 wheels using cp3x :)
Not sure what's the correct thing to do with the bdist_wheel class given the explicit setting of Py_LIMITED_API = 0x03060000, but I think either way should be fine here haha
| InstallPackage $env:PYTHON wheel | ||
| } | ||
|
|
||
| main |
There was a problem hiding this comment.
I added this file years ago so I don't even remember what it's for. Was this necessary for Python 3 only?
There was a problem hiding this comment.
It might have been necessary to install some missing python versions on AppVeyor at some point.
It's definitely not needed for Python 2.7
| export PSUTIL_DEBUG=1 | ||
| python ../psutil/tests/runner.py | ||
| python ../psutil/tests/test_memleaks.py | ||
| shell: bash |
There was a problem hiding this comment.
Before I was against removing appveyor, but with pyproject.toml, wheels and Py_LIMITED_API I now see why this is necessary. One question though: with this new addition GitHub will test setup.py using Py_LIMITED_API, while appveyor CI will test python 2.7 + setup.py without Py_LIMITED_API. Am I correct? Do you still see value in keep using appveyor or do you think we should abandon it?
Some background on why we're using appveyor:
- historical reason: initially (years ago) GitHub did not support windows
- to split work on 2 CI services and therefore complete the whole CI test/build sooner (but perhaps I'm wrong on this)
There was a problem hiding this comment.
I personally find using 2 CI services a bit confusing and harder to maintain, and GHA's better integration with the repo/PRs is a nice advantage.
I believe there's no speed benefit to this split. GHA's concurrency limits are (much?) higher than the current jobs amount.
Edit: Ugh, I forgot building Windows 2.7 wheels (on GHA, didn't know AppVeyor has the tools) requires adding a Visual C++ dependency...
There was a problem hiding this comment.
Am I correct?
Yes you are.
Do you still see value in keep using appveyor or do you think we should abandon it?
If you want to keep Python 2.7 wheels on Windows, then I'd say it shall remain. AppVeyor still keeps old images around and that includes some build tools "required" for Python 2.7 that are not available for download anymore and not available on GHA.
Given AppVeyor has 1 concurrent job and GHA has higher concurrency limits than the current jobs amount, it will be faster with GHA. The current limiter on master to get a green CI is AppVeyor with an average of 30 minutes. This PR cuts that at least in half.
|
Another consideration. https://docs.python.org/3/c-api/stable.html mentions |
|
CC @ericsnowcurrently in case he has some comment about performances (see #2089). |
| CPU_ZERO(&cpu_set); | ||
| for (i = 0; i < seq_len; i++) { | ||
| PyObject *item = PySequence_Fast_GET_ITEM(py_cpu_seq, i); | ||
| PyObject *item = PySequence_GetItem(py_cpu_set, i); |
There was a problem hiding this comment.
| PyObject *item = PySequence_GetItem(py_cpu_set, i); | |
| PyObject *item = PySequence_GetItem(py_cpu_set, i); | |
| if (!item) { | |
| goto error; | |
| } |
(Previously, PySequence_Fast checked for errors while getting items.)
Hi! I wrote that page. Limited API is a bit slower, but it's still on the C level. If the code is primarily in Python, you're not likely to notice a slowdown. Tthe biggest threat of slowdown I know of is that common operations ( Still, if you run the benchmarks and they prove me wrong, I'd love to hear about that! |
Agreed! This should also help with the problem I described in gh-2089. As to performance, there are only some situations where it matters. I don't have anything useful to add to what @encukou said. 😄 (Thanks, @encukou!) Speaking of performance... If you have a little bit of time and performance regressions are important for this project to avoid then one thing you might consider is writing some meaningful benchmarks for it. It's relatively easy to write benchmarks that you can run using pyperformance: https://pyperformance.readthedocs.io/custom_benchmarks.html. Those benchmarks would live in your repo. Benchmarks that represent a high-level workloads (macro benchmarks) are best for identifying real-world performance improvements or regressions. Those that focus on specific features, capabilities, or APIs can be helpful to identify the source of regressions and the granular impact of a change. Both are useful. See https://github.com/faster-cpython/ideas/wiki/All-About-Python-Benchmarking for more info. Again, if performance is an important feature of your project then I recommend writing benchmarks specific to it. CC @mdboom |
Signed-off-by: mayeut <mayeut@users.noreply.github.com>
This PR adds Python 3.11 to the versions tested compared to It does not add Python 3.4 or 3.5 tests.
Up to you (but in another PR) depending on the level of support you want to give 3.4 (or 3.5) |
|
@encukou wrote:
It's nicely written. :) @encukou wrote:
Fair enough. Then I don't think we have a problem. The most "critical" parts I can think of are for/while loops where the C extension populates a list or dict in C, but nothing more. I assume that even with @ericsnowcurrently wrote:
Interesting. I don't think something that advanced is necessary here, but incidentally I'm working on another project where I may use something like that. Thanks. |
|
@mayeut given the comments above related to performance I don't think it's necessary to do further benchmarks, as I assume this work won't introduce any significant regression. Anyway, I have still improved the benchmark script a bit in 085691a (which eventually inspired #2157). Also, completely unrelated, since I see you're from Paris: I will be in Paris (first time) from Saturday to Tuesday for vacation. If you want to meet for a coffee and shake hands, feel free to shoot me an email at g.rodola@gmail.com. =) Merging. |
* 'master' of https://github.com/giampaolo/psutil: add windows test for free physical mem giampaolo#2074 fix OSX tests broken by accident update HISTORY + give CREDITS for @arossert, @smoofra, @mayeut for giampaolo#2102, giampaolo#2156, giampaolo#2010 build fix for Mac OS, Apple Silicon (giampaolo#2010) Linux: fix missing SPEED_UNKNOWN definition (giampaolo#2156) Use system-level values for Windows virtual memory (giampaolo#2077) feature: use ABI3 for cp36+ (giampaolo#2102) fix py2 compatibility improve API speed benchmark script giampaolo#2102 fix: linter issues from giampaolo#2146 (giampaolo#2155) chore: skip test_cpu_freq on macOS arm64 (giampaolo#2146) pre-release + give CREDITS to @mayeut (PR giampaolo#2040) and @eallrich (new supporter) Fix a typo (giampaolo#2047) move isort and coverage config into pyproject.toml fix giampaolo#2021, fix giampaolo#1954, provide OSX arm64 bins + add pyproject.toml (giampaolo#2040) refactor git_log.py
|
@mayeut I uploaded a new release on PYPI and got the following error when uploading Windows wheels on PYPI: I'm not sure why this happens only with Windows wheels. I suspect it has to do with the package |
|
This is the problem: Not sure why it affects only windows ABI3 wheels. |
|
@giampaolo, I'll take a look at this. |
|
Thanks. FYI setup.py removes the HTML data from README.rst, which is then used to set the |
Summary
fixes Generate Wheels for CPython 3.11 (or Use Limited API) #2089
Description
Use Limited API when building with CPython 3.6+ on Linux/macOS/Windows.
This allows to use pre-built wheels in all future versions of CPython 3.
Windows builds for CPython 3.6+ are moved from AppVeyor to GitHub Actions.
Diff of wheels produced: