API: Make PyArray_Chunk opaque on internal builds#30814
API: Make PyArray_Chunk opaque on internal builds#30814ngoldbaum wants to merge 1 commit intonumpy:mainfrom
Conversation
| typedef struct PyArray_Chunk PyArray_Chunk; | ||
| #else | ||
| typedef PyArray_Chunk_fields PyArray_Chunk; | ||
| #endif |
There was a problem hiding this comment.
Sorry, to some degree, I want to just keep going here (or the other PRs), but I a bit unsure what the aim is.
If the aim is to be able to compile downstream with the new stable ABI that is missing PyObject_HEAD, I would tend to simply not define PyArray_Chunk if PyObject_HEAD isn't defined. (Or I guess define it to an #error "harharhar PyArray_BufferConvert can't be used with this build")
If the goal is to compile NumPy with this (I am doubtful about this mid-term, but OK with preparing it long term), then we need a version of _fields that doesn't include PyObject_HEAD.
(Of course the alternative to just preparing is to put a deprecation warning now and making it "error only" in a year or so.)
There was a problem hiding this comment.
(But maybe this _fields definition is a way to prepare for all the _fields to omit PyObject_HEAD in a follow-up?)
EDIT: And to be clear, I am also OK with just saying "carry on" even if this is all a bit fuzzy but feels to you like it will help!
There was a problem hiding this comment.
And to be clear, I am also OK with just saying "carry on" even if this is all a bit fuzzy but feels to you like it will help!
I appreciate it!
Keep in mind that all of this work is in support of end-to-end tests of PEP 803 and is not intended to be mergeable.
For this PR I was mostly just trying to understand what the heck PyArray_Chunk even is, given the poor docs and that its based on concepts that were removed in Python 3 from the Python C API. I agree that it probably makes sense just not to expose this struct and PyArray_BufferConverter in an opaque pyobject limited API, should such a thing happen in reality.
FWIW, my plan (roughly) is:
- Make NumPy compilable and runnable with all PyObject-extending structs opaque in internal builds, adding accessors and setters as needed. This assumes that NumPy itself is a reasonably complete consumer of the NumPy C API.
- Octopus merge all the PRs
- Change all the NPY_INTERNAL_BUILD macros I added in the various draft PRs I have open to check
_Py_OPAQUE_PYOBJECTinstead. Possibly revert all the changes to numpy internals if we're worried about performance overhead of static inline functions.- Note that I tried testing this on Windows but got inconclusive, random benchmark results on some of the ufunc benchmarks and a bunch of the
bench_ufunc_stridesbenchmarks doing things like comparing a commit with a no-op commit that has the same content.
- Note that I tried testing this on Windows but got inconclusive, random benchmark results on some of the ufunc benchmarks and a bunch of the
- Actually try building a real-world Cython project that depends on NumPy using the opaque pyobject ABI.
Also, @kumaraditya303 is going to help out with this and may take over for me completely.
There was a problem hiding this comment.
I missed two important steps above:
- Ifdef-out the
PyObject_HEADpart of the fields structs on opaque pyobject builds. - Add C API functions that return pointers to the field structs, usable on the opaque pyobject build and add ifdefs for all the field accessor inline functions to call the API function on opaque pyobject builds.
There was a problem hiding this comment.
OK, cool. To me starting with the changes in NumPy internals (like these here) doesn't seem very necessary for the sake of making the headers _Py_OPAQUE_PYOBJECT compatible, but not complaining.
(Although, yeah, that testing all offset functions/macros 100% might be a bit hard without a pretty complete user as a basis.)
I didn't expose any accessors because I'm not sure this struct should be or needs to be public at all. c.f. #30813.