TYP: Check untyped defs (except vendored)

[simonjayhawkins]

@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

[simonjayhawkins]

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 #30868 (comment)

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.

[jbrockmendel]

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?

[simonjayhawkins]

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.

[jbrockmendel]

makes sense, thanks

[simonjayhawkins]

would preceding the comments with a TODO: for the ones added here help differentiate?

[jbrockmendel]

Probably not worth worrying about

[jbrockmendel]

LGTM cc @jreback

[jbrockmendel]

needs rebase

[jreback]

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.

[simonjayhawkins]

@jreback green

[jreback]

thanks @simonjayhawkins pls open an issue to track the fixing of these

[simonjayhawkins]

thanks @simonjayhawkins pls open an issue to track the fixing of these

issue opened #37715