Skip to content

BUG: np.size is no longer an instance of FunctionType (nightly/dev build) #23307

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
djhoese opened this issue Mar 1, 2023 · 8 comments
Closed
Labels

Comments

@djhoese
Copy link
Contributor

djhoese commented Mar 1, 2023

Describe the issue:

I have a package that has a CI environment that installs numpy and many other packages from their current unstable/dev versions (nightly builds) and runs the tests. A couple months ago (I think) we started running into a failure where one of our dependencies (holoviews) started crashing when performing the check isinstance(np.size, types.FunctionType). We are not installing a dev version of holoviews. It seems to be tied to numpy's dev version.

Reproduce the code example:

import numpy as np
import types
assert isinstance(np.size, types.FunctionType)

Error message:

N/A

Runtime information:

[{'numpy_version': '1.25.0.dev0+831.g3b5eff00e',
  'python': '3.9.13 | packaged by conda-forge | (main, May 27 2022, '
            '16:56:21) \n'
            '[GCC 10.3.0]',
  'uname': uname_result(system='Linux', node='janet', release='6.0.12-76060006-generic', version='#202212290932~1674139725~22.04~ca93ccf SMP PREEMPT_DYNAMIC Thu J', 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']}},
 {'architecture': 'Haswell',
  'filepath': '/home/davidh/miniconda3/envs/satpy_py39_unstable2/lib/python3.9/site-packages/numpy.libs/libopenblas64_p-r0-15028c96.3.21.so',
  'internal_api': 'openblas',
  'num_threads': 12,
  'prefix': 'libopenblas',
  'threading_layer': 'pthreads',
  'user_api': 'blas',
  'version': '0.3.21'}]

Context for the issue:

I'm not sure how many packages have code like this, but holoviews has this bit of code:

https://github.com/holoviz/holoviews/blob/4e83af3e6af0e19fa6c7e226791011372916fc70/holoviews/plotting/bokeh/hex_tiles.py#L26-L30

    aggregator = param.ClassSelector(
        default=np.size, class_=(types.FunctionType, tuple), doc="""
      Aggregation function or dimension transform used to compute bin
      values. Defaults to np.size to count the number of values
      in each bin.""")

And the param library does this check between that class_ tuple and the default object (in this case np.size):

https://github.com/holoviz/param/blob/cd1b4ae50c01c14c0eb22721e41a9c8fb1e8f738/param/__init__.py#L1381-L1385

        if is_instance:
            if not (isinstance(val, class_)):
                raise ValueError(
                    "%s parameter %r value must be an instance of %s, not %r." %
                    (param_cls, self.name, class_name, val))

I am not a holoviews developer, just use it for one small portion of my package (Satpy). As far as I can tell np.size is defined as a function here:

@array_function_dispatch(_size_dispatcher)
def size(a, axis=None):

So I'm lost as to where/how this is not being satisfied. I've seen the failure on Python 3.9 environments. I have not tried other versions of Python.

@djhoese djhoese changed the title BUG: np.size is no longer an instance of FunctionType BUG: np.size is no longer an instance of FunctionType (nightly/dev build) Mar 1, 2023
@djhoese
Copy link
Contributor Author

djhoese commented Mar 1, 2023

Just to be sure it wasn't some weird Python 3.9 thing, here's a 3.11 environment:

conda create -n numpy_dev_py311 python=3.11 numpy
conda activate numpy_dev_py311
conda uninstall -y --force numpy
python -m pip install --index-url https://pypi.anaconda.org/scipy-wheels-nightly/simple/ --trusted-host pypi.anaconda.org --no-deps --pre --upgrade numpy
python -c "import numpy as np; import types; print(isinstance(np.size, types.FunctionType))"

Prints False, but should be True unless this is expected.

@ngoldbaum
Copy link
Member

This is happening because of #23020, which moved the __array_function__ dispatcher into C. Maybe @seberg has other thoughts given the downstream breakage in holoviews, but one way holoviews could fix this is by using the inspect module:

In [7]: inspect.isroutine(np.size)
Out[7]: True

#23032 is related as well, since the same change also impacted sphinx.

@djhoese
Copy link
Contributor Author

djhoese commented Mar 1, 2023

I'm not sure inspect would work for them. At least not as a drop in replacement. They are checking if some object is an instance of one or more classes. In this particular case they are checking that something callable is a FunctionType which is maybe too strict of a check (a class with a __call__ method returns False for this isinstance check).

Maybe it will require more work in holoviews.

Edit: Actually a __call__ method on an instance fails isroutine too.

@seberg
Copy link
Member

seberg commented Mar 1, 2023

isroutine is more limited than having a __call__ or callable. typing.Callable also works for isinstance().
Not sure if there is a different abstract class that would be a simple drop-in replacement, although it would be pretty trivial to write something fairly sensible:

import abc

class MyCallable(abc.ABC):
    @classmethod
    def __subclasscheck__(cls, other):
        return hasattr(other, "__call__")

Note that FunctionType (unlike isroutine) does not even cover functions defined in C. Is this a serious "everything is coming down crashes" thing, or more a question of how to fix it in holoviz?

@djhoese
Copy link
Contributor Author

djhoese commented Mar 2, 2023

Is this a serious "everything is coming down crashes" thing, or more a question of how to fix it in holoviz?

I honestly have no idea. Holoviews is used in one small part of one small utility function of the library I work on. Otherwise, I guess technically this could trigger edge cases in any library that likes to pass function pointers around.

I did notice something interesting, if you print out the repr it still says/thinks it is a function:

>>> import numpy as np
>>> np.size
<function size at 0x7f711b14cc30>
>>> np.size.__class__
<class 'numpy._ArrayFunctionDispatcher'>
>>> np.size.__class__.__class__
<class 'type'>

But I guess _ArrayFunctionDispatcher being of type type means np.size is an instance of a class. Unless _ArrayFunctionDispatcher was made to be a subclass (or call into) the builtin Function type we can't hope for np.size to be a more recognizable function. And like you said, if any part of this is C functions that aren't carefully handled isinstance with FunctionType isn't going to work anyway.

Ok enough rambling...I'm sure there will be other cases of this change causing issues downstream but the fact that no one has brought it up before me suggests you/we won't find out until release.

@djhoese
Copy link
Contributor Author

djhoese commented Mar 3, 2023

The issues in holoviews have now been fixed by using Callable instead of FunctionType. Feel free to close this or completely rewrite _ArrayFunctionDispatcher to present as functions 😉

@seberg
Copy link
Member

seberg commented Mar 3, 2023

Feel free to close this or completely rewrite _ArrayFunctionDispatcher to present as functions

That is not possible, nor is it something Python does. Check abs, or len or functools.partial or any function defined in Cython or...

@seberg seberg closed this as completed Mar 3, 2023
@djhoese
Copy link
Contributor Author

djhoese commented Mar 3, 2023

Wow, this led me down a short rabbit hole. Python (def/lambda) functions versus all other types of callables. Interesting. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants