API: Expose the dtype C API #25754

ngoldbaum · 2024-02-02T18:51:19Z

This exposes the DType API publicly. The functions are exposed using the codegen like the rest of the numpy API. The types were not straightofward to expose using the codegen, so I did it manually, keeping a separate PyCapsule containing a separate API table. Both the main array API table and the new dtype API table are exposed using import_array().

mattip · 2024-02-03T19:38:38Z

Are all these APIs required to be made public in order to enable user-defined DTypes?

mattip · 2024-02-03T19:46:34Z

Could you divide this into two PRs:

exposing APIs that can work with LIMITED_API
exposing APIs that cannot work with LIMITED_API

ngoldbaum · 2024-02-03T21:02:04Z

Unfortunately you need to access the dtypemeta struct to initialize a custom dtype. See e.g. here. You also need to use the APIs that take PyArray_DTypeMeta arguments, in particular the initialize from spec function to initialize the user's custom PyArray_DTypeMeta subtype.

@seberg suggested on Slack to add a typedef for PyArray_DTypeMeta to PyTypeObject when Py_LIMITED_API is set. That will allow exposing the APIs, but anyone who wants to use it to define a custom dtype, which requires accessing the real struct, won't be able to use the limited API macro.

Users of the custom dtypes from python won't care. I think that would let me get rid of all the ifdefs in dtype.h and still keep limited API support.

Does that sound reasonable?

I guess we could also provide macros that allow setting and getting the dtypemeta attributes and make PyArray_DTypeMeta opaque but I haven't thought very carefully about that. My understanding is that ultimately we'd be able to use the real APIs for setting up heap types, which are in the limited API but were buggy for metaclasses until python 3.11 - sebastian knows more details about that than me.

ngoldbaum · 2024-02-05T20:49:42Z

Nice, it looks like that fix works, see last commit.

ngoldbaum · 2024-02-06T22:27:06Z

This should be ready to review as far as the code changes go. I still need to look at the documentation. I'm going to try to get the doxygen integration working (although that might be a pain because the codegen forces me to break the doxygen formatting slightly). If that doesn't work, I'll just add the docstrings to the sphinx API docs manually.

ngoldbaum · 2024-02-06T22:29:09Z

One question I have: how should I test that my changes to import_array() work when run against old versions of NumPy? Build an extension against NumPy 2 and run it against e.g. Numpy 1.26?

ngoldbaum · 2024-02-07T02:45:08Z

Excellent, tests are green :)

ngoldbaum · 2024-02-09T18:44:59Z

numpy/_core/include/numpy/experimental_dtype_api.h

-#define PyArray_CDoubleDType (*(PyArray_DTypeMeta *)__experimental_dtype_api_table[36])
-#define PyArray_CLongDoubleDType (*(PyArray_DTypeMeta *)__experimental_dtype_api_table[37])
-/* String/Bytes */
-#define PyArray_StringDType (*(PyArray_DTypeMeta *)__experimental_dtype_api_table[38])


I never noticed until now but I think it's a bug that we exposed this name in the public DType API. I changed it to PyArray_BytesDType instead to avoid the python2 baggage, which is its internal name. This is being newly exposed so let's give it a name that makes sense in Python3.

ngoldbaum · 2024-02-09T19:57:47Z

numpy/_core/include/numpy/dtype_api.h

+typedef PyTypeObject PyArray_DTypeMeta;
+
+#endif /* Py_LIMITED_API */
+


@seberg there are some comments below about "expected changes" for the ArrayMethod API. Do you still want to eventually do those changes? Now is probably the time...

If you don't notice anything that should change, probably not a big worry. All of this is designed so that we can deprecate/replace it after all.

ngoldbaum · 2024-02-09T21:01:23Z

numpy/_core/include/numpy/dtype_api.h

@@ -141,6 +149,7 @@ typedef struct {
 #define NPY_METH_unaligned_contiguous_loop 8
 #define NPY_METH_contiguous_indexed_loop 9

+


_NPY_METH_get_loop is still marked as private. Maybe we should just make it public given it's pretty useful?

yah, got to make it public, but we could follow up also to keep the diff smaller.

(There are a few things I dislike about it, but we aren't gonna change them quickly, and that is OK)

ngoldbaum · 2024-02-09T21:03:26Z

numpy/_core/include/numpy/dtype_api.h

@@ -187,6 +196,7 @@ typedef NPY_CASTING (resolve_descriptors_with_scalars_function)(
        npy_intp *view_offset);


+


Should we give the other typedefs in this file names like PyArrayMethod_ResolveDescriptorsFunction?

doc/source/reference/c-api/array.rst

ngoldbaum · 2024-02-10T02:31:20Z

The main thing left now is the typedefs, because I think we probably want to give them names in the PyArray namespace somewhere but wanted confirmation on that before doing it.

seberg · 2024-02-10T11:27:21Z

numpy/_core/src/multiarray/public_dtype_api.c

+    api_table[37] = &PyArray_PyComplexAbstractDType;
+    api_table[38] = &PyArray_DefaultIntDType;
+    /* Non-legacy DTypes that are built in to NumPy */
+    api_table[39] = &PyArray_StringDType;


So I think I had suggested that before, but just mentionig it and hoping @mattip can voice an opinion:

An alternative that I rather like would be to add a big chunk of "empty slots" with a common to the api-gen (i.e. just with a + list(range(...))) and then fill the normal API table here and above with that offset.

There isn't a huge reason for it, but I somehwat like the idea of having fewer ~~structs~~ capsules (in fact, I would support duplicating the ufunc API in the struct so that at some point we can consolidate it all into one).

I may have some tweaks if I review the API, but I itself, but I would prefer to just push this through, and the docs look nice on a quick glance.
(Tweaking this API is completely fine also after RC, the 0 people using it in their wheels already then, can recompile :))

doc/source/reference/c-api/array.rst

seberg · 2024-02-12T18:48:25Z

One question I have: how should I test that my changes to import_array() work when run against old versions of NumPy?

Don't worry about it too much, downstream runs these tests on their CI.

ngoldbaum · 2024-02-12T19:43:02Z

OK awesome. The last thing that needs to be documented is the typedefs. We have PyArrayMethod_StridedLoop already but all the other typedefs have names like resolve_descriptors_function. Do you agree that we should change the names of all the typedefs to use camel cased names in the PyArrayMethod namespace?

seberg · 2024-02-12T19:45:34Z

Do you agree that we should change the names of all the typedefs to use camel cased names in the PyArrayMethod namespace?

That sounds like a good idea.

ngoldbaum · 2024-02-13T22:06:56Z

I think this is fully ready now.

ngoldbaum · 2024-02-13T22:14:36Z

Also as a followup we're going to need to fix all the dtypes in numpy-user-dtypes after this is merged. We should probably also add a noisy cron CI job there to let us know if we break public API.

seberg · 2024-02-13T22:17:58Z

I think this is fully ready now.

Awesome, I'll have a look tomorrow, would like to get this in soon even soon (even if we notice some follow ups, which also goes for any naming bike-shedding: happy to apply any before the final release).

(I think I may try to see how ugly it is to just fill up a gap in the dtype table, rather than exporting a second capsule, since I don't love the idea of exporting two capsules.)

ngoldbaum · 2024-02-13T22:29:22Z

The doc build appears to have passed, but just a note that on my local mac setup I'm now hitting sphinx-doc/sphinx#11449 when I do spin docs locally. Disabling parallel builds fixes it. There must be something in here that sphinx isn't happy about for whatever reason. The discussion in that issue is very confusing so I'm not sure what to do about it.

ngoldbaum · 2024-02-13T23:02:18Z

just a note that on my local mac setup I'm now hitting sphinx-doc/sphinx#11449 when I do spin docs locally

I ended up figuring out this was because of seg fault coming from IPython of all places, see sphinx-doc/sphinx#11449 (comment). Anyway, probably nothing to do with this PR.

seberg

I have pushed a small doc fixup and the second commit which removes the additional capsule and adds the new API directly (by just reserving slots 320-360 for these, the last of which is empty, but I thought it doesn't matter).

I also moved the table lookup defines into _public_dtype_api_table.h just to try to not bloat that generate_api.py script.

I think I prefer that quite a bit, not because it is much prettier code-wise, but because I don't like juggling multiple capsules.

I can see renaming some of the function definitions to include Func at the end (not the ones with Loop), and we should see if anyone has any other suggestions.

But for me: I think we can merge it now, ping the mailing list and create issues for anything else. As this is completely new API, I do not see a problem with changing some things during RC phase, also.

mhvk

Had a look at the new documentation and mostly found some typos, but also some requests for clarification.

Overall, very nice!

I guess one todo item will be to ensure the same user dtypes are updated to the new names...

mhvk · 2024-02-14T15:09:27Z

doc/source/reference/c-api/array.rst

+
+   The unaligned loops are currently only used in casts and will never be picked
+   in ufuncs (ufuncs create a temporary copy to ensure aligned inputs).
+   These flags are ignored when ``NPY_METH_get_loop`` is defined, where instead


These flags -> These functions

changed it to "slot IDs", which I think is a little more accurate than "functions".

doc/source/reference/c-api/array.rst

mhvk · 2024-02-14T15:10:58Z

doc/source/reference/c-api/array.rst

+   .. c:enumerator:: NPY_METH_REQUIRES_PYAPI
+
+      Indicates the method must hold the GIL. If this flag is not set, the GIL
+      is released before the ufunc is called.


ufunc -> loop? (or does this not hold for casts?)

mhvk · 2024-02-14T15:11:27Z

doc/source/reference/c-api/array.rst

+   .. c:enumerator:: NPY_METH_NO_FLOATINGPOINT_ERRORS
+
+      Indicates the method cannot generate floating errors, so checking for
+      floating errors after the ufunc completes can be skipped.


ufunc -> loop

mhvk · 2024-02-14T15:14:08Z

doc/source/reference/c-api/array.rst

+   created for and the concrete descriptor instances used at runtime, determines
+   the correct descriptors to pass into the loop implementation, which may or
+   may not be the descriptors passed in by the user. The *method* is an opaque
+   pointer to the underlying cast or ufunc loop which is currently exposed


what does "which is currently exposed publicly" mean here? I would have expected a (link to a) description of how to use this inside one's own resolution loop.

typo, should have been "currently not exposed publicly".

doc/source/reference/c-api/array.rst

mhvk · 2024-02-14T15:36:38Z

doc/source/reference/c-api/array.rst

+^^^^^^^^^^^^^^^^^^^^^^
+
+In addition the above slots, the following slots are exposed to allow
+filling the ``PyArray_ArrFuncs`` struct attached to descriptor


Would be good to have a working link to PyArray_ArrFuncs here. Also, are these still necessary, or just there for backwards compatibility? (e.g., dotfunc seems pretty pointless, but compare likely useful)

It's not just backcompat: we should replace them all, but we don't have that yet (e.g. for sorts/compare).

Added a note that this will likely change in the future.

ah, missed that - so ignore my request just now to add such a note!

mhvk · 2024-02-14T15:37:35Z

doc/source/reference/c-api/array.rst

+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+These macros and static inline functions are provided to allow more
+understandable and iomatic code when working with ``PyArray_DTypeMeta``


iomatic -> idiomatic

mhvk · 2024-02-14T15:39:13Z

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

@@ -48,14 +48,17 @@ supportive role: the :c:data:`PyArrayIter_Type`, the
 . The :c:data:`PyArrayIter_Type` is the type for a flat iterator for an
 ndarray (the object that is returned when getting the flat
 attribute). The :c:data:`PyArrayMultiIter_Type` is the type of the
-object returned when calling ``broadcast`` (). It handles iteration
-and broadcasting over a collection of nested sequences. Also, the
+object returned when calling ``broadcast`` (). It handles iteration and


Why the ()? (not new, I know)

no idea, deleted it

mhvk · 2024-02-14T15:42:37Z

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

+          Corresponds to the type number for legacy data types. Data
+          types defined outside of NumPy and possibly future data types
+          shipped with NumPy will have ``type_num`` set to -1, so this
+          should be relied on to descriminate between data types.


should not be relied on?

ngoldbaum · 2024-02-14T18:51:39Z

The last commit responds to all of Marten's comments.

seberg · 2024-02-14T18:54:07Z

Should we get this in once things pass, just so I can rebase the other ones? (Happy if there are follow up changes.) Assuming you were happy with my API table reorganization.

ngoldbaum · 2024-02-14T19:02:13Z

Yeah no worries about the reorganization, the only reason I didn't do it myself is because I had docs to write still

doc/source/reference/c-api/array.rst

mhvk

Beyond the question whether things are c:type or c:function, I think you answered/dealt with all my queries, thanks!

…s doc

ngoldbaum · 2024-02-14T19:25:32Z

I missed a doc build warning which the last commit fixes, along with a reorganization that moves some old docs into a place that makes more sense, allowing me to add a new section heading that I can link to from another page.

ngoldbaum · 2024-02-14T19:42:30Z

@seberg did you want me to ping the mailing list before we merge this? or only after to alert everyone that it got merged?

seberg · 2024-02-14T20:12:47Z

I thought it's more useful aftr as the docs are build?

ngoldbaum · 2024-02-14T20:16:08Z

I could link to the doc build for this PR: https://output.circle-artifacts.com/output/job/39bc55ca-c9e3-42e5-940d-6224c4c903c8/artifacts/0/doc/build/html/reference/c-api/array.html#arraymethod-api

mhvk · 2024-02-14T20:28:53Z

My 2¢: merge and ask for comments then - even a major refactoring would at this stage be better done in a different PR!

mhvk · 2024-02-14T20:29:52Z

p.s. Probably good to note in any message to the mailing list that the new dtypes underlie the new StringDType as well as proper support for string ufuncs.

ngoldbaum · 2024-02-14T20:56:01Z

OK, fair enough, let's hit the merge button to unblock the other string PRs.

API: Expose the dtype C API

ngoldbaum requested review from seberg and mattip February 2, 2024 18:51

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

ngoldbaum force-pushed the expose-dtype-api branch from dfb8332 to 20b3a81 Compare February 6, 2024 22:23

ngoldbaum marked this pull request as ready for review February 6, 2024 22:27

ngoldbaum changed the title ~~API: Expose the dtype C API [WIP]~~ API: Expose the dtype C API Feb 6, 2024

ngoldbaum added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Feb 7, 2024

ngoldbaum force-pushed the expose-dtype-api branch from e680875 to 2b36426 Compare February 9, 2024 18:43

ngoldbaum commented Feb 9, 2024

View reviewed changes

ngoldbaum commented Feb 10, 2024

View reviewed changes

doc/source/reference/c-api/array.rst Outdated Show resolved Hide resolved

seberg reviewed Feb 10, 2024

View reviewed changes

mhvk mentioned this pull request Feb 10, 2024

MAINT: combine string ufuncs by passing on auxilliary data #25796

Merged

seberg reviewed Feb 11, 2024

View reviewed changes

doc/source/reference/c-api/array.rst Outdated Show resolved Hide resolved

ngoldbaum added 3 commits February 13, 2024 11:50

API: expose the DType API publicly

fee9578

DOC: begin dtype API docs

f95657c

DOC: remove copy/paste error from docstring

6f3b0eb

ngoldbaum removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Feb 13, 2024

DOC: fix some sphinx issues

61665da

DOC: Do some minor doc changes to newly exposed DType/UFunc API

486c118

seberg force-pushed the expose-dtype-api branch from 4d46968 to 5c96ba8 Compare February 14, 2024 12:21

MAINT: Manually insert DTypes into the existing table

eb01d23

seberg force-pushed the expose-dtype-api branch from 5c96ba8 to eb01d23 Compare February 14, 2024 12:22

seberg approved these changes Feb 14, 2024

View reviewed changes

mhvk reviewed Feb 14, 2024

View reviewed changes

DOC: respond to review comments

7dba091

mhvk reviewed Feb 14, 2024

View reviewed changes

doc/source/reference/c-api/array.rst Show resolved Hide resolved

mhvk reviewed Feb 14, 2024

View reviewed changes

DOC: rearrange types and structures page so I can link to the ArrFunc…

228e4a5

…s doc

ngoldbaum merged commit 88ab374 into numpy:main Feb 14, 2024
63 checks passed

mhvk pushed a commit to mhvk/numpy that referenced this pull request Feb 14, 2024

Merge pull request numpy#25754 from ngoldbaum/expose-dtype-api

e4f3c90

API: Expose the dtype C API

ngoldbaum mentioned this pull request Feb 14, 2024

DOC: two minor fixes for DType API doc formatting #25828

Merged

		typedef PyTypeObject PyArray_DTypeMeta;

		#endif /* Py_LIMITED_API */

		@@ -141,6 +149,7 @@ typedef struct {
		#define NPY_METH_unaligned_contiguous_loop 8
		#define NPY_METH_contiguous_indexed_loop 9

		@@ -187,6 +196,7 @@ typedef NPY_CASTING (resolve_descriptors_with_scalars_function)(
		npy_intp *view_offset);

API: Expose the dtype C API #25754

API: Expose the dtype C API #25754

Conversation

ngoldbaum commented Feb 2, 2024 • edited

mattip commented Feb 3, 2024

mattip commented Feb 3, 2024

ngoldbaum commented Feb 3, 2024 • edited

ngoldbaum commented Feb 5, 2024

ngoldbaum commented Feb 6, 2024

ngoldbaum commented Feb 6, 2024

ngoldbaum commented Feb 7, 2024

ngoldbaum Feb 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngoldbaum commented Feb 10, 2024

seberg Feb 10, 2024 • edited

Choose a reason for hiding this comment

seberg commented Feb 12, 2024

ngoldbaum commented Feb 12, 2024

seberg commented Feb 12, 2024

ngoldbaum commented Feb 13, 2024

ngoldbaum commented Feb 13, 2024

seberg commented Feb 13, 2024

ngoldbaum commented Feb 13, 2024

ngoldbaum commented Feb 13, 2024

seberg left a comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngoldbaum commented Feb 14, 2024

seberg commented Feb 14, 2024

ngoldbaum commented Feb 14, 2024

mhvk left a comment

Choose a reason for hiding this comment

ngoldbaum commented Feb 14, 2024

ngoldbaum commented Feb 14, 2024

seberg commented Feb 14, 2024

ngoldbaum commented Feb 14, 2024

mhvk commented Feb 14, 2024

mhvk commented Feb 14, 2024

ngoldbaum commented Feb 14, 2024

ngoldbaum commented Feb 2, 2024 •

edited

ngoldbaum commented Feb 3, 2024 •

edited

ngoldbaum Feb 9, 2024 •

edited

seberg Feb 10, 2024 •

edited