Closed astrofrog closed 1 year ago
@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
.
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.
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:
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)
For anyone coming to this now, see my latest summary of changes in https://github.com/liberfa/pyerfa/pull/94#issuecomment-1710775593
Note that following a comment by @mhvk I've pushed a temporary commit to see what fails if I remove the typestruct workaround.
Thanks @astrofrog
@astrofrog - I think this is OK to merge once the numpy/python 3.12 issue is sorted! Thanks!
@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 (https://github.com/liberfa/pyerfa/pull/94/commits/da319b5c4447070c3481dff487945b422e3e2aed). The wheels are now tested on Python 3.12. I believe this is therefore ready to merge assuming the CI passes.
Ah I need to add a tweak for win32, will do shortly
numpy is going to push out wheel for win32 "soon", if you can wait a bit.
Workaround removed - this should be ready if the CI passes
@mhvk @avalentino are you happy for this to be merged now?
yes
@mhvk @pllim - I think this is finally ready
Does switching to the limited API imply anything for performance?
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!
Doesn't this also limit what code Cython can produce?
Ah, sorry, this doesn't use cython, nevermind then
Belated LGTM. Thanks!
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 ofPyMem_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 usingPyMem_malloc
ormalloc
?@mhvk - as a Numpy expert, do you have any ideas?