Update to isort 5.7.0 #187
Conversation
|
Hi @akainth015! 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 cla@fb.com. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
thank you @anankan , can you click on "Import to Phabricator" ? we will need to run some more tests internally and ensure the isort change is aligned with the linters used in our fb development. Thank you so much for this PR! :) |
setup.cfg
Outdated
| @@ -9,5 +9,4 @@ line_length = 88 | |||
| multi_line_output = 3 | |||
| use_parentheses = True | |||
| lines_after_imports = 2 | |||
| known_third_party = PIL,bs4,classy_vision,cv2,detectron2,fairscale,faiss,fvcore,hydra,mock,nbconvert,nbformat,numpy,omegaconf,parameterized,pkg_resources,pycocotools,recommonmark,run_distributed_engines,scipy,setuptools,sklearn,sphinx_rtd_theme,tabulate,torch,torchvision,tqdm,utils | |||
There was a problem hiding this comment.
is this change enforced by isort 5.7?
There was a problem hiding this comment.
I checked it out and it turns out it is not, I had just assumed (incorrectly) that since seed-isort-config was unnecessary that the option would be deprecated.
| @@ -1,7 +1,8 @@ | |||
| import unittest | |||
|
|
|||
| import torch | |||
| from vissl.models.heads import LinearEvalMLP, MLP | |||
|
|
|||
There was a problem hiding this comment.
I am curious about the additional lines. Do you know isort is enforcing this?
There was a problem hiding this comment.
After your comment above, I figured it may have been influenced by my removing the known_third_party field. So I reverted the commit 22cf0d4 that applied the changes to other files and reintroduced the upgrade with the exception of removing known_third_party. These new lines were still applied to the original codebase, so it seems it is indeed something isort is enforcing.
Since the history with the reverted commit would just reintroduce the changes, I have instead pushed a simple new commit to restore the original setup.cfg, since it appears to have no effect on the sorting. I now remember that I reasoned that since known_third_party was generated, and that since we were no longer generating anything to update it, that I would remove it from the codebase to avoid a stagnant line.
I think you tagged the wrong person 😛, but I don't see an "Import to Phabricator" button anywhere on this pull request page. |
|
@akainth015 , let's open the PR in the MLH fork directly and we will look into it there. |
Summary: Integration of XCiT into VISSL :) Pull Request resolved: fairinternal/ssl_scaling#187 Reviewed By: iseessel Differential Revision: D31378757 Pulled By: prigoyal fbshipit-source-id: d383390a3ae4def585eaf7bef6d743e9c5c059e8
Our MLH team had issues committing files using the existing pre-commit hooks so I updated to
isort==5.7.0, which no longer requiresseed-isort-configand provides its own pre-commit hook. Once modified, the changes were run throughdev/linter.shwhich updated many of the older files that had been silently breaking the rules enforced for new files.The most pertinent changes between
isort4 and 5 are listed below:-sphave been renamed to--spto be less ambiguousisortwill not do anything by default. This has been remedied by specifying that it should check"${DIR}"indev/linter.shAfter merging, contributors will need to reinstall the pre-commit hooks via
pre-commit install.