BUG: memoryview error with Cython 0.29.31

[tupui]

Cython 0.29.31 was release a few hours ago and it corresponds to our CI going red.

https://github.com/cython/cython/releases/tag/0.29.31

See for instance: https://github.com/scipy/scipy/runs/7551272609?check_suite_focus=true

Error compiling Cython file:
1453
------------------------------------------------------------
1454
...
1455
    return np.array(result, dtype=np.int64)
1456

1457

1458
@cython.wraparound(False)
1459
@cython.boundscheck(False)
1460
def _weightedrankedtau(ordered[:] x, ordered[:] y, intp_t[:] rank, weigher, bool additive):
1461
^
1462
------------------------------------------------------------
1463

1464
_stats.pyx:173:0: Referring to a memoryview typed argument directly in a nested closure function is not supported in Cython 0.x. Either upgrade to Cython 3, or assign the argument to a local variable and use that in the nested function.

[tupui]

@jjerphan this might interest you.

[tupui]

This is due to the changes here cython/cython#4849 and the associated issue was cython/cython#4798

[WarrenWeckesser]

Thanks @tupui.

It looks like the problem is that _weightedrankedtau contains the statement def weigh(...), and weigh uses the memoryviews y, temp, rank and perm from the _weightedrankedtau function scope. So weigh is a closure that captures memoryviews. Apparently the Cython bug is that the reference count of these memoryviews will not be incremented when the closure is called, but will be decremented when the closure is exited, which is bad. The Cython "fix" for 0.29.31 is the fix from the old Henny Youngman joke "Doctor, it hurts when I do this."

[tupui]

oh I see. Do we then just need to put this function outside?

[WarrenWeckesser]

At the moment, I'm mostly speculating, so take this with a grain of salt...

Do we then just need to put this function outside?

If my understanding of the problem is correct, then yes, I think that would be one way to fix it. Move weigh out of _weightedrankedtau, and add all the variables that it needs from within the scope of _weightedrankedtau as additional parameters.

That might be more than necessary, however. The error message says

_stats.pyx:173:0: Referring to a memoryview typed argument directly in a nested closure
function is not supported in Cython 0.x. Either upgrade to Cython 3, or assign the argument
to a local variable and use that in the nested function.

Maybe the "memoryview typed argument" only refers to y and rank, which are arguments of _weightedrankedtau that are also used in weigh(). So it might be sufficient to leave weigh where it is, and just add y and rank as parameters instead of implicitly capturing it in the closure, or perhaps, as suggested in the error message, define a local variables in weigh and assign y and rank to them (but I don't know enough about what Cython is doing to know if and why that would actually avoid the problem).

If any Cython devs/gurus happen to stop by, maybe they can suggest the simplest fix.

[scoder]

We're talking about this inner function, apparently:

scipy/scipy/stats/_stats.pyx

Line 235 in 7f941db

def weigh(intp_t offset, intp_t length):

IIUC, it uses the outer function's memoryview arguments y and rank, but not x. Due to the bug in Cython 0.29.x, this is generally unsafe. But you can just assign the arguments to local variables and use those in the inner function. Or pass the outer arguments explicitly as inner arguments, but local variables seem simpler here. (Although it would be interesting to know if avoiding a closure has a performance impact.)

Cython 3.0 has a proper fix, but that's not easy to backport. Thus the error in 0.29.31. Sorry for the hassle.

[WarrenWeckesser]

Thanks @scoder!

[jjerphan]

@tupui: Thanks. scikit-learn has the same problem. I will resolve the issue there first, and I might inspect this issue then.

[tupui]

Thanks @jjerphan. Warren already has a fix, I am just waiting for the CI to finish to get it in as it break all workflows. So we better fix this quick and if there are improvements to make later, we can still iterate 😃

[jjerphan]

The fix looks proper to me and I do not think you'll need further iterations then.

[da-woods]

FWIW it looks like the error message was probably overly strict with Cython and the issue is only with arguments to a cdef function (not a regular def function) so I'm going to submit a PR to soften it (which would let your original code work).

[xkszltl]

What would be the suggestion currently? Use 1.9 RC build (doesn't seen to work)? wait for new fixes in 1.9? Or downgrade Cython?

[tupui]

@xkszltl we are about to release 1.9 so if you can wait a few days that would be it. Otherwise you can use nightly builds we are making. But you might need to wait a bit until this PR gets into the nightly (not sure when is the rebuild happening).

[xkszltl]

Actually I tried 1.9.0rc3 locally with 0.29.31 but still hit some issues (didn't look into, seems like same or similar issues as before). Does 1.9 carry extra fixes on top?

[tupui]

@xkszltl It's normal as Cython 0.29.31 is the breaking release. If you want to use 1.9.0rc3 you need to use Cython 0.29.30. SciPy 1.9 will have the fix to work with Cython 0.29.31 as we just merged the fix (so after 1.9rc3 was made).

[scoder]

Cython 0.29.32 has been released, which removes the error again. Sorry for the inconvenience, and thanks for reporting back the issue.

[tupui]

Thank you @scoder for your help! 😃 If you don't mind there is still a discussion in #16719 that could use your insights.