BUG: fix a number of issues around iterators and StringDType by ngoldbaum · Pull Request #32156 · numpy/numpy

ngoldbaum · 2026-07-30T22:13:13Z

PR summary

Fixes #32153. Also fixes a number of other issues that are caused by the same problem: array creation can replace descriptors. The iterator code in particular had a number of spots that didn't handle this case.

I added a new NPY_DT_has_finalize predicate to branch based on whether or not there is a finalize_descr implementation, which might replace dtype instances.

I also added a new npy_resync_finalized_descr, which does the necessary reference bookkeeping when a descriptor has been replaced.

Also deletes an assert in the np.concatenate implementation that could trigger for StringDType if a descriptor is replaced.

There are also a few minor unrelated C error checking fixes included as well.

AI Disclosure

An AI model found the issue and I used it to help develop and debug the fix and new tests.

ngoldbaum · 2026-08-05T19:42:40Z

@ikrommyd I requested for you to give this a look but feel free to ping me if you're unsure about details and want me to walk you through this

ngoldbaum · 2026-08-06T20:27:22Z

@MaanasArora any chance you can give this one a look? Thanks!!

ikrommyd

Looks overall good to me, left a few comments though

ikrommyd · 2026-08-07T08:31:11Z

                                                   NPY_OP_ITFLAG_READ) &&
-                          PyArray_NDIM(op[iop]) == 0) {
+                          PyArray_NDIM(op[iop]) == 0 &&
+                          !NPY_DT_has_finalize(NPY_DTYPE(op_dtype[iop]))) {


Why doesn't this branch resync as well? Reproducer by claude below:

import numpy as np s = np.array("Z" * 30, dtype="T") dt = np.dtypes.StringDType() np.empty(1, dtype=dt) # attach dt, so a temp created from it gets a replaced descriptor it = np.nditer([s], flags=["refs_ok"], op_dtypes=[dt], casting="unsafe") str(next(it))

raises TypeError: Iterator operand required copying or buffering, but neither copying nor buffering was enabled. Wouldn't resyncing fix that?

Yes it should. After adding resyncing here I also realized that the fancy-index assignment was also subtly broken, see the change at line 2148 of array_assign_subscript.

ikrommyd · 2026-08-07T08:33:38Z

+    /*
+     * descrs[0] and descrs[2] both describe op[0], which is used as both
+     * the previous-value input and the output (see `dataptr_copy` below).
+     */
+    npy_resync_finalized_descr(&descrs[0], op[0]);
+    npy_resync_finalized_descr(&descrs[1], op[1]);
+    npy_resync_finalized_descr(&descrs[2], op[0]);
+


This is in PyUFunc_Accumulate. Does PyUFunc_Reduce (probably inside PyUFunc_ReduceWrapper) or others like reduceat etc need the same treatment?

I added some code to re-sync the ArrayMethod contexts in PyUFunc_ReduceWrapper. It can't use this pattern of the new NpyIter_GetArrayMethodContext I also added because reductions alias descriptors. However I could use NpyIter_GetArrayMethodContext in PyUFunc_GeneralizedFunctionInternal and execute_ufunc_loop where it nicely simplifies things.

ikrommyd · 2026-08-07T08:39:51Z

I did not look at this unseen code in the diff but fable mentioned that these two should probably be reversed such that NpyIter_Deallocate should see that PyErr_Occurred since it branches on it in line 657 now.

Missed this comment on my first pass but I just pushed a fix in the last commit.

ikrommyd · 2026-08-07T08:41:43Z

+    assert_array_equal(out, a)
+
+
+def test_nditer_copy_and_writeback():


it.copy() is not called tested in this test

See test_nditer_distinct_allocators, unless you were thinking of something different

ikrommyd · 2026-08-07T08:42:03Z

The ufunc.accumulate changes are untested.

see test_reduce_distinct_allocators

ngoldbaum · 2026-08-07T19:48:53Z

@mhvk you may be interested in this too. The bulk of the PR is tests and the C changs are all in the guts of the iterator internals. If we get one more review on this I think we can merge this in time for the bugfix release this weekend.

mhvk

Yikes, this really gets into the weeds. Two possible relevant queries, two suggestions that can be ignored...

mhvk · 2026-08-07T21:12:19Z

        if (PyArray_GetDTypeTransferFunction(
                1, itemsize, itemsize,
-                descr, PyArray_DESCR(self),
+                NpyIter_GetDescrArray(mit->outer)[mit->num_fancy],


Can this never happen in the subspace_iter above?

FWIW, seems that the subspace path in PyArray_MapIterNew replaces descr (in this case PyArray_DESCR(self)), so hopefully not!

numpy/numpy/_core/src/multiarray/mapping.c

Lines 3048 to 3057 in ba165bb

extra_op = (PyArrayObject *)PyArray_NewFromDescr(&PyArray_Type,

extra_op_dtype,

mit->nd_fancy + PyArray_NDIM(subspace),

mit->dimensions, strides,

NULL, 0, NULL);

if (extra_op == NULL) {

goto fail;

}

// extra_op_dtype might have been replaced, so get a new reference

extra_op_dtype = PyArray_DESCR(extra_op);

Yeah, subspace_iter is unbuffered so the descriptors match. I added StringDType tests for this though.

mhvk · 2026-08-07T21:18:54Z


+    /*
+     * op_dtypes tracks the loop's descriptors so the initial-value buffers
+     * use the same ones.  The context must not outlive `iter`.


Maybe "...not outlive iter, as we are filled with borrowed references" (maybe, "like for PyIter_GetArrayMethodContext")
Also perhap: "Reminder: inputs have nout inputs that hold results
and one input that provides the values, i.e., nin = nout+1"

mhvk · 2026-08-07T21:37:33Z

        }
    }

+    /*


Can anything happen to descrs outside of the iterator block? And even then, what can happen to 0 and 1? If not, I'd prefer moving it there (and have assert here if you wish). But I may be missing something.

You're right, descrs was used for two different purposes. I gave the loiop a separate borrowed descriptor array built from the final operands, which resolves the need to resync here.

ikrommyd

All good from my side, I don't see anything else. There's just the remaining comments from Marten left.
Some conflicts may come from this PR to my reduction loops in reduceat and accumulate PRs but shouldn't be anything particularly annoying.

MaanasArora · 2026-08-08T02:00:54Z

Sorry, could only get through a brief pass, this gets very deep! But looks good to me.

ngoldbaum · 2026-08-08T02:36:28Z

Thanks all! Marten pointed out something that let me simplify this even more.

mhvk

Looks all OK now!

ngoldbaum · 2026-08-08T15:14:34Z

@charris just in case there's still time for one more backport, I'm merging this

charris · 2026-08-08T15:33:42Z

Looking at the backport, the remaining diff of descriptor.c with main is:

diff --git a/numpy/_core/src/multiarray/descriptor.c b/numpy/_core/src/multiarray/descriptor.c
index ddb61a5912..7e82a702a7 100644
--- a/numpy/_core/src/multiarray/descriptor.c
+++ b/numpy/_core/src/multiarray/descriptor.c
@@ -526,8 +526,9 @@ _convert_from_array_descr(PyObject *obj, int align)
                     "Field elements must be tuples with at most 3 elements, got '%R'", item);
             goto fail;
         }
-        if (_reject_unsupported_field_dtype(conv) < 0) {
-            Py_DECREF(conv);
+        if (PyObject_IsInstance((PyObject *)conv, (PyObject *)&PyArray_StringDType)) {
+            PyErr_Format(PyExc_TypeError,
+                         "StringDType is not currently supported for structured dtype fields.");
             goto fail;
         }
         if ((PyDict_GetItemWithError(fields, name) != NULL) // noqa: borrowed-ref OK
@@ -670,10 +671,6 @@ _convert_from_list(PyObject *obj, int align)
         if (conv == NULL) {
             goto fail;
         }
-        if (_reject_unsupported_field_dtype(conv) < 0) {
-            Py_DECREF(conv);
-            goto fail;
-        }
         dtypeflags |= (conv->flags & NPY_FROM_FIELDS);
         if (align) {
             int _align = conv->alignment;
@@ -1167,12 +1164,6 @@ _convert_from_dict(PyObject *obj, int align)
             Py_DECREF(ind);
             goto fail;
         }
-        if (_reject_unsupported_field_dtype(newdescr) < 0) {
-            Py_DECREF(newdescr);
-            Py_DECREF(tup);
-            Py_DECREF(ind);
-            goto fail;
-        }
         PyTuple_SET_ITEM(tup, 0, (PyObject *)newdescr);
         int _align = 1;
         if (align) {
@@ -1638,9 +1629,7 @@ _convert_from_type(PyObject *obj) {
     else {
         PyObject *DType = PyArray_DiscoverDTypeFromScalarType(typ);
         if (DType != NULL) {
-            PyArray_Descr *ret = PyArray_GetDefaultDescr((PyArray_DTypeMeta *)DType);
-            Py_DECREF(DType);
-            return ret;
+            return PyArray_GetDefaultDescr((PyArray_DTypeMeta *)DType);
         }
         PyArray_Descr *ret = _try_convert_from_dtype_attr(obj);
         if ((PyObject *)ret != Py_NotImplemented) {

Note that _reject_unsupported_field_dtype is now defined, but unused in the 2.5.x branch. I can either remove it, or bring in the rest of descriptor.c from main.

charris · 2026-08-08T15:36:52Z

I am tempted to bring down the whole descriptor.c from main, the additional changes seem largely independent and unlikely to cause trouble.

ngoldbaum · 2026-08-08T15:46:39Z

@charris I'd drop the descriptor.c changes from the backport and remove _reject_unsupported_field_dtype. Those are only there to support #32027, which probably shouldn't be backported. Although that said, I could see a case for backporting #32027 too since it fixes possible crashes. It also makes things that happened to work no longer work, so I erred on the side of not marking it for backport.

charris · 2026-08-08T15:57:48Z

OK, I'll leave descriptor.c unchanged as everything there looks #32027 related.

charris · 2026-08-08T16:01:41Z

There is one leak fix, however, I'll put that back in.

charris · 2026-08-08T16:39:43Z

There are also problems with PyUFunc_ReduceWrapper, whose signature has also changed in main so I don't trust the changes in the backport. I think the best option is to not backport this, main has diverged too much from 2.5.x.

ngoldbaum · 2026-08-08T16:46:55Z

Fair enough

ikrommyd · 2026-08-09T11:06:19Z

I am responsible for the changes in PyUFunc_ReduceWrapper in main. They should all be due to the reduction loops introduction. I could backport this PR safely if you want for the next 2.5.x release. When it comes to reductions, all Nathan's code needs is just to be collapsed down to 1-output reductions instead of N-output reductions. The signature change shouldn't affect anything as the change was from PyArrayObject -> PyObject just to cover the tuple of arrays the N-output reductions return.

…2156) Signed-off-by: Iason Krommydas <[email protected]> Co-authored-by: Nathan Goldbaum <[email protected]>

…#32304)

…2156)

BUG: fix a number of issues around iterators and StringDType

968ba96

ngoldbaum added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported component: numpy.strings String dtypes and functions labels Jul 30, 2026

ngoldbaum requested a review from ikrommyd August 5, 2026 19:42

ikrommyd reviewed Aug 7, 2026

View reviewed changes

ngoldbaum added 2 commits August 7, 2026 13:26

MAINT: simplify, add more tests, respond to review comments

2536c02

BUG: skip writeback in OOM path to match other error paths

ba165bb

ngoldbaum requested a review from ikrommyd August 7, 2026 19:48

mhvk reviewed Aug 7, 2026

View reviewed changes

ikrommyd approved these changes Aug 8, 2026

View reviewed changes

ngoldbaum added 2 commits August 7, 2026 20:31

MAINT: simplify and test subarrays too

e706b76

Merge branch 'main' into fix-finalize-descr-syncing

d0d9005

mhvk approved these changes Aug 8, 2026

View reviewed changes

ngoldbaum merged commit 87310db into numpy:main Aug 8, 2026
91 checks passed

charris mentioned this pull request Aug 8, 2026

BUG: fix a number of issues around iterators and StringDType (#32156) #32225

Closed

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 8, 2026

ikrommyd mentioned this pull request Aug 15, 2026

BUG: fix a number of issues around iterators and StringDType (#32156) #32304

Merged

ikrommyd added a commit to ikrommyd/numpy that referenced this pull request Aug 16, 2026

BUG: fix a number of issues around iterators and StringDType (numpy#3…

f8271f8

…2156) Signed-off-by: Iason Krommydas <[email protected]> Co-authored-by: Nathan Goldbaum <[email protected]>

ngoldbaum pushed a commit that referenced this pull request Aug 17, 2026

BUG: fix a number of issues around iterators and StringDType (#32156) (…

0409d19

…#32304)

ngoldbaum added a commit to ngoldbaum/numpy that referenced this pull request Sep 8, 2026

BUG: fix a number of issues around iterators and StringDType (numpy#3…

3d3f2ed

…2156)

SwayamInSync mentioned this pull request Nov 3, 2026

[Self Review] ENH: use output-only promoters only when their output DType is requested SwayamInSync/numpy#5

Open

		assert_array_equal(out, a)


		def test_nditer_copy_and_writeback():

	extra_op = (PyArrayObject *)PyArray_NewFromDescr(&PyArray_Type,
	extra_op_dtype,
	mit->nd_fancy + PyArray_NDIM(subspace),
	mit->dimensions, strides,
	NULL, 0, NULL);
	if (extra_op == NULL) {
	goto fail;
	}
	// extra_op_dtype might have been replaced, so get a new reference
	extra_op_dtype = PyArray_DESCR(extra_op);

                       }
                   }
+                  /*

Conversation

ngoldbaum commented Jul 30, 2026

PR summary

AI Disclosure

Uh oh!

ngoldbaum commented Aug 5, 2026

Uh oh!

ngoldbaum commented Aug 6, 2026

Uh oh!

ikrommyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikrommyd Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Aug 7, 2026

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaanasArora Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikrommyd left a comment

Choose a reason for hiding this comment

Uh oh!

MaanasArora commented Aug 8, 2026

Uh oh!

ngoldbaum commented Aug 8, 2026

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Aug 8, 2026

Uh oh!

Uh oh!

charris commented Aug 8, 2026

Uh oh!

charris commented Aug 8, 2026

Uh oh!

ngoldbaum commented Aug 8, 2026

Uh oh!

charris commented Aug 8, 2026

Uh oh!

charris commented Aug 8, 2026

Uh oh!

charris commented Aug 8, 2026

ikrommyd Aug 7, 2026 •

edited

Loading

MaanasArora Aug 8, 2026 •

edited

Loading

ikrommyd commented Aug 9, 2026 •

edited

Loading