MNT: use pythoncapi_compat.h in npy_compat.h #26189
Conversation
|
In the past we used very few macro definitions and that also felt much to use a submodule (and the full file which has a lot of things we don't need). I suspect we still only use a small, but significantly larger, fraction of them all and we have a lot of submodules now anyway. |
There was a problem hiding this comment.
Yes, seems a sensible way to go. I guess eventually the submodule will disappear again, once borrowed references are a thing of the past? And that any actual changes in the code to use new routines that give new references will be done in follow-up?
p.s. I had to do a bit of a double-take since you seemed to remove the usage of the headers that you were introducing, but that just meant I should have read your comments better...
Maybe but I don’t see the CPython C API remaining static after the nogil effort over the next few years, and this way we don’t need to maintain our own compatibility shims. I also like these shims because their names match the upstream names so they don’t need another migration in the future to get rid of them besides removing the compat header include. |
There was a problem hiding this comment.
This LGTM overall, thanks @ngoldbaum. _core/src/common seems like the right place to put this. I have one suggestion to avoid contributors being puzzled after a git pull and missing the submodule - add this near the top of numpy/_core/meson.build:
if not fs.exists('src/common/pythoncapi-compat')
error('Missing the `pythoncapi-compat` git submodule! Run `git submodule update --init` to fix this.')
endifThere are a couple of other checks like that already, like for svml and highway.
Following on from #26158 and #26159, this adds a new submodule for
pythoncapi_compat.hand includes it innpy_pycompat.h. I also removed all the spots wherenpy_pycompat.hwas getting included unnecessarily, probably leftovers from the python 2->3 migrations.This doesn't actually use the new header yet, that will come in followups.