Don't type check __getattr__?

[mathause]

In #4592 I had the issue that mypy did not raise an error on a missing method:

from xarray.core.common import DataWithCoords

hasattr(xr.core.common.DataWithCoords, "reduce") # -> False

def test(x: "DataWithCoords"):
    x.reduce() # mypy does not error

This is because DataWithCoords implements __getattr__:

class A:
    pass

class B:
    def __getattr__(self, name):
        ...

def testA(x: "A"):
    x.reduce() # mypy errors

def testB(x: "B"):
    x.reduce() # mypy does not error

The solution seems to be to not typecheck __getattr__ (see python/mypy#6251 (comment)):

from typing import no_type_check

class C:
    @no_type_check
    def __getattr__(self, name):
        ...

def testC(x: "C"):
    x.reduce() # mypy errors

The only __getattr__ within xarray is here:

xarray/xarray/core/common.py

Line 221 in 1735892

def __getattr__(self, name: str) -> Any:

Using @no_type_check leads to 24 errors and not all of them can be trivially solved. E.g. DataWithCoords wants of use self.isel but does not implement the method. The solution is probably to add isel to DataWithCoords as an ABC or using NotImplemented.

Thoughts?

All errors

xarray/core/common.py:370: error: "DataWithCoords" has no attribute "isel"
xarray/core/common.py:374: error: "DataWithCoords" has no attribute "dims"
xarray/core/common.py:378: error: "DataWithCoords" has no attribute "indexes"
xarray/core/common.py:381: error: "DataWithCoords" has no attribute "sizes"
xarray/core/common.py:698: error: "DataWithCoords" has no attribute "_groupby_cls"
xarray/core/common.py:761: error: "DataWithCoords" has no attribute "_groupby_cls"
xarray/core/common.py:866: error: "DataWithCoords" has no attribute "_rolling_cls"; maybe "_rolling_exp_cls"?
xarray/core/common.py:977: error: "DataWithCoords" has no attribute "_coarsen_cls"
xarray/core/common.py:1108: error: "DataWithCoords" has no attribute "dims"
xarray/core/common.py:1109: error: "DataWithCoords" has no attribute "dims"
xarray/core/common.py:1133: error: "DataWithCoords" has no attribute "indexes"
xarray/core/common.py:1144: error: "DataWithCoords" has no attribute "_resample_cls"; maybe "resample"?
xarray/core/common.py:1261: error: "DataWithCoords" has no attribute "isel"
xarray/core/alignment.py:278: error: "DataAlignable" has no attribute "copy"
xarray/core/alignment.py:283: error: "DataAlignable" has no attribute "dims"
xarray/core/alignment.py:286: error: "DataAlignable" has no attribute "indexes"
xarray/core/alignment.py:288: error: "DataAlignable" has no attribute "sizes"
xarray/core/alignment.py:348: error: "DataAlignable" has no attribute "dims"
xarray/core/alignment.py:351: error: "DataAlignable" has no attribute "copy"
xarray/core/alignment.py:353: error: "DataAlignable" has no attribute "reindex"
xarray/core/alignment.py:356: error: "DataAlignable" has no attribute "encoding"
xarray/core/weighted.py:157: error: "DataArray" has no attribute "notnull"
xarray/core/dataset.py:3792: error: "Dataset" has no attribute "virtual_variables"
xarray/core/dataset.py:6135: error: "DataArray" has no attribute "isnull"

Edit:
one problem is certainly the method injection, as mypy cannot detect those types.

[max-sixty]

Great observation @mathause . I think there are two parts of this:

• Do we want other libraries which do da.longitude to raise a mypy error? That may be a tradeoff with raising the true error on da.isel
• How do we design the type hierarchy? We could add methods to DataWithCoords or add some Dataset_Or_DataArray-like type

Having methods like isel typed would be a win I think

[mathause]

Do we want other libraries which do da.longitude to raise a mypy error? That may be a tradeoff with raising the true error on da.jsel

Good point. Given that the accessors are also going via __getattr__ I would not remove the typing. Due to the accessors it also needs to be -> Any. In conclusion DataWithCoords only makes sense as a mixin.

[max-sixty]

Good point re accessors, I hadn't considered those. So sounds like raising an error on da.isel isn't possible regardless...

[shoyer]

I think we can probably safely add @no_type_check to __getattr__ here, though it's true DataWithCoords was only intended as a mix-in.

Looking up coordinates with attributes like da.longitude is a shortcut mostly intended for interactive use-cases. If you are type-checking your code for safely, you should likely prefer writing da['longitude'] to remove ambiguity about whether ds.longitude is a method, accessor or DataArray object.

[mathause]

I can open a PR but is there a clean way to handle accessors?

Do you prefer ABCs or NotImplementedError for the missing methods in DataWithCoords?

[headtr1ck]

Has anyone tried this recently? For me mypy ignores the @no_type_check decorator and never raises an error anyway.

I think raising errors on not defined attributes is useful for typos but will likely annoy people that use custom accessors or use the variable shortcut (although the latter apparently don't care about type checking anyway).

[headtr1ck]

Has anyone tried this recently? For me mypy ignores the @no_type_check decorator and never raises an error anyway.

Figured it out: @no_type_check only removes typing for arguments and returns, so basically the function behaves as untyped. What we want is that mypy thinks this method does not exist at all. This can only be done by wrapping the complete method definition with if not TYPE_CHECKING.

[headtr1ck]

In #8204 I have noticed that defining __getattr__ will make the class pass any Protocol checks. (Maybe that's a Mypy bug or just a very bad side effect that should raise a Mypy warning if done).