Upgrade flake8 to 6.0.0 #7222
Upgrade flake8 to 6.0.0 #7222
Conversation
|
Hi @mpearce25! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Hey @mpearce25 and thanks for the PR. We are currently on a very tight schedule for some stuff that we need to finish until end of week for the next release. I'll come back to this next week. In case I forget, feel free to ping me. |
|
Have we considered using lintrunner and ruff in this project? |
why not to move to ruff? https://github.com/charliermarsh/ruff |
Yes, but it is not a priority right now. There is a push from Nova to unite the CI system across all domain libraries, but linting is at the bottom of their list.
Because that costs time that we currently don't have. |
There was a problem hiding this comment.
Thanks @mpearce25 for the PR! I have a small comment inline. Otherwise LGTM if CI is green.
CONTRIBUTING.md
Outdated
| @@ -83,7 +83,7 @@ Instead of relying directly on `black` however, we rely on | |||
| [ufmt](https://github.com/omnilib/ufmt), for compatibility reasons with Facebook | |||
| internal infrastructure. | |||
|
|
|||
| To format your code, install `ufmt` with `pip install ufmt==1.3.2 black==22.3.0 usort==1.0.2` and use e.g.: | |||
| To format your code, install `ufmt` with `pip install ufmt==1.3.2 black==22.3.0 usort==1.0.2 flake8=6.0.0` and use e.g.: | |||
There was a problem hiding this comment.
Since this section is about formatting, can we not include flake8 here, but rather have a separate installation command below where flake8 usage is explained? Alternatively, we could also rephrase this to not start with
To format your code, install
ufmt
There was a problem hiding this comment.
I split up the formatting and linting sections.
Also promoted the "typing" section to same level as formatting/linting and unit tests. Mypy operates independently from fromatting/linting and I wanted to avoid confusion in the pre-commit hooks section.
lmk if these changes look good.
CI is failing same as other PRs. Issues seem unrelated, but I'm fine waiting until everything is green again to avoid compounding any CI issues.
There was a problem hiding this comment.
CI is failing same as other PRs. Issues seem unrelated, but I'm fine waiting until everything is green again to avoid compounding any CI issues.
Just as a heads up, this will take some time. See #7299.
There was a problem hiding this comment.
The previous issue that led to these type hints being removed is partially hidden behind internal meta tools, but seems to basically be https://github.com/pytorch/vision/pull/4631/files#r744161323 .
Imo it would be odd to sacrifice typing in torchvision just to avoid adding some type ignore or assert in the 3rd party code. However, lmk if that's not the case.
Addresses: #7200
Changes
Typing updates
For
torchvision/models/detection/generalized_rcnn.pytype hints were:Testing