Warn if mypy version doesn't match CI #51799
Conversation
💊 CI failures summary and remediationsAs of commit 39f02aa (more details on the Dr. CI page):
1 failure not recognized by patterns:
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
There was a problem hiding this comment.
@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Could this be too strict? Is it the case that most mypy versions work with the one we use, or is it more like using any other version would cause conflicting warnings? If the first, I have high level questions:
|
|
@janeyx99 Good questions. I'm not sure about "most" With regard to your two questions, could you clarify more what the motivation would be for printing this warning in fewer cases, or for not failing the local test run? Note that with this PR as-is, the warning will only appear if all of these conditions are met:
Given that the error message shows what command to run to correct the |
|
Yes, I was thinking of if the local mypy version is okay (i.e., does not give warnings we don't care about), it might be annoying to see this warning message every time mypy was run. Basically, if a programmer is doing fine with a different mypy version, I'm not sure we want to insist a switch. |
Are we assuming that the developer already has a Python environment (either I suppose I could modify the message to say that if you don't already have a Python environment for PyTorch development and are just using the global environment, then you should make one. |
|
Yea that's fair. This would be the way to enforce that PyTorch devs should use the same version of mypy used in CI. The enforcement can vary in degree where it's more a suggestion/warning vs a rule, but I'm not sure how much we want to be lax about it so you can decide :) |
There was a problem hiding this comment.
@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: This PR adds a local [`mypy` plugin](https://mypy.readthedocs.io/en/stable/extending_mypy.html#extending-mypy-using-plugins) that warns if you accidentally run `mypy` using a version that doesn't match [the version we install for CI](https://github.com/pytorch/pytorch/blob/6045663f391e9bebac31e006a98c1dd381792936/.circleci/docker/common/install_conda.sh#L117), since this trips people up sometimes when `mypy` gives errors in some versions (see pytorch#51513) but not others. Pull Request resolved: pytorch#51799 Test Plan: To check that this doesn't break our `mypy` test(s) when you have the correct version installed: ``` python test/test_type_hints.py ``` To check that this does indeed warn when you have an incorrect `mypy` version installed, switch to a different version (e.g. 0.782), and run the above command or either of these: ``` mypy mypy --config-file=mypy-strict.ini ``` You should get the following message on stderr: ``` You are using mypy version 0.782, which is not supported in the PyTorch repo. Please switch to mypy version 0.770. For example, if you installed mypy via pip, run this: pip install mypy==0.770 Or if you installed mypy via conda, run this: conda install -c conda-forge mypy=0.770 ``` Reviewed By: janeyx99 Differential Revision: D26282010 Pulled By: samestep fbshipit-source-id: 7b423020d0529700dea8972b27afa2d7068e1b12
3 participants
This PR adds a local
mypyplugin that warns if you accidentally runmypyusing a version that doesn't match the version we install for CI, since this trips people up sometimes whenmypygives errors in some versions (see #51513) but not others.Test plan:
To check that this doesn't break our
mypytest(s) when you have the correct version installed:To check that this does indeed warn when you have an incorrect
mypyversion installed, switch to a different version (e.g. 0.782), and run the above command or either of these:You should get the following message on stderr: