Skip to content
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

CI: run pyright #43672

Merged
merged 8 commits into from
Sep 25, 2021
Merged

CI: run pyright #43672

merged 8 commits into from
Sep 25, 2021

Conversation

twoertwein
Copy link
Member

@twoertwein twoertwein commented Sep 20, 2021

Run pyright in strict mode with a very long list of rules that need to be ignored. The idea is to enable more rules over time.

pre-commit seamlessly installs pyright (depends on node.js) but running pyright as part of the pre-commit slows the pre-commit down by ~85s.

@jreback jreback added CI Continuous Integration Typing type annotations, mypy/pyright type checking labels Sep 21, 2021
@twoertwein twoertwein marked this pull request as ready for review September 21, 2021 12:37
@twoertwein
Copy link
Member Author

Some points to consider:

  • As far as I know, pyright does not have an option to ignore type errors in specific lines. If it is a false positive, pyright will most likely fix it within days(!). If it is a type error, pyright recommends to use cast.
  • Pyright can exclude/include files/directories.
  • @erictraut recommends to focus on enabling reportGeneralTypeIssues for at least a small list of files (and then growing the list of files from there).

While this PR enables very few rules for all files (except for the tests), it would make sense in subsequent PRs to prioritize fixing reportGeneralTypeIssues: do not cover all files with pyright but enable more rules (specifically reportGeneralTypeIssues).

@erictraut
Copy link

You can ignore type errors on a specific line using a # type: ignore comment at the end of that line. This is documented in PEP 484 and is supported by all Python type checkers.

@twoertwein
Copy link
Member Author

You can ignore type errors on a specific line using a # type: ignore comment at the end of that line. This is documented in PEP 484 and is supported by all Python type checkers.

Thank you for the correction! I remembered one issue on pyright that mentioned that there wasn't this possibility.

@erictraut
Copy link

erictraut commented Sep 25, 2021

I noticed that you added "pandas/tests" to the "ignore" section of the configuration. This tells pyright to analyze these files but suppress reporting any diagnostics in them. I recommend moving it to "exclude" instead, which tells pyright to avoid analyzing them unless they are explicitly imported by other modules that are included in the analysis set. Making this change drops the total analysis time from 67 seconds to 19 seconds on my machine.

exclude = ["pandas/tests"]

I'll also point out that as more type annotations are added to the code base, the overall analysis time will decrease. Currently, pyright needs to perform a lot of expensive computations to infer function return types based on the function implementation and call-site argument types. I would anticipate that the total analysis time would drop by 50% or more as you add more type annotations.

@twoertwein
Copy link
Member Author

Making this change drops the total analysis time from 67 seconds to 19 seconds on my machine.

That's amazing, thank you for noticing that! 19s should be more than fast enough to run pyright with all pre-commit hooks.

@twoertwein
Copy link
Member Author

twoertwein commented Sep 25, 2021

mypy used to be in the pre-commit but was removed in #35066 (because of running in a different environment and bugs(?) in mypy pre-commit/pre-commit#1580 (comment)).

We probably need to add all the type-stub packages as dependencies to the pyright hook (pyright comes with its copy of typeshed, so we might not need to install various type-stub packages(?)). At a minimum, we probably need to add numpy>1.21.0 to have access to numpy.typing.

edit: It seems that pre-commit doesn't isolate pyright from the current python environment because it is a node.js application(?)! That's very convenient, especially because it seems that pre-commit doesn't support installing node dependencies and python dependencies for the same check.

@jreback jreback added this to the 1.4 milestone Sep 25, 2021
@jreback jreback merged commit 5441d4e into pandas-dev:master Sep 25, 2021
@jreback
Copy link
Contributor

jreback commented Sep 25, 2021

thanks @twoertwein very nice.

@jreback
Copy link
Contributor

jreback commented Sep 25, 2021

pls open issues as appropriate (or a single with check boxes) to enable features on pyright checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants