BUG: flaky test under pytest-run-parallel

[ngoldbaum]

==================================== ERRORS ====================================
___________ ERROR at call of test_real_imag_ufunc_minimal[imag-imag] ___________

ufunc = <ufunc 'imag'>, attr = 'imag'

    @pytest.mark.parametrize("ufunc,attr", [
        (np._core.umath.real, "real"), (np._core.umath.imag, "imag")])
    def test_real_imag_ufunc_minimal(ufunc, attr):
        with pytest.raises(TypeError):
            ufunc(np.array([1, 2, 3]))  # non-complex or object raises
    
        arr = np.array([1 + 2j, 3 + 4j])
>       res = ufunc(arr)
              ^^^^^^^^^^
E       RecursionError: Stack overflow (used 16364 kB) in __subclasscheck__

arr        = array([1.+2.j, 3.+4.j])
attr       = 'imag'
ufunc      = <ufunc 'imag'>

As seen here: https://github.com/numpy/numpy/actions/runs/23796435549/job/69344855092?pr=31108#step:9:47471

Almost certainly fallout from merging #30984. I'm going to try to reproduce this locally and understand what's happening.

Ping @seberg @kumaraditya303

[ngoldbaum]

I can reproduce this on my Mac if I do this:

$ while spin test --no-build "numpy/_core/tests/test_arrayobject.py::test_real_imag_ufunc_minimal" -- --parallel-threads=100 -sv; do; done

It crashes after about 10 loops. It needs to be a "fresh" interpreter (using pytest-run-parallel's --forever option doesn't trigger the issue) so this has something to do with cache initialization, which I guess makes sense.

[ngoldbaum]

@seberg while we work on the issues raised in #31083, would you be OK with reverting #30984? We could xfail it as well but since the failure isn't deterministic, it would be the sort of xfail we might forget to remove afterwards. I'm not comfortable with this sort of crash being possible now. It's also not great that CI is hitting it regularly.

[kumaraditya303]

I think the issue is that PyUFunc_AddLoop is called dynamically and there is no serialization or locking so multiple threads could end up adding multiple loops or overwrite loops added by other threads.

[ngoldbaum]

FWIW here's the first few frames of the stack trace on the crashing thread, everything after is the same two frames over and over:

(lldb) bt 100
* thread #4, name = 'Thread-46003 (closure)', stop reason = breakpoint 1.1
  * frame #0: 0x0000000100ae0430 libpython3.14t.dylib`_Py_CheckRecursiveCall(tstate=0x000000012600f800, where=" in comparison") at ceval.c:619:21
    frame #1: 0x00000001009c97b8 libpython3.14t.dylib`_Py_EnterRecursiveCallTstate(tstate=0x000000012600f800, where=" in comparison") at pycore_ceval.h:224:41
    frame #2: 0x00000001009ca3ec libpython3.14t.dylib`PyObject_RichCompare(v=0x0000000102219a18, w=0x0000043b90467800, op=2) at object.c:1106:9
    frame #3: 0x00000001009ca2f4 libpython3.14t.dylib`PyObject_RichCompareBool(v=0x0000000102219a18, w=0x0000043b90467800, op=2) at object.c:1131:11
    frame #4: 0x00000001009ff230 libpython3.14t.dylib`tuple_richcompare(v=0x0000043b92175280, w=0x0000000c0c1d1b00, op=2) at tupleobject.c:687:17
    frame #5: 0x00000001009ca590 libpython3.14t.dylib`do_richcompare(tstate=0x000000012600f800, v=0x0000043b92175280, w=0x0000000c0c1d1b00, op=2) at object.c:1060:15
    frame #6: 0x00000001009ca410 libpython3.14t.dylib`PyObject_RichCompare(v=0x0000043b92175280, w=0x0000000c0c1d1b00, op=2) at object.c:1109:21
    frame #7: 0x00000001009ca2f4 libpython3.14t.dylib`PyObject_RichCompareBool(v=0x0000043b92175280, w=0x0000000c0c1d1b00, op=2) at object.c:1131:11
    frame #8: 0x0000000102083d84 _multiarray_umath.cpython-314t-darwin.so`PyUFunc_AddLoop(ufunc=0x0000043b92100f00, info=0x0000000c0c1d1b40, ignore_duplicate=1) at dispatching.cpp:130:19 [opt]
    frame #9: 0x00000001020a5538 _multiarray_umath.cpython-314t-darwin.so`int real_imag_promoter<&NPY_DType_Slots::imag_meth>(ufunc=0x0000043b92100f00, op_dtypes=<unavailable>, signature=<unavailable>, new_op_dtypes=0x0000000171e244e0) at real_imag_ufuncs.cpp:220:15 [opt]
    frame #10: 0x00000001020850b0 _multiarray_umath.cpython-314t-darwin.so`call_promoter_and_recurse(ufunc=0x0000043b92100f00, info=0x0000043b921752c0, op_dtypes=0x0000000171e249b0, signature=0x0000000172e0f0e0, operands=0x0000000172e0f0f0) at dispatching.cpp:616:31 [opt]
    frame #11: 0x0000000102085088 _multiarray_umath.cpython-314t-darwin.so`promote_and_get_info_and_ufuncimpl(ufunc=<unavailable>, ops=<unavailable>, signature=<unavailable>, op_dtypes=<unavailable>, legacy_promotion_is_possible=<unavailable>) at dispatching.cpp:924:16 [opt]
    frame #12: 0x0000000102085448 _multiarray_umath.cpython-314t-darwin.so`call_promoter_and_recurse(ufunc=0x0000043b92100f00, info=<unavailable>, op_dtypes=0x0000000171e24e80, signature=0x0000000172e0f0e0, operands=0x0000000172e0f0f0) at dispatching.cpp:660:21 [opt] [inlined]
    frame #13: 0x00000001020853fc _multiarray_umath.cpython-314t-darwin.so`promote_and_get_info_and_ufuncimpl(ufunc=<unavailable>, ops=<unavailable>, signature=<unavailable>, op_dtypes=<unavailable>, legacy_promotion_is_possible=<unavailable>) at dispatching.cpp:924:16 [opt]
    frame #14: 0x0000000102085448 _multiarray_umath.cpython-314t-darwin.so`call_promoter_and_recurse(ufunc=0x0000043b92100f00, info=<unavailable>, op_dtypes=0x0000000171e25350, signature=0x0000000172e0f0e0, operands=0x0000000172e0f0f0) at dispatching.cpp:660:21 [opt] [inlined]
    frame #15: 0x00000001020853fc _multiarray_umath.cpython-314t-darwin.so`promote_and_get_info_and_ufuncimpl(ufunc=<unavailable>, ops=<unavailable>, signature=<unavailable>, op_dtypes=<unavailable>, legacy_promotion_is_possible=<unavailable>) at dispatching.cpp:924:16 [opt]
    frame #16: 0x0000000102085448 _multiarray_umath.cpython-314t-darwin.so`call_promoter_and_recurse(ufunc=0x0000043b92100f00, info=<unavailable>, op_dtypes=0x0000000171e25820, signature=0x0000000172e0f0e0, operands=0x0000000172e0f0f0) at dispatching.cpp:660:21 [opt] [inlined]
    frame #17: 0x00000001020853fc _multiarray_umath.cpython-314t-darwin.so`promote_and_get_info_and_ufuncimpl(ufunc=<unavailable>, ops=<unavailable>, signature=<unavailable>, op_dtypes=<unavailable>, legacy_promotion_is_possible=<unavailable>) at dispatching.cpp:924:16 [opt]

[seberg]

Hmmmm, yeah, I guess the loop adding needs a lock in either case maybe? I would prefer not reverting and just adding that lock.
I do care quite a lot about fixing that feature. Now we could remove the ufunc part of it (i.e. the object fix), but that would be a hell of a lot harder than adding the right locking.

EDIT: To be clear, this adds the loop dynamically during promotion. And I think just locking everything during promotion may make sense.
But I expect the issue here is unrelated to that (unless there is a bug in the cache), but rather that adding the loop itself isn't fully thread safe.

[kumaraditya303]

Hmmmm, yeah, I guess the loop adding needs a lock in either case maybe? I would prefer not reverting and just adding that lock.
I do care quite a lot about fixing that feature. Now we could remove the ufunc part of it (i.e. the object fix), but that would be a hell of a lot harder than adding the right locking.

The code in question calls a bunch of CPython C-APIs and I think it is re-entrant so you cannot use a simple mutex, you would have to use a critical section on either an object or on a per-ufunc PyMutex.

[seberg]

I don't think adding a loop should be re-entrant (or at least only in pretty theoretical ways) or am I missing something? Promotion could be more reasonably re-entrant (but IIRC that used to ignore previously, and I can see ignoring it. Except for promotion triggering adding a loop, I guess -- which might be re-entrant if it used the same lock.).

A per-ufunc mutex isn't problematic as such, though? We used to have a per-ufunc shared-mutex until very recently even a re-entrant mutex is probably not a big issue if that is required?
(Critical sections safety to me is very unclear unless its content uses no Python API at all? -- it feels a bit like "maybe as safe as the GIL". But the GIL is at best in practice safe enough in cases such as here?)

(In the end I assume we can say that contention on the mutex is meaningless, because the cache lookup fast-path is the only relevant thing.)

[ngoldbaum]

@seberg is there any chance you'd be available next Wednesday at 4:30 PM UTC for a video call to talk about this? That time works best for Kumar, the NumPy community meetings are too late for him.

Thursday and Friday at the same time also work.

[seberg]

Yes, we can do that. I suppose I thought this wouldn't be hard (at least not the first part of fixing that one test).

[ngoldbaum]

I just sent out an invite.

I suppose I thought this wouldn't be hard (at least not the first part of fixing that one test).

It's still not clear to me or Kumar what the requirements are. I also personally am a little uncomfortable about how it feels like there's an expectation that Kumar or I need to be the ones contributing a fix for the flaky test. I still think it makes sense to revert #30984 so this isn't a fire drill to fix before 2.5.0 get released and we can come up with a good design for handling these issues.

Basically, I want to have an opportunity for us to clear up misunderstandings face-to-face.

[seberg]

All this needs is a mutex on the ufunc during loop registration, something that technically we seem to need anyway.
That seems just as easy as reverting, and yeah, I care about that PR. Sure, I thought Kumar or you might jump at it, but I can just add that one mutex (may per ufunc), I was simply mostly out the past week.

(Yes, there is the thing about future need for cache invalidation related gh-30593 and locking for public API, which does need some discussion probably).

[ngoldbaum]

I'm not sure we can "just add a mutex" here and keep the thread-safe but lock-free fast path. That's part of what I wanted to talk about.