API: Make ``descr->f`` only accessible through ``PyDataType_GetArrFuncs`` by seberg · Pull Request #25812 · numpy/numpy

seberg · 2024-02-12T16:17:15Z

This is the next step in hiding/refactoring the PyArray_Descr struct. It:

Adds a PyDataType_GetArrFuncs function. Internally this is a static inline
function, but publically, it maps to the _PyDataType_GetArrFuncs API function
in a backwards compatible manner.

The unfortunate thing (does anyone have an idea?) thing here is that I had to "duplicate" the PyArray_GETITEM and PyArray_SETITEM into dtypemeta.h to pull this off, as there is no reasonable way to have diverging versions of the inline function PyDataType_GetArrFuncs.

Adds some docs, but I intentionally omitted docs in the 2.0 transition guide (they should be part of a larger fixups).

Marking as draft, as there will be small collisions with the previous PR.

(EDIT: Used a slightly modified version of my tool to do most of the MAINT: Access descr->f-> only through static inline function commit, I have not fixed the style everywhere.)

EDIT: Marked as ready for review, the order of the PRs doesn't really matter after all.

charris · 2024-02-13T17:11:06Z

doc/release/upcoming_changes/25812.c_api.rst

@@ -0,0 +1,5 @@
+* The ``->f`` slot has been removed from ``PyArray_Descr``.


You added the slot back.

I renamed it, and will remove it completely in a follow up, thought I would add the note here already.

ngoldbaum · 2024-02-13T18:10:46Z

Doesn't need to happen now but if not please add a note to make sure this gets discussed in the transition guide.

doc/release/upcoming_changes/25812.c_api.rst

doc/source/reference/c-api/types-and-structures.rst

numpy/_core/code_generators/numpy_api.py

numpy/_core/include/numpy/ndarraytypes.h

numpy/_core/src/multiarray/arraytypes.c.src

ngoldbaum · 2024-02-13T18:21:18Z

numpy/_core/src/multiarray/dtypemeta.c

+ * is only valid after checking for NULL.  Checking for NULL is generally
+ * encouraged.
+ *
+ * The public name of this is a macro until 1.x BC is irrelevant.


This helps but I'm still a little confused why we need to publicly expose a static inline function which calls thing function which calls another static inline function. I guess it's just dealing with naming conflicts but it would be nice to explicitly spell out the problem and maybe expand this comment a bit.

Internally, I thought an inline function is nicer for the simple pointer chasing that is going on.

Publically, an inline function isn't possible because the struct is fully opaque. So,

the public static inline is then needed allow defining it in a backcompat way.

I added a few sentences here to try to clarify it. I agree it is an unfortunate dance, necessary due to trying to hide it fully, but also needing the backcompat.

ngoldbaum · 2024-02-16T15:43:19Z

This is going to break scipy (looks like just to get at copyswap):

± rg "\->f\->"
scipy/signal/_lfilter.c.in
343:    PyArray_CopySwapFunc *copyswap = PyArray_DESCR((PyArrayObject *)x)->f->copyswap;
387:    PyArray_CopySwapFunc *copyswap = PyArray_DESCR((PyArrayObject *)x)->f->copyswap;

scipy/signal/_correlate_nd.c.in
206:    PyArray_CopySwapFunc *copyswap = PyArray_DESCR(curx->ao)->f->copyswap;

scipy/signal/_sigtoolsmodule.c
919:	copyswap = PyArray_DESCR(ret)->f->copyswap;

and pandas:

± rg "\->f\->"
pandas/_libs/src/vendored/ujson/python/objToJSON.c
408:  npyarr->getitem = (PyArray_GetItemFunc *)PyArray_DESCR(obj)->f->getitem;

I'm not sure what other downstream impact there will be but this is enough for me to say we shouldn't merge this without some discussion in the transition guide so there's an obvious place to point people to for migration instructions.

ngoldbaum

PyDataType_GetArrFuncs needs an entry in the C API functions doc too I think, unless the docstring you added automatically populates sphinx somehow.

doc/source/reference/c-api/types-and-structures.rst

seberg · 2024-02-17T17:50:40Z

I'm not sure what other downstream impact there will be but this is enough for me to say we shouldn't merge this without some discussion in the transition guide so there's an obvious place to point people to for migration instructions.

Can I please create a single brief entry at the end of this (elsize and alignment should change, too and the other PR already doesn't make sense to have seperate entries in the transition guide for)?
I opened gh-25820 for that reason, and really all of these changes are a single few sentence entry end as we just need to list the fields that now must use macros/function entries.

This is going to break scipy (looks like just to get at copyswap):

Yes, it is but, it is also going to break them in an easy to fix way (just as it was easy to change in NumPy everywhere).

ngoldbaum · 2024-02-17T17:52:47Z

Yes that's fine, we can expand it later.

ngoldbaum · 2024-02-18T02:36:50Z

In the interest of shipping the RC sooner rather than later, I'm going to merge this one now. We know this will break a few downstream projects. The fix is to use the new public API for accessing these functions. Please let us know if there's some corner case we didn't anticipate.

ngoldbaum · 2024-02-18T02:38:12Z

Oops, except there are conflicts. I'll fix this on Monday and merge it if @seberg doesn't get to it first.

This introduces ``PyDataType_GetArrFuncs`` as public and private API to fetch the ``PyArray_ArrFuncs *`` slot and thus allowing to remove it from ``PyArray_Descr``. Internally, the function is defined as inline in ``dtypemeta.h``.

Fetching the arrayfuncs now requires `dtypemeta.h` internally (externally, we force use of an API function call). So move these inline helpers also to `dtypemeta.h` as they cannot be defined without the header. (This is unfortunate, but I didn't have a better idea)

This allows removing the `->f` fields from the descriptor struct.

…nups)

Also, use C99 struct initializer to make that work.

seberg · 2024-02-18T13:57:26Z

doc/source/reference/c-api/types-and-structures.rst

 PyArray_ArrFuncs
 ----------------

+.. c:function:: PyArray_ArrFuncs *PyDataType_GetArrFuncs(PyArray_Descr *dtype)


PyDataType_GetArrFuncs needs an entry in the C API functions doc too I think

I had added a brief entry here.

ngoldbaum · 2024-02-18T17:24:15Z

Failures are unrelated, merging.

tylerjereddy · 2024-02-18T19:09:06Z

The compat include is a bit annoying downstream, #define NO_IMPORT_ARRAY usage in places might be related from the comments I see in that header. I'm working on it..

* Fixes scipygh-20107 * WIP for now because I don't like that I had to manually adjust the NumPy compat shim header... * I confirmed that this branch restored full build/test suite passing on x86_64 Linux with NumPy in the following scenarios: build with NumPy `main`, test with NumPy `main`; build with NumPy `main`, test with NumPy `1.26.4` (and 1.x only of course) * although I did apply the suggestions at or linked from: numpy/numpy#25812 I'm not happy about the fact that I have to modify `npy_2_compat.h` manually (this seems very much not ideal, perhaps I'll open an upstream issue) [skip cirrus] [skip circle]

seberg requested a review from ngoldbaum February 12, 2024 16:17

github-actions bot added the 30 - API label Feb 12, 2024

seberg force-pushed the set-getitem-rewire branch 6 times, most recently from 1995aa0 to bcb80b3 Compare February 12, 2024 18:31

seberg marked this pull request as ready for review February 13, 2024 09:36

charris reviewed Feb 13, 2024

View reviewed changes

ngoldbaum reviewed Feb 13, 2024

View reviewed changes

seberg force-pushed the set-getitem-rewire branch 4 times, most recently from 8cae64e to 35251dd Compare February 15, 2024 01:18

seberg requested a review from ngoldbaum February 16, 2024 14:49

rgommers added this to the 2.0.0 release milestone Feb 16, 2024

ngoldbaum reviewed Feb 16, 2024

View reviewed changes

doc/source/reference/c-api/types-and-structures.rst Show resolved Hide resolved

seberg added 6 commits February 18, 2024 14:55

MAINT: Make arrfuncs access full opaque from user side

314f29e

This introduces ``PyDataType_GetArrFuncs`` as public and private API to fetch the ``PyArray_ArrFuncs *`` slot and thus allowing to remove it from ``PyArray_Descr``. Internally, the function is defined as inline in ``dtypemeta.h``.

MAINT: Access descr->f-> only through static inline function

01b33fc

This allows removing the `->f` fields from the descriptor struct.

API: Completely remove ->f field from Descriptor struct (final clea…

531d899

…nups)

DOC: Update main documentation for ->f removal and add release note

b6511a0

MAINT: Fixup, GetArrFuncs must be static inline everywhere

d7cc437

seberg added 3 commits February 18, 2024 14:55

BUG: Must make wrapper static inline to match forward definition

02accfb

MAINT: Keep ->f slot around to not break downstream wheels right away

1a24d45

Also, use C99 struct initializer to make that work.

DOC: Add a bit of docs to _PyDataType_GetArrFuncs on why it exists

d028cc1

seberg commented Feb 18, 2024

View reviewed changes

seberg force-pushed the set-getitem-rewire branch from 35251dd to d028cc1 Compare February 18, 2024 13:59

ngoldbaum merged commit 441c9e6 into numpy:main Feb 18, 2024

This was referenced Feb 18, 2024

MAINT: shim for descr->f access scipy/scipy#20108

Merged

MAINT, DOC: npy_2_compat.h header shims/docs? #25850

Closed

rgommers mentioned this pull request Feb 20, 2024

Revert "API: Make descr->f only accessible through PyDataType_GetArrFuncs" #25862

Merged

		@@ -0,0 +1,5 @@
		* The ``->f`` slot has been removed from ``PyArray_Descr``.

Uh oh!

Conversation

seberg commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charris Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

seberg Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Feb 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ngoldbaum Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

seberg Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

seberg Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Feb 16, 2024

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seberg commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum commented Feb 17, 2024

Uh oh!

ngoldbaum commented Feb 18, 2024

Uh oh!

ngoldbaum commented Feb 18, 2024

Uh oh!

seberg Feb 18, 2024

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Feb 18, 2024

Uh oh!

tylerjereddy commented Feb 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seberg commented Feb 12, 2024 •

edited

Loading

seberg commented Feb 17, 2024 •

edited

Loading