-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: Incorrect signatures for np.random functions when NumPy built with Cython 0.29 #24429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@bashtage FYI |
Why doesn't mock recognize the default parameter |
@mattip Because the signature is inferred incorrectly. The parameters are inferred without a keyword default value. |
Ahh, I got it backwards. 3.0.0 works, 0.29.36 gets the signature without kwargs which is broken. |
gh-21975 was (effectively) backported to 1.26. If it creates any issues let's just pull it out of there. If Cython 3 is fine, I suspect we can keep it on main. |
Sounds like only Cython can fix, so if they aren't going to fix in 0.29.x than reversion in the branch seems like the only option. |
Cython 3 and Cython 2 both recognize the |
We are pinning Cython 3 on main, and the binding is removed on 1.26, so this can be just closed. |
Describe the issue:
When NumPy 1.26.0b1 is built with Cython 0.29,
inspect.signature
returns an inaccurate signature for Cython functions, e.g., innp.random
.With 1.26.0b1 built with Cython 0.29.36:
If Cython 3.0 is used, the signature is correct:
In particular, this breaks tests that use
mock.patch.object
to patchnp.random
functions, e.g. this no longer works:Since no signatures are better than wrong signatures, and Cython 3.0 enables the
binding
directive by default anyway (https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#binding-functions), might I suggest reverting #21975 ? The distributed NumPy wheels are built with Cython 3.0 anyway, aren't they? So they don't need thebinding
directive.Reproduce the code example:
Error message:
Runtime information:
Context for the issue:
This change breaks approximately 40 tests in our code base.
The text was updated successfully, but these errors were encountered: