Make use of CPython limited API #94

astrofrog · 2022-10-24T22:45:27Z

This is meant to be an experiment in trying to see if we can use the Limited API - doing so would mean we would only need to build one wheel for each platform (rather than one per Python version and per platform). Note that this wouldn't require any changes to the wheel building machinery as cibuildwheel recognizes this and will only build one wheel and test it on all Python versions.

I am running into the issue that PyArray_malloc is an alias of PyMem_RawMalloc which isn't in the Limited API. I was wondering if there is a way we can allocate the arrays that would be compatible with the Limited API, for example using PyMem_malloc or malloc?

@mhvk - as a Numpy expert, do you have any ideas?

mhvk · 2022-10-25T00:44:29Z

@astrofrog - there definitely wasn't put much thought in this, so I think we can indeed change it. It is done inside a ufunc, which means the GIL is released, so given the comment in https://github.com/numpy/numpy/blob/8d61ebc25a117337d148f1e3d96066653bd6419a/numpy/core/include/numpy/ndarraytypes.h#L359-L368, I don't think we can use the non-raw PyMem function. But given that the actual memory allocation typically is very small, it should not be a problem to just use malloc.

pllim · 2023-08-18T16:10:26Z

setup.cfg

@@ -56,3 +56,6 @@ max-line-length = 100

 [pycodestyle]
 max-line-length = 100
+
+[bdist_wheel]
+py_limited_api = cp37


I bumped Python minversion to 3.9 in #107 , so this needs updating. FYI

astrofrog · 2023-09-07T20:53:09Z

Switching to using malloc() seems to fix things - after a couple of other fixes this seems to work.

Note that I've also simplified the wheel building config to not split up the different Python versions into separate builds. When using the limited API, cibuildwheel will first build e.g. the Python 3.9 wheel and then just test it on 3.10, 3.11 and 3.12 (but not build it again each time). But note that for now Python 3.12 probably won't work because of Numpy. So I have disabled Python 3.12 in the cibuildwheel config - however note that this doesn't actually matter as much as before because we only build wheels on Python 3.9 anyway - 3.10, 3.11 and 3.12 is just for testing.

astrofrog · 2023-09-07T21:11:02Z

Well this seems like it could actually be ready (modulo review). The TL;DR is that this enables use of the limited CPython API which means that we can just build a wheel with the oldest supported version of CPython and it will then work with any future Python 3.x version. This is recognized by cibuildwheel which will just build the wheel for Python 3.9 and then test it on 3.9, 3.10, 3.11, etc. The benefits are:

Reduced CI time for building wheels (only build once per platform/arch)
No need to build wheels every time a new Python version is released
Fewer files to upload to PyPI

For now Python 3.12 testing of the wheels is disabled but we can just enable it once there are stable releases of Numpy that work on Python 3.12 (but we don't need to hold back this PR until then)

astrofrog · 2023-09-07T21:12:34Z

.github/workflows/ci_workflows.yml

@@ -113,7 +113,7 @@ jobs:
        python -m venv --system-site-packages tests
        source tests/bin/activate
        python -m pip install --editable .[test]
-        (nm -u erfa/ufunc.cpython-*.so | grep eraA2af) || exit 1
+        (nm -u erfa/ufunc.*.so | grep eraA2af) || exit 1


The so files are now called ufunc.abi3.so hence the change

erfa/ufunc.c.templ

pllim · 2023-09-07T21:32:03Z

.github/workflows/ci_workflows.yml

-      # Required so that cp312 can grab a pre-release numpy.
-      # Can remove when cp312 is released.
-      env: |
-        CIBW_ENVIRONMENT: PIP_PRE=1


I don't grok the internals of erfa, so I cannot review but I wonder why you remove the extra job for py312. I split it out on purpose because py312 needs pre-release of numpy.

See #94 (comment) - with the limited API we don't build Python 3.12 wheels anymore. We just build 3.9 wheels and test them on 3.10+. Testing them on 3.12 is disabled for now because we can't have Python 3.12 as a separate job anymore with this PR.

If you would be more comfortable waiting for a Python 3.12 release of Numpy we can do that, but it's not strictly needed - if we know via other CI config that Python 3.12 works fine then testing the wheels on 3.9, 3.10 and 3.11 is probably sufficient.

Hmm... What about using artifact upload/download for the py312? Though maybe numpy will ship py312 wheel anyway when 1.26 is released, so maybe we can just wait it out. Thanks for the clarification!

Happy to just wait for 1.26 final, no point overcomplicating things.

astrofrog · 2023-09-07T21:55:00Z

For anyone coming to this now, see my latest summary of changes in #94 (comment)

mhvk

Looks good to me but one question and one request for a comment in the code.

I agree that we should wait until py312 compatible numpy is available.

erfa/ufunc.c.templ

astrofrog · 2023-09-08T13:03:25Z

Note that following a comment by @mhvk I've pushed a temporary commit to see what fails if I remove the typestruct workaround.

avalentino

LGTM

avalentino · 2023-09-08T15:42:16Z

Thanks @astrofrog

mhvk · 2023-09-08T16:30:15Z

@astrofrog - I think this is OK to merge once the numpy/python 3.12 issue is sorted! Thanks!

astrofrog · 2023-09-08T19:12:07Z

@mhvk @avalentino @pllim - I just realized there is an easy way to request pre-releases on Python 3.12 similar to what @pllim was doing before without having to split up the build matrix so have pushed a commit here (da319b5). The wheels are now tested on Python 3.12. I believe this is therefore ready to merge assuming the CI passes.

pllim · 2023-09-08T19:18:34Z

pyproject.toml

+
+[[tool.cibuildwheel.overrides]]
+select = "cp312-*"
+environment = "PIP_PRE=1"


Does this mean all the wheels now use numpy 1.26rc or just cp312?

Yes on 3.12 the wheels are tested against the numpy RC. Note that we don't build any Python 3.12 wheels so the build dependencies are not important there.

p.s. Are you going to do a similar patch for astropy?

Haha unfortunately not, Cython's support for the limited API is very rudimentary

astrofrog · 2023-09-08T19:25:50Z

Ah I need to add a tweak for win32, will do shortly

pllim · 2023-09-08T19:29:57Z

numpy is going to push out wheel for win32 "soon", if you can wait a bit.

pllim

numpy 1.26 stable is out with py312 wheels. I think this can be simplified now.

astrofrog · 2023-09-21T21:02:56Z

Workaround removed - this should be ready if the CI passes

astrofrog · 2023-09-23T21:16:07Z

@mhvk @avalentino are you happy for this to be merged now?

avalentino · 2023-09-24T05:53:22Z

yes

mhvk

A question whether we still need to skip python 3.12, just to be sure. If it is needed, do we need an issue to remind us to remove it?

Otherwise, a very nice simplification in our wheel building, for a single, simple change. Great! Ready to (squash&) merge!

mhvk · 2023-09-24T07:24:53Z

pyproject.toml

@@ -11,3 +11,6 @@ requires = [
    "numpy>=1.26.0b1; python_version>='3.12'"
 ]
 build-backend = 'setuptools.build_meta'
+
+[tool.cibuildwheel]


Do we still need this now that numpy 1.26 is out?

Oops no will remove

pllim · 2023-09-24T14:42:28Z

pyproject.toml

I think you need a rebase to pick up my merged changes to this file.

astrofrog · 2023-09-24T21:12:12Z

@mhvk @pllim - I think this is finally ready

mhvk

OK, let's get it in! Thanks, @astrofrog!

maxnoe · 2023-09-25T08:20:26Z

Does switching to the limited API imply anything for performance?

astrofrog · 2023-09-25T10:19:53Z

It shouldn't as the code is virtually unchanged (the one function that was changed was an alias to malloc anyway). But if anyone finds a performance degradation let me know!

maxnoe · 2023-09-25T10:29:29Z

Doesn't this also limit what code Cython can produce?

maxnoe · 2023-09-25T10:31:02Z

Ah, sorry, this doesn't use cython, nevermind then

pllim · 2023-09-25T16:15:07Z

Belated LGTM. Thanks!

pllim reviewed Aug 18, 2023

View reviewed changes

astrofrog force-pushed the limited-api branch from dd30e9a to 978feef Compare September 7, 2023 20:37

astrofrog changed the title ~~Experiment with Python Limited API~~ Make use of cpython limited API Sep 7, 2023

astrofrog changed the title ~~Make use of cpython limited API~~ Make use of CPython limited API Sep 7, 2023

astrofrog marked this pull request as ready for review September 7, 2023 21:07

astrofrog requested review from pllim, avalentino and mhvk and removed request for pllim September 7, 2023 21:11

astrofrog commented Sep 7, 2023

View reviewed changes

erfa/ufunc.c.templ Show resolved Hide resolved

pllim reviewed Sep 7, 2023

View reviewed changes

mhvk reviewed Sep 8, 2023

View reviewed changes

erfa/ufunc.c.templ Show resolved Hide resolved

erfa/ufunc.c.templ Show resolved Hide resolved

astrofrog force-pushed the limited-api branch from 7db3702 to 6b3b642 Compare September 8, 2023 13:10

avalentino approved these changes Sep 8, 2023

View reviewed changes

pllim reviewed Sep 8, 2023

View reviewed changes

pllim reviewed Sep 20, 2023

View reviewed changes

astrofrog force-pushed the limited-api branch from da319b5 to 0595682 Compare September 21, 2023 21:02

mhvk approved these changes Sep 24, 2023

View reviewed changes

pllim reviewed Sep 24, 2023

View reviewed changes

pyproject.toml Outdated

Copy link

Contributor

pllim Sep 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need a rebase to pick up my merged changes to this file.

astrofrog added 10 commits September 24, 2023 21:10

Enable Python Limited API

42a7a74

Add workaround for _typeobject as in lpsinger/ligo.skymap@b1bc1ab

f8936e5

Use malloc instead of PyArray_malloc

6d96643

Bump py_limited_api version

c72b772

Updated Py_LIMITED_API

e156542

Updated shared library path in workflow

5a356fd

Try and fix Windows compilation error

afceebb

Simplify wheel building config

36bcee0

Added comment about workaround

3460592

Added note about malloc()

844a5dc

astrofrog force-pushed the limited-api branch from 0595682 to 844a5dc Compare September 24, 2023 20:14

mhvk approved these changes Sep 25, 2023

View reviewed changes

mhvk merged commit 9276257 into liberfa:master Sep 25, 2023
19 checks passed

pllim mentioned this pull request Oct 14, 2023

Remove constraint on numpy in pyproject.toml? #119

Closed

Make use of CPython limited API #94

Make use of CPython limited API #94

Conversation

astrofrog commented Oct 24, 2022

mhvk commented Oct 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Sep 7, 2023 • edited Loading

astrofrog commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Sep 7, 2023

mhvk left a comment

Choose a reason for hiding this comment

astrofrog commented Sep 8, 2023

avalentino left a comment

Choose a reason for hiding this comment

avalentino commented Sep 8, 2023

mhvk commented Sep 8, 2023

astrofrog commented Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Sep 8, 2023

pllim commented Sep 8, 2023

pllim left a comment

Choose a reason for hiding this comment

astrofrog commented Sep 21, 2023

astrofrog commented Sep 23, 2023

avalentino commented Sep 24, 2023

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Sep 24, 2023

mhvk left a comment

Choose a reason for hiding this comment

maxnoe commented Sep 25, 2023

astrofrog commented Sep 25, 2023

maxnoe commented Sep 25, 2023

maxnoe commented Sep 25, 2023

pllim commented Sep 25, 2023

astrofrog commented Sep 7, 2023 •

edited

Loading

astrofrog commented Sep 8, 2023 •

edited

Loading