TYP: Check untyped defs (except vendored)
State: closed

@jbrockmendel has recently addressed many of the modules that were not previously checked. There are now many less modules outstanding.

There has been a concern in the past that if we add ignores, the issues may not be addressed and sit in the codebase. (from previous experience with other linting tools)

There are however pros to including the ignores. The issues are visible to people working on the relevant code and when reviewing the code. **Also as mypy will be checking the relevant modules, PRs changing these modules will have additional static checking on ci and the chance of regression/errors is likely reduced.**

IMO the pros always outweighed the cons, but now that there are less ignores involved (admittedly the number is still not trivial), I think it is appropriate to reconsider this approach.

One caveat, NumPy types currently resolve to `Any` and we have not yet discussed how we will 'switch' to using NumPy types #36092. (reverting to using `check_untyped_defs=False` on some modules could make the switch more managable)

Another possible upside, is in the review of typing PRs to fix these issues. Reviewers of typing PRs cannot see explicitly see why changes are being made and a typical reviewer is normally suspicious of any changes to the codebase. This can lead to long review cycles of typing PRs. I'm not sure if having the resolved mypy errors in the diff will help, but if the hypothesis is correct could be an argument for following a similar approach for resolving the numerous errors that will be reported once NumPy types no longer resolve to `Any`

Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
Interesting. MyPy on local(Windows) reports `pandas\_testing.py:2845: error: Module has no attribute "tzset"`.

on ci here we get `pandas/_testing.py:2913: error: unused 'type: ignore' comment`

see https://stackoverflow.com/questions/62004265/python-3-time-tzset-alternative-for-windows

to avoid platform consistency issues when running mypy, in #28339 I added `platform = linux-64` (and `python_version = 3.6`) to the mypy config. I'll do the same here for now for platform only.

Running the test suite locally (on Windows) fails, see https://github.com/pandas-dev/pandas/issues/30868#issuecomment-572837671

So as an aside to this issue. We might want to consider running mypy using a platform/python version matrix if it can detect these types of issue.


im going to follow your lead on this if you think its worthwhile, but one quibble: currently `type:ignore` indicates "someone has looked at this at least somewhat carefully and not found a better option", whereas check_untyped_defs=False suggests the possibility of relatively low hanging fruit.  Are we at a point where the latter implication is inaccurate?
pyre has two ignore options `pyre-fixme` and `pyre-ignore`. from https://pyre-check.org/docs/errors/#suppressing-individual-errors

> `# pyre-fixme` indicates there is an issue in the code that will be revisited later.
> `# pyre-ignore` indicates there's an issue with the type checker or the code is too dynamic and we have decided to not fix this. If this is a Pyre bug, make sure you open an issue on our tracker.

we have the convention of adding the mypy issue link if the ignore is there because of an issue with the type checker.

imo if we have an ignore that doesn't have a mypy issue then it was added because it will be revisted later.

I'm not sure that we have (m)any instances where we have decided that the code is too dynamic.

> whereas check_untyped_defs=False suggests the possibility of relatively low hanging fruit. Are we at a point where the latter implication is inaccurate?

from the diff, we should be able to identify any easy fixes that could be done as a precursor. oth those fixes may easier to review once the mypy errors are explicitly in the diff.
makes sense, thanks
would preceding the comments with a `TODO:` for the ones added here help differentiate?
Probably not worth worrying about
LGTM cc @jreback 
needs rebase
yeah i think this is ok, but for sure want to try to fix this (as I know you have been).

can you merge master one more go and ping on green.
@jreback green
thanks @simonjayhawkins pls open an issue to track the fixing of these
> thanks @simonjayhawkins pls open an issue to track the fixing of these

issue opened #37715