MNT: refactor casting inner loop return value handling, small cleanups #29452

mattip · 2025-07-28T03:32:42Z

In order to better handle a non-zero return value from casting inner-loop functions, define a Py_CheckRetAndFPEAfterLoop function (suggestions for a better name are welcome) that handles both the return value and any FPE error handling.

Along the way, clean up some use of old NPY_ERROR/NPY_SUCCESS (which has odd values: NPY_ERROR is 0, NPY_SUCCESS is 1) and refactor gotos in the huge array_subscript function.

Once this goes in, I can use Py_CheckRetAndFPEAfterLoop to formalize the return value for same_value failures in casting in #29129.

ngoldbaum

Seems like a good idea to factor this out into a re-usable function. Just a small driveby bit of bikeshedding.

ngoldbaum · 2025-07-28T16:54:43Z

numpy/_core/src/multiarray/dtype_transfer.c

+ */
+NPY_NO_EXPORT int
+Py_CheckRetAndFPEAfterLoop(const char * name, int ret, NPY_ARRAYMETHOD_FLAGS flags)


Let's give it a PyArray prefix so we're not stepping on the python.h namespace. Also it's kinda an awkward name but I can't think of a better one.

@ngoldbaum can you comment briefly that you like the idea of introducing this? Because I think if we add it, it mainly means that we add these special/magic return values as public API, which is OK, but something to keep in mind.
(If users want a special return, one could imagine extending the capability with context there and customizing this function.)

We have functions prefixed PyArrayMethod_ also, I think that fits better, or maybe PyUFunc_ just because the current FPE check is there (and public).
Otherwise, the "ret" refers to nothing?

Because I think if we add it, it mainly means that we add these special/magic return values as public API, which is OK, but something to keep in mind.
(If users want a special return, one could imagine extending the capability with context there and customizing this function.)

Maybe making this extensible and using a context is better? I agree PyArrayMethod_ or PyUfunc_ are both probably better namespaces for this. The former if we want to make this re-usable by other people.

I think you probably have a better handle on whether it makes sense to make this general with a context. If it does we probably also need an API to set up and clean up the context like the other PyArrayMethod mechanisms?

Maybe making this extensible and using a context is better?

I am not quite sure what you refer to. There is the context passed to the ufunc loop. For now that isn't needed, though. The idea here (this doesn't yet!) is to introduce special return values basically as a list of "build in Python errors supported on ufuncs".

I don't mind that, but wanted to loop you in. The refactor itself is great, if context means extending the signature, I am honestly happy to defer that, as it only matters when/if we make it public.

Then I agree this is fine as-is. Maybe the docstring needs a comment that if it ever gets made public we need to decide how to handle making ret public (an enum? a new object?)

Also let's go with PyArrayMethod_ sorry for all the bikeshedding!

decide how to handle making ret public

Just to note: ret (i.e. the special values) are effectively public right away even without the function itself being public.
Since ret is the result from any ArrayMethod (ufunc or cast)!

But yeah, maybe we should have an enum when we start using it (which is effectively right away, since Matti wants to use it for the casting PR).

Oh I see what you're saying. Sorry, it took a while to get the point across 🙃

Yes, we should definitely document the special return values and what they mean as soon as the special return values lead to special behavior in NumPy. If that's already the case then we should have been documenting that!

We should also probably try to avoid using obvious return values that existing functions are likely to already use. Maybe all the special return values should be bigger than 0xFF to make sure they're "far away" from common values?

I think nobody should use anything that isn't -1, but yeah, we probably might as well start at some larger value when adding them. All return values also must be negative actually (not strictly, but they will be, since I think only negative ones will actually break out of the loop).

If I understand correctly, the bottom line is that there is no action needed for this PR, but when we start to use a return value it should:

be a well-documented ENUM

be less than -0xff

mattip · 2025-07-29T01:18:16Z

CI is passing

seberg

Looks fine, I would like Nathan to confirm he likes to make the special return values public.

The actual error handling needs some tiny adjustments (and I think the longer prefix is probably a better name).

numpy/_core/src/multiarray/multiarraymodule.c

numpy/_core/tests/test_custom_dtypes.py

seberg · 2025-07-30T07:34:06Z

numpy/_core/src/umath/ufunc_object.c

@@ -984,6 +984,10 @@ try_trivial_single_output_loop(PyArrayMethod_Context *context,
        res = -1;
    }

+    /*
+     * Do not use PyArray_CheckRetAndFPE since we don't want to look up the
+     * name if no error


I suppose that may have been a thought here, but I wonder if that is worth it, the function is just return ufunc->name ? ufunc->name : "<unnamed ufunc>";?

seberg · 2025-07-30T07:38:50Z

numpy/_core/src/umath/_scaled_float_dtype.c

@@ -454,7 +454,7 @@ sfloat_init_casts(void)
        .nout = 1,
        /* minimal guaranteed casting */
        .casting = NPY_SAME_KIND_CASTING,
-        .flags = NPY_METH_SUPPORTS_UNALIGNED,
+        .flags = NPY_METH_SUPPORTS_UNALIGNED | NPY_METH_REQUIRES_PYAPI,


Suggested change

.flags = NPY_METH_SUPPORTS_UNALIGNED | NPY_METH_REQUIRES_PYAPI,

.flags = NPY_METH_SUPPORTS_UNALIGNED,

If this change is necessary, that is a bug in the error check function, and it is almost surprising more isn't failing (probably defensive PyErr_Occurred() checks due to legacy paths).

It is a bug in the scaled_float_dtype. It uses the C-API to set a PyErr, but does not declare (here) that it uses the C-API. This can lead to other bugs since the inner loop will be called without the GIL.

No it's not a bug, the loop uses:

npy_gil_error(PyExc_TypeError, "error raised inside the core-loop: non-finite factor!");

to set the error which it is allowed to do and which is a common pattern. The only reason you don't see this pattern +failing a lot more, is that ufuncs have an additional PyErr_Occurred() check that always happens (due to old loops not indicating an error).

seberg · 2025-07-30T07:43:30Z

numpy/_core/src/multiarray/mapping.c

-            if (fpes && PyUFunc_GiveFloatingpointErrors("cast", fpes) < 0) {
-                return -1;
+            if (res ==0) {
+                res = -101;


Looks mangled up, probably just a typo. I suppose -1 as error already set makes this OK, although I wonder if we shouldn't just change the order or skip the check if the deallocate fails (even if I guess that might give the less relevant error).

I guess the best thing to do if the deallocate fails is to return -1 immediately, overriding any potential existing PyErr. But what does that mean in terms of calling into python with potentially a PyErr set?

You can call PyErr_Clear to clear the error indicator if you're sure that's correct and it's set. Might need to wrap in an if (PyErr_Occurred()) block before calling it.

Yeah, this change is fine. NpyIter_Deallocate() can be (and is commonly) called with an error in flight since it is a deallocation function.
One could argue about the non NpyIter_Deallocate() error being the first one probably (and thus the more interesting one), but it doesn't really matter much -- may also be that it actually chains the error.

seberg · 2025-07-30T07:45:02Z

numpy/_core/src/multiarray/dtype_transfer.c

+{
+    int res = 0;
+    if (flags & NPY_METH_REQUIRES_PYAPI && PyErr_Occurred()) {


As mentioned elsewhere this logic looks wrong. It should probably just assume -1 means an error is already set.

seberg · 2025-07-30T07:46:39Z

numpy/_core/src/umath/ufunc_object.c

@@ -984,6 +984,10 @@ try_trivial_single_output_loop(PyArrayMethod_Context *context,
        res = -1;
    }


Here is the "doesn't correctly return -1 because it's an old style loop" logic. I.e. it is dealt with explicitly before the function is called. (It is also unrelated to the "needs API" flag since it is valid and typical to grab the GIL to set an error.)

seberg · 2025-07-30T16:54:32Z

(I'll note that there are still a few places where this is missing, e.g. in the iterator that calls the cast functions, I think. I can be convinced to overlook it, but if we have special return values we have to make sure to catch them everywhere.)

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

mattip · 2025-07-31T04:19:45Z

I redid the logic.

Loops can set a PyErr even if they do not have the NPY_METH_REQUIRES_PYAPI flag, as long as they return -1. In time we could tighten that and prefer a clear separation of pure C loops from C-API code, using the return value to signify there was an error. I think this is preferable, and is the motivation for this PR.
There is a question around how to handle NpyIter_Deallocate. On one hand, it should always be called to avoid a resource leak. But it may not be safe to call NpyIter_Deallocate with PyErr set. For now I did not call NpyIter_Deallocate() if there is an error, pending review.

mattip · 2025-07-31T04:20:49Z

I'll note that there are still a few places where this is missing, e.g. in the iterator that calls the cast functions

I thought I covered them all. Could you point these out?

seberg · 2025-07-31T04:49:17Z

Arg... the reason is that iternext doesn't check FPEs, but it does do casts that can break the loop with an error. Dunno what's best, checking FPEs there is more often than we want to probably, so there we actually need a non-FPE checking version.

mattip · 2025-07-31T06:31:51Z

Hmm. This is more complicated than I first thought. Maybe I should go with the convention that an inner loop will only return -1 if it has set a PyErr, and not try to return specific enum`ed errors in #29129

seberg · 2025-07-31T07:47:40Z

Maybe I should go with the convention that an inner loop will only return -1

Yes please, I do like this, but unfortunately I really think if we do it we have to introduce it everywhere, which isn't quite trivial.

MNT: refactor casting inner loop return value handling, small cleanups

472733c

github-actions bot added the 03 - Maintenance label Jul 28, 2025

fix errors

1d52804

ngoldbaum reviewed Jul 28, 2025

View reviewed changes

change name, return early if PyErr detected

0a79638

seberg reviewed Jul 30, 2025

View reviewed changes

mattip and others added 3 commits July 31, 2025 02:25

Update numpy/_core/tests/test_custom_dtypes.py

7cf3ec9

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

Update numpy/_core/src/multiarray/multiarraymodule.c

1a5ba5b

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

fixes and changes from review

8b4103e

mattip closed this Jul 31, 2025

	.flags = NPY_METH_SUPPORTS_UNALIGNED \| NPY_METH_REQUIRES_PYAPI,
	.flags = NPY_METH_SUPPORTS_UNALIGNED,

		@@ -984,6 +984,10 @@ try_trivial_single_output_loop(PyArrayMethod_Context *context,
		res = -1;
		}

Uh oh!

MNT: refactor casting inner loop return value handling, small cleanups #29452

MNT: refactor casting inner loop return value handling, small cleanups #29452

Uh oh!

Conversation

mattip commented Jul 28, 2025

Uh oh!

ngoldbaum 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum Jul 30, 2025 • 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!

ngoldbaum Jul 30, 2025 • 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!

mattip commented Jul 29, 2025

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

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!

seberg Jul 31, 2025 • 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!

seberg commented Jul 30, 2025

Uh oh!

mattip commented Jul 31, 2025

Uh oh!

mattip commented Jul 31, 2025

Uh oh!

seberg commented Jul 31, 2025

Uh oh!

mattip commented Jul 31, 2025

Uh oh!

seberg commented Jul 31, 2025

Uh oh!

Uh oh!

ngoldbaum Jul 30, 2025 •

edited

Loading

ngoldbaum Jul 30, 2025 •

edited

Loading

seberg Jul 31, 2025 •

edited

Loading