[MLH] Pre-commit Hooks Configuration Changes #283
Conversation
grace-omotoso
commented
Apr 8, 2021
grace-omotoso
commented
- Update pre-commit steps and isort configuration
- Update CI test command
facebook-github-bot
added
the
CLA Signed
|
@prigoyal has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
tools/train_svm_low_shot.py
Outdated
| @@ -6,23 +6,19 @@ | |||
| from argparse import Namespace | |||
| from typing import Any, List | |||
|
|
|||
| from hydra.experimental import compose, initialize_config_module | |||
There was a problem hiding this comment.
@akainth015 , @grace-omotoso , do we know what triggered this change? it doesn't seem aligned with the linter to me.
There was a problem hiding this comment.
Seems correct to me, moved from line 13 to up here, probably because hydra is considered 3rd party
There was a problem hiding this comment.
gotcha, thank you for clarifying. This currently conflicts with the linter we have at facebook which expects the alphabetical ordering. Is it possible to address this?
There was a problem hiding this comment.
Hm, does that mean alphabetical within the same block of imports? We could remove the no_lines_before = FIRSTPARTY directive, perhaps that would solve the issue? Although it would result in a lot of changes to existing files.
There was a problem hiding this comment.
Hi @akainth015 I am new to the vissl team at FB. This PR looks great -- it will be a huge help to us!!
I think this may have been caused by removing the known_third_party above?
There was a problem hiding this comment.
Hi @iseessel, nice to meet you! Glad that this will be useful
We could add extra_scripts to the known_third_party option; that would address the issue. Semantically, is that acceptable?
| @@ -9,5 +9,6 @@ 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 it possible to revert this?
There was a problem hiding this comment.
Yes, but since that was generated by seed-isort-config it seemed better to remove it, as directed by the isort documentation.
| @@ -9,5 +9,6 @@ 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 | |||
| ensure_newline_before_comments = True | |||
| no_lines_before = FIRSTPARTY | |||
There was a problem hiding this comment.
I would recommend to not introduce the new rule in this PR.
There was a problem hiding this comment.
I added these rules to minimize the effect the upgrade had on the existing files.
|
thank you! I'll patch this PR today and test it and will aim to land it/merge it. |
|
@grace-omotoso has updated the pull request. You must reimport the pull request before landing. |
|
@grace-omotoso has updated the pull request. You must reimport the pull request before landing. |
|
@prigoyal has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
