Ensure isinstance checks use ABC classes where available

[WillAyd]

Here's a quick little script I wrote to check for things like isinstance(obj, MultiIndex)

import ast
import pathlib


# Taken from pandas.core.dtypes.generic
BAD_NAMES = {'Index', 'Int64Index', 'UInt64Index', 'RangeIndex',
             'Float64Index', 'MultiIndex', 'DatetimeIndex', 'TimedeltaIndex',
             'PeriodIndex', 'CategoricalIndex', 'IntervalIndex', 'IndexClass',
             'Series', 'DataFrame', 'SparseDataFrame', 'SparseSeries',
             'SparseArray', 'Categorical', 'DatetimeArray', 'TimedeltaArray',
             'PeriodArray', 'Period', 'DateOffset', 'Interval', 'ExtensionArray',
             'PandasArray'}

def check_name_or_attr(filename, node):
    if isinstance(node, ast.Name):  # direct name
        if node.id in BAD_NAMES:
            print(f"{filename}:{node.lineno} isinstance check of {node.id}")
    elif isinstance(node, ast.Attribute):  # ex: pd.DataFrame
        if node.attr in BAD_NAMES:
            print(f"{filename}:{node.lineno} isinstance check of {node.attr}")
    

for path in pathlib.Path('.').glob('**/*.py'):
    filename = path.resolve()

    with open(path) as fh:
        contents = fh.read()
        tree = ast.parse(contents)

        for node in ast.walk(tree):
            try:
                isinstance_check = node.value.func.id == 'isinstance'
            except AttributeError:
                continue

            if isinstance_check:
                types = node.value.args[1]

                if isinstance(types, ast.Tuple):
                    for elt in types.elts:
                        check_name_or_attr(filename, elt)
                else:
                    check_name_or_attr(filename, types)

Running that from project root yields all of the following errors:

/Users/williamayd/clones/pandas/pandas/core/frame.py:4681 isinstance check of MultiIndex
/Users/williamayd/clones/pandas/pandas/core/indexing.py:2499 isinstance check of MultiIndex
/Users/williamayd/clones/pandas/pandas/core/indexing.py:564 isinstance check of MultiIndex
/Users/williamayd/clones/pandas/pandas/core/reshape/tile.py:540 isinstance check of Series
/Users/williamayd/clones/pandas/pandas/core/reshape/concat.py:373 isinstance check of DataFrame
/Users/williamayd/clones/pandas/pandas/core/reshape/concat.py:377 isinstance check of Series
/Users/williamayd/clones/pandas/pandas/core/computation/align.py:72 isinstance check of Series
/Users/williamayd/clones/pandas/pandas/core/arrays/period.py:931 isinstance check of Period
/Users/williamayd/clones/pandas/pandas/core/arrays/period.py:932 isinstance check of Period
/Users/williamayd/clones/pandas/pandas/core/indexes/base.py:3626 isinstance check of MultiIndex
/Users/williamayd/clones/pandas/pandas/core/indexes/base.py:3627 isinstance check of MultiIndex
/Users/williamayd/clones/pandas/pandas/io/formats/excel.py:453 isinstance check of Index
/Users/williamayd/clones/pandas/pandas/io/formats/excel.py:501 isinstance check of Index
/Users/williamayd/clones/pandas/pandas/io/formats/excel.py:552 isinstance check of Index
/Users/williamayd/clones/pandas/pandas/io/formats/excel.py:592 isinstance check of Index

So I think these need to be swapped out for their ABC equivalents

[jbrockmendel]

If they aren't inducing otherwise-unnecessary runtime imports, the non-ABC versions are fine, aren't they?

FWIW I benchmarked these a long time ago and isinstance(x, ABCFoo) is about twice the time as isinstance(x, Foo), though both numbers are tiny.

[WillAyd]

Yea valid points but I'm not sure what percent of these they would apply to. Particular with pandas.core.frame (the first issue) we are only import MultiIndex for is instancechecks when we actually already have the ABCMultiIndex in there as well, so that at least is a very good candidate to change

Noted on performance but we are still in the range of nanoseconds so I think OK unless stuck in a performance critical loop

[jreback]

would be +1 in making imports simpler / more consistent

[souvik3333]

Hi @WillAyd, can you explain bit more about it? I am new here and interested in it.

[WillAyd]

Sure - for any of the failures at the bottom of the OP there should be an ABC class defined in pandas.core.dtypes.generic that you could use instead, especially if we only import the objects in the OP for isinstance checks

[addisonlynch]

@WillAyd I think I can tackle this...there may be a number of additional instances that the script is missing because it's not parsing multi-line statements. Consider pandas/core/frame.py (script has identified the below line):

pandas/pandas/core/frame.py

Line 4615 in 5d9fd7e

multi_col = isinstance(self.columns, MultiIndex)

But since the ast parser can only handle simple statements, multi-line statements like the following are ignored:

pandas/pandas/core/frame.py

Lines 1737 to 1739 in 5d9fd7e

	if isinstance(self.index, MultiIndex):
	# array of tuples to numpy cols. copy copy copy
	ix_vals = list(map(np.array, zip(*self.index.values)))

In this file, MultiIndex is indeed only imported for the isinstance checks. Just wanted to confirm with you before updating the script (or perhaps you could do so, I'm not an expert with ast so I'm not sure how to tokenize the multi-line statements such as the above)

[WillAyd]

Thanks for looking. The script is probably secondary to actually making the changes so if you’ve looked and would like to submit a PR can take things from there

[simonjayhawkins]

when using the ABCs in isinstance checks the expressions resolves to Any.

so every PR citing this issue is reducing the type checking and not allowing mypy to narrow types.

should we have ABCs that are not created dynamically at runtime or should we only use the ABCs where necessary to eliminate the need for a cast.

the contributing guide would suggest the later in the short term until we have a solution.. "the use of cast is strongly discouraged. Where applicable a refactor of the code to appease static analysis is preferable"

[simonjayhawkins]

should we have ABCs that are not created dynamically at runtime or should we only use the ABCs where necessary to eliminate the need for a cast.

also could add types to pandas.core.dtypes.generic using cast or add a stub file for pandas.core.dtypes.generic

adding a stub file would be inconsistent with our current approach for a standard internal python module, but is straightforward if we don't need to type check pandas.core.dtypes.generic itself. (it's mainly dynamic in nature).

[WillAyd]

FWIW I benchmarked these a long time ago and isinstance(x, ABCFoo) is about twice the time as isinstance(x, Foo), though both numbers are tiny.

By the way @jbrockmendel what you describe might have been resolved in Py37:

https://bugs.python.org/issue31333

Haven't benchmarked personally just sharing as I came across