ENH: Add per-extension Limited API toggle and port _pocketfft_umath to be limited api compatible#31910
ENH: Add per-extension Limited API toggle and port _pocketfft_umath to be limited api compatible#31910prathamhole14 wants to merge 5 commits into
_pocketfft_umath to be limited api compatible#31910Conversation
bc7671e to
75b9580
Compare
rgommers
left a comment
There was a problem hiding this comment.
Thanks for starting to look into the limited API topic @prathamhole14, looking forward to seeing how far that can be pushed.
This PR isn't mergeable I'm afraid. We should not actually convert individual extensions to already compile to abi3.so extensions. That makes the meson.build files more complex, potentially causes some slowdowns (not in this case, but in general), and has no upsides.
If C/C++-level changes are needed, I think those we can make. The two changes here seem unnecessary though. When you find that source-level changes are needed, that is usually to avoid APIs that aren't part of the Limited C API. At that point the most interesting question (after it builds) is what the performance cost of the changes is, if any - so benchmarks are usually good to include.
| static struct PyModuleDef_Slot _pocketfft_umath_slots[] = { | ||
| {Py_mod_exec, (void*)_pocketfft_umath_exec}, | ||
| #if PY_VERSION_HEX >= 0x030c00f0 // Python 3.12+ | ||
| #if PY_VERSION_HEX >= 0x030c00f0 && (!defined(Py_LIMITED_API) || Py_LIMITED_API >= 0x030c0000) // Python 3.12+ |
There was a problem hiding this comment.
Both changes to this file look unnecessary - it should compile fine without this?
There was a problem hiding this comment.
OH yes the first one is unnecessary as min required version is 3.12
But I think the second one does require the check, because if we set Py_LIMITED_API=3.12, the 3.13 slot gets hidden, and it will fail to compile on Python 3.13 without that check?
I tested it on 3.13, I get the following error.
../../numpy/fft/_pocketfft_umath.cpp:427:6: error: 'Py_mod_gil' was not declared in this scope
427 | {Py_mod_gil, Py_MOD_GIL_NOT_USED},
| ^~~~~~~~~~
../../numpy/fft/_pocketfft_umath.cpp:427:18: error: 'Py_MOD_GIL_NOT_USED' was not declared in this scope
427 | {Py_mod_gil, Py_MOD_GIL_NOT_USED},
| ^~~~~~~~~~~~~~~~~~~
|
It may be useful to include a tracker in gh-31913 per extension module. This one can be marked off as working in principle I'd say. |
|
If it gets too annoying to work on this without being able to toggle per-extension-module support, we can add build system support along the lines of the PyWavelets/pywt#828. It will be opt-in and a private build option flag, so it's hidden except for the purposes of working on this topic. Then the only change per extension module should be to add one line like: limited_api: py_limited_api,with |
|
Thanks you @rgommers for the detailed reviews.
limited_api: py_limited_api,
Yes it makes more sense to do this, just pushed the code in the last commit for this. For now I have set the default to false so we won't be shipping |
_pocketfft_umath compile with Limited API_pocketfft_umath to be limited api compatible
I ran the benchmarks (spin bench -c main -t bench_core) after installing |
|
Yes, if you do not carefully set up your machine for benchmarking, benchmarks with microsecond timings can vary 10-20%. Note your changes only touch fft, so this run is a good way to show the variability of your system. Laptops with no fans are particularly vulnerable to heat-related clock governing changes. |
PR summary
Background: Since
_pocketfft_umathis smaller I decided to tackle it first but realized there is no need for refractor to structs there.Therefore I decided to add
#define Py_LIMITED_API 0x030c0000in the cpp file and compile which worked fine on my machine (linux-ubuntu) after a few tweaks, but failed on CI -- instead of the preprocessor,limited_apiis to be configured inmeson.buildtherefore I followed the pattern from the below (since checks on CI failed with#error "The limited API is not currently supported in the free-threaded build".numpy/numpy/_core/tests/examples/limited_api/meson.build
Line 39 in da679e7
Related to: #31913
CC: @mattip
Edit: To create
abi3.socompiled files (could be for testing only) i.e. modules with limited api, use-Dpython.allow_limited_api=trueflag while installing or build numpy.AI Disclosure
None