Skip to content

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

Closed
hawkinsp opened this issue Aug 16, 2023 · 8 comments

Comments

@hawkinsp
Copy link
Contributor

hawkinsp commented Aug 16, 2023

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., in np.random.

With 1.26.0b1 built with Cython 0.29.36:

In [1]: import numpy as np, inspect

In [2]: inspect.signature(np.random.poisson)
Out[2]: <Signature (lam, size)>

If Cython 3.0 is used, the signature is correct:

In [1]: import numpy as np, inspect

In [2]: inspect.signature(np.random.poisson)
Out[2]: <Signature (lam=1.0, size=None)>

In particular, this breaks tests that use mock.patch.object to patch np.random functions, e.g. this no longer works:

In [1]: import numpy as np, inspect

In [2]: from unittest import mock

In [3]: m = mock.patch.object(np.random, "poisson", autospec=True).start()

In [4]: np.random.poisson(lam=0.5)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[4], line 1
----> 1 np.random.poisson(lam=0.5)

File <string>:2, in poisson(*args, **kwargs)

File ~/.pyenv/versions/3.11.0/lib/python3.11/unittest/mock.py:185, in _set_signature.<locals>.checksig(*args, **kwargs)
    184 def checksig(*args, **kwargs):
--> 185     sig.bind(*args, **kwargs)

File ~/.pyenv/versions/3.11.0/lib/python3.11/inspect.py:3204, in Signature.bind(self, *args, **kwargs)
   3199 def bind(self, /, *args, **kwargs):
   3200     """Get a BoundArguments object, that maps the passed `args`
   3201     and `kwargs` to the function's signature.  Raises `TypeError`
   3202     if the passed arguments can not be bound.
   3203     """
-> 3204     return self._bind(args, kwargs)

File ~/.pyenv/versions/3.11.0/lib/python3.11/inspect.py:3174, in Signature._bind(self, args, kwargs, partial)
   3167 except KeyError:
   3168     # We have no value for this parameter.  It's fine though,
   3169     # if it has a default value, or it is an '*args'-like
   3170     # parameter, left alone by the processing of positional
   3171     # arguments.
   3172     if (not partial and param.kind != _VAR_POSITIONAL and
   3173                                         param.default is _empty):
-> 3174         raise TypeError('missing a required argument: {arg!r}'. \
   3175                         format(arg=param_name)) from None
   3177 else:
   3178     if param.kind == _POSITIONAL_ONLY:
   3179         # This should never happen in case of a properly built
   3180         # Signature object (but let's have this check here
   3181         # to ensure correct behaviour just in case)

TypeError: missing a required argument: 'size'```

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 the binding directive.

Reproduce the code example:

import numpy as np
import inspect
inspect.signature(np.random.poisson)

Error message:

See above.

Runtime information:

In [1]: import sys, numpy; print(numpy.__version__); print(sys.version)
1.26.0b1
3.11.0 (main, Dec  1 2022, 19:33:09) [GCC 12.2.0]

In [2]: print(numpy.show_runtime())
WARNING: `threadpoolctl` not found in system! Install it by `pip install threadpoolctl`. Once installed, try `np.show_runtime` again for more detailed build information
[{'numpy_version': '1.26.0b1',
  'python': '3.11.0 (main, Dec  1 2022, 19:33:09) [GCC 12.2.0]',
  'uname': uname_result(system='Linux', node='<redacted>', release='6.3.11-redacted-amd64', version='#1 SMP PREEMPT_DYNAMIC Debian 6.3.11-redacted (2023-07-18)', machine='x86_64')},
 {'simd_extensions': {'baseline': ['SSE', 'SSE2', 'SSE3'],
                      'found': ['SSSE3',
                                'SSE41',
                                'POPCNT',
                                'SSE42',
                                'AVX',
                                'F16C',
                                'FMA3',
                                'AVX2'],
                      'not_found': ['AVX512F',
                                    'AVX512CD',
                                    'AVX512_KNL',
                                    'AVX512_KNM',
                                    'AVX512_SKX',
                                    'AVX512_CLX',
                                    'AVX512_CNL',
                                    'AVX512_ICL',
                                    'AVX512_SPR']}}]
None

Context for the issue:

This change breaks approximately 40 tests in our code base.

@hawkinsp
Copy link
Contributor Author

@bashtage FYI

@mattip
Copy link
Member

mattip commented Aug 16, 2023

Why doesn't mock recognize the default parameter None for size?

@charris charris added this to the 1.26.0 release milestone Aug 16, 2023
@hawkinsp
Copy link
Contributor Author

hawkinsp commented Aug 16, 2023

@mattip Because the signature is inferred incorrectly. The parameters are inferred without a keyword default value.

@mattip
Copy link
Member

mattip commented Aug 16, 2023

Ahh, I got it backwards. 3.0.0 works, 0.29.36 gets the signature without kwargs which is broken.

@seberg
Copy link
Member

seberg commented Aug 17, 2023

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.

@bashtage
Copy link
Contributor

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.

@charris
Copy link
Member

charris commented Sep 1, 2023

Cython 3 and Cython 2 both recognize the binding directive, but have different defaults. Because the 1.x.x series defaulted to "False" before Cython 3, we will make that explicit for the 1.26.x release as well.

@seberg
Copy link
Member

seberg commented Jan 31, 2024

We are pinning Cython 3 on main, and the binding is removed on 1.26, so this can be just closed.

@seberg seberg closed this as completed Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants