[reland] Refactor mypy configs list into editor-friendly wrapper by samestep · Pull Request #50826 · pytorch/pytorch

samestep · 2021-01-20T18:06:45Z

Closes #50513 by resolving all four checkboxes. If this PR is merged, I will also modify one or both of the following wiki pages to add instructions on how to use this mypy wrapper for VS Code editor integration:

Test plan:

Unit tests for globbing function:

python test/test_testing.py TestMypyWrapper -v

Manual checks:

Uninstall mypy and run python test/test_type_hints.py to verify that it still works when mypy is absent.
Reinstall mypy and run python test/test_type_hints.py to verify that this didn't break the TestTypeHints suite.
Run python test/test_type_hints.py again (should finish quickly) to verify that this didn't break mypy caching.

Run torch/testing/_internal/mypy_wrapper.py on a few Python files in this repo to verify that it doesn't give any additional warnings when the TestTypeHints suite passes. Some examples (compare with the behavior of just running mypy on these files):

torch/testing/_internal/mypy_wrapper.py $PWD/README.md
torch/testing/_internal/mypy_wrapper.py $PWD/tools/fast_nvcc/fast_nvcc.py
torch/testing/_internal/mypy_wrapper.py $PWD/test/test_type_hints.py
torch/testing/_internal/mypy_wrapper.py $PWD/torch/random.py
torch/testing/_internal/mypy_wrapper.py $PWD/torch/testing/_internal/mypy_wrapper.py

Remove type hints from torch.testing._internal.mypy_wrapper and verify that running mypy_wrapper.py on that file gives type errors.
Remove the path to mypy_wrapper.py from the files setting in mypy-strict.ini and verify that running it again on itself no longer gives type errors.
Add test/test_type_hints.py to the files setting in mypy-strict.ini and verify that running the mypy wrapper on it again now gives type errors.
Change a return type in torch/random.py and verify that running the mypy wrapper on it again now gives type errors.
Add the suggested JSON from the docstring of torch.testing._internal.mypy_wrapper.main to your .vscode/settings.json and verify that VS Code gives the same results (inline, while editing any Python file in the repo) as running the mypy wrapper on the command line, in all the above cases.

facebook-github-bot

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ezyang · 2021-01-21T15:05:04Z

This PR is making me wonder if there isn't a way to have a single unified mypy configuration, so you no longer need to do have a script groveling through the ini files to work out which configuration should be used in. I will admit that when I added mypy-strict.ini, I didn't know very much about how mypy config files worked, and just did the thing that I knew would work for making stricter checking happen.

samestep · 2021-01-21T15:48:10Z

@ezyang I'd be up for unifying the configs if it's possible, since ideally it'd be good not to incur the complexity cost this PR adds (or more accurately, the existing complexity of having multiple mypy configs). I don't currently know a ton about mypy config files, but I see the difficulty as being twofold:

Because we set many different flags in mypy-strict.ini, it seems like we'd need to add separate sections to mypy.ini duplicating those flags for each of those patterns (e.g. [mypy-tools.codegen.gen], [mypy-tools.autograd.*], etc., each including the same long list of flags).
Although merging the configs would address the second checkbox in Make it easier to run mypy locally #50513, the first checkbox would still apply, because if you run mypy on an individual file, it overrides the files setting in the config file. Thus, I believe we would also need to restructure the config such that it doesn't set files at all (perhaps instead setting ignore_errors = True for everything in the complement of the set currently specified by files).

If we resolve point 1 but not point 2 then I'd still need part of the functionality of the mypy_wrapper.py script this PR adds (which I can obviously use regardless of whether this PR is merged, at the expense of the third checkbox in #50513).

samestep · 2021-01-21T15:49:43Z

I see this mypy issue opened in October which sounds somewhat similar to what we'd like to be able to do, but it doesn't seem to have gotten much attention.

facebook-github-bot

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

rgommers

This simplifies the test file, so LGTM modulo my minor comments. The manual test plan plus CI passing seems fine to me.

facebook-github-bot

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-01-22T21:39:11Z

@samestep merged this pull request in 73dffc8.

samestep · 2021-01-23T00:00:11Z

Reverting this PR because its test_glob test fails on Windows.

facebook-github-bot

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

walterddr

lgtm

codecov · 2021-01-25T20:55:02Z

Codecov Report

Merging #50826 (40f1eac) into master (e34992e) will decrease coverage by 0.00%.
The diff coverage is 57.57%.

@@            Coverage Diff             @@
##           master   #50826      +/-   ##
==========================================
- Coverage   80.99%   80.98%   -0.01%     
==========================================
  Files        1916     1917       +1     
  Lines      209552   209585      +33     
==========================================
+ Hits       169736   169742       +6     
- Misses      39816    39843      +27

Summary: Closes pytorch#50513 by resolving the first three checkboxes. If this PR is merged, I will also modify one or both of the following wiki pages to add instructions on how to use this `mypy` wrapper for VS Code editor integration: - [Guide for adding type annotations to PyTorch](https://github.com/pytorch/pytorch/wiki/Guide-for-adding-type-annotations-to-PyTorch) - [Lint as you type](https://github.com/pytorch/pytorch/wiki/Lint-as-you-type) The test plan below is fairly manual, so let me know if I should add more automated tests to this PR. Pull Request resolved: pytorch#50826 Test Plan: Unit tests for globbing function: ``` python test/test_testing.py TestMypyWrapper -v ``` Manual checks: - Uninstall `mypy` and run `python test/test_type_hints.py` to verify that it still works when `mypy` is absent. - Reinstall `mypy` and run `python test/test_type_hints.py` to verify that this didn't break the `TestTypeHints` suite. - Run `python test/test_type_hints.py` again (should finish quickly) to verify that this didn't break `mypy` caching. - Run `torch/testing/_internal/mypy_wrapper.py` on a few Python files in this repo to verify that it doesn't give any additional warnings when the `TestTypeHints` suite passes. Some examples (compare with the behavior of just running `mypy` on these files): ```sh torch/testing/_internal/mypy_wrapper.py README.md torch/testing/_internal/mypy_wrapper.py tools/fast_nvcc/fast_nvcc.py torch/testing/_internal/mypy_wrapper.py test/test_type_hints.py torch/testing/_internal/mypy_wrapper.py torch/random.py torch/testing/_internal/mypy_wrapper.py torch/testing/_internal/mypy_wrapper.py ``` - Remove type hints from `torch.testing._internal.mypy_wrapper` and verify that running `mypy_wrapper.py` on that file gives type errors. - Remove the path to `mypy_wrapper.py` from the `files` setting in `mypy-strict.ini` and verify that running it again on itself no longer gives type errors. - Add `test/test_type_hints.py` to the `files` setting in `mypy-strict.ini` and verify that running the `mypy` wrapper on it again now gives type errors. - Remove type hints from `torch/random.py` and verify that running the `mypy` wrapper on it again now gives type errors. - Add the suggested JSON from the docstring of `torch.testing._internal.mypy_wrapper.main` to your `.vscode/settings.json` and verify that VS Code gives the same results (inline, while editing any Python file in the repo) as running the `mypy` wrapper on the command line, in all the above cases. Reviewed By: glaringlee, walterddr Differential Revision: D25977352 Pulled By: samestep fbshipit-source-id: 4b3a5e8a9071fcad65a19f193bf3dc7dc3ba1b96

…orch#50826) Summary: Closes pytorch#50513 by resolving all four checkboxes. If this PR is merged, I will also modify one or both of the following wiki pages to add instructions on how to use this `mypy` wrapper for VS Code editor integration: - [Guide for adding type annotations to PyTorch](https://github.com/pytorch/pytorch/wiki/Guide-for-adding-type-annotations-to-PyTorch) - [Lint as you type](https://github.com/pytorch/pytorch/wiki/Lint-as-you-type) Pull Request resolved: pytorch#50826 Test Plan: Unit tests for globbing function: ``` python test/test_testing.py TestMypyWrapper -v ``` Manual checks: - Uninstall `mypy` and run `python test/test_type_hints.py` to verify that it still works when `mypy` is absent. - Reinstall `mypy` and run `python test/test_type_hints.py` to verify that this didn't break the `TestTypeHints` suite. - Run `python test/test_type_hints.py` again (should finish quickly) to verify that this didn't break `mypy` caching. - Run `torch/testing/_internal/mypy_wrapper.py` on a few Python files in this repo to verify that it doesn't give any additional warnings when the `TestTypeHints` suite passes. Some examples (compare with the behavior of just running `mypy` on these files): ```sh torch/testing/_internal/mypy_wrapper.py $PWD/README.md torch/testing/_internal/mypy_wrapper.py $PWD/tools/fast_nvcc/fast_nvcc.py torch/testing/_internal/mypy_wrapper.py $PWD/test/test_type_hints.py torch/testing/_internal/mypy_wrapper.py $PWD/torch/random.py torch/testing/_internal/mypy_wrapper.py $PWD/torch/testing/_internal/mypy_wrapper.py ``` - Remove type hints from `torch.testing._internal.mypy_wrapper` and verify that running `mypy_wrapper.py` on that file gives type errors. - Remove the path to `mypy_wrapper.py` from the `files` setting in `mypy-strict.ini` and verify that running it again on itself no longer gives type errors. - Add `test/test_type_hints.py` to the `files` setting in `mypy-strict.ini` and verify that running the `mypy` wrapper on it again now gives type errors. - Change a return type in `torch/random.py` and verify that running the `mypy` wrapper on it again now gives type errors. - Add the suggested JSON from the docstring of `torch.testing._internal.mypy_wrapper.main` to your `.vscode/settings.json` and verify that VS Code gives the same results (inline, while editing any Python file in the repo) as running the `mypy` wrapper on the command line, in all the above cases. Reviewed By: walterddr Differential Revision: D26049052 Pulled By: samestep fbshipit-source-id: 0b35162fc78976452b5ea20d4ab63937b3c7695d

samestep requested review from a team, ezyang and rgommers January 20, 2021 18:06

facebook-github-bot added the cla signed label Jan 20, 2021

facebook-github-bot reviewed Jan 20, 2021

View reviewed changes

samestep mentioned this pull request Jan 20, 2021

Make it easier to run mypy locally #50513

Closed

4 tasks

samestep commented Jan 20, 2021

View reviewed changes

Comment thread test/test_type_hints.py Outdated

facebook-github-bot reviewed Jan 20, 2021

View reviewed changes

facebook-github-bot reviewed Jan 21, 2021

View reviewed changes

rgommers added the module: typing Related to mypy type annotations label Jan 22, 2021

rgommers approved these changes Jan 22, 2021

View reviewed changes

Comment thread torch/testing/_internal/mypy_wrapper.py Outdated

facebook-github-bot reviewed Jan 22, 2021

View reviewed changes

facebook-github-bot closed this in 73dffc8 Jan 22, 2021

facebook-github-bot added the Merged label Jan 22, 2021

samestep reopened this Jan 22, 2021

samestep added 4 commits January 25, 2021 07:38

Refactor mypy configs list into mypy wrapper

87ec2f3

Pass TestTypeHints regardless of mypy or its stub

0dd3d21

Address review comments and fix globbing bug

77ae4fe

Fix test_glob on Windows hopefully

40f1eac

facebook-github-bot reviewed Jan 25, 2021

View reviewed changes

walterddr approved these changes Jan 25, 2021

View reviewed changes

samestep changed the title ~~Refactor mypy configs list into editor-friendly wrapper~~ [reland] Refactor mypy configs list into editor-friendly wrapper Jan 25, 2021

facebook-github-bot closed this in 6dda036 Jan 26, 2021

samestep mentioned this pull request Feb 9, 2021

Don't constrain Flake8 and mypy to specific files ezyang/ghstack#37

Merged

samestep deleted the refactor-mypy-tests-into-wrapper branch April 21, 2021 17:40

Conversation

samestep commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Jan 21, 2021

Uh oh!

samestep commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samestep commented Jan 21, 2021

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

rgommers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 22, 2021

Uh oh!

samestep commented Jan 23, 2021

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jan 25, 2021

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

samestep commented Jan 20, 2021 •

edited

Loading

samestep commented Jan 21, 2021 •

edited

Loading