add `ufmt` as code formatter #4384

pmeier · 2021-09-08T15:52:57Z

Supersedes #4178. Instead of using isort and black individually, ufmt uses usort and black in an atomic way.

To keep this PR reviewable, I'm only going to pull the trigger on all formatting changes if the PR is approved otherwise.

This PR is blocked for now until ufmt>=1.3 is released.

NicolasHug

Sweet, thanks @pmeier ! I made a quick pass, the strategy sounds good.

Do we know when the ufmt release will be out?

pyproject.toml

NicolasHug · 2021-09-08T16:04:11Z

torchvision/ops/poolers.py

@@ -2,7 +2,7 @@
 from torch import nn, Tensor

 import torchvision
-from torchvision.ops import roi_align
+from .roi_align import roi_align


just curious, was this flagged somehow?

See #4178 (comment). In short: we relied on a specific order of the imports due to a naming conflict. Without this fix we need to exclude these imports from being sorted.

There might have been a reason for that initially, but I'm wondering why we even use absolute imports within the torchvision package. Every torchvision improt should be relative there. Perhaps @fmassa knows?

(anyway, this is for another PR, let's leave it as is :) )

Perhaps because absolute import is recommended by PEP8?

pyproject.toml

pmeier

The lint workflow is failing, because the version of ufmt that we need. Otherwise this should be good to go.

@NicolasHug: in #4178 we talked about adding pre-commit hooks for this. I have a PR up that adds this to ufmt: omnilib/ufmt#15. Since they are not necessary to run ufmt, IMO we can add them in the future.

pyproject.toml

pmeier · 2021-09-09T06:36:32Z

pyproject.toml

+local_utils = [
+    "_utils_internal",
+    "common_utils",
+    "dataset_utils",
+]


These are local utilities in our test/ folder. Since its not a package, i.e. tests/__init__.py is missing, usort does not pick up on the fact that these are local dependencies and sorts them into the third party category.

Let's add this comment in the file to provide context for future us :)

Also to avoid conflicting it with other utils, can we change the name to local_test_utils? Alternatively we could just add these into the "first-party" category?

Alternatively we could just add these into the "first-party" category?

We could, but if you don't have a strong opinion about this, I would leave them in an extra category. IMO, first party and local should be separated, because the latter indicates that this cannot be import the "normal" way. In fact, isort has a section for local imports where all the relative imports go. Unfortunately, usort does not have that. I've asked about it in facebook/usort#30.

pmeier · 2021-09-09T06:37:51Z

setup.cfg

+ignore = E203, E402, W503, W504, F821
+per-file-ignores =
+    __init__.py: F401, F403, F405
+    ./hubconf.py: F401
+    torchvision/models/mobilenet.py: F401, F403
+    torchvision/models/quantization/mobilenet.py: F401, F403
+    test/smoke_test.py: F401


See #4178 (comment) for details on this change.

NicolasHug

Thanks @pmeier

@NicolasHug: in #4178 we talked about adding pre-commit hooks for this. I have a PR up that adds this to ufmt: omnilib/ufmt#15. Since they are not necessary to run ufmt, IMO we can add them in the future.

I'm a bit confused, do we need pre-commit hooks in the ufmt repo in order to have pre-commit hooks in torchvision?

NicolasHug · 2021-09-09T07:27:08Z

pyproject.toml

+local_utils = [
+    "_utils_internal",
+    "common_utils",
+    "dataset_utils",
+]


Let's add this comment in the file to provide context for future us :)

Also to avoid conflicting it with other utils, can we change the name to local_test_utils? Alternatively we could just add these into the "first-party" category?

NicolasHug · 2021-09-09T07:33:03Z

torchvision/ops/poolers.py

@@ -2,7 +2,7 @@
 from torch import nn, Tensor

 import torchvision
-from torchvision.ops import roi_align
+from .roi_align import roi_align


There might have been a reason for that initially, but I'm wondering why we even use absolute imports within the torchvision package. Every torchvision improt should be relative there. Perhaps @fmassa knows?

(anyway, this is for another PR, let's leave it as is :) )

pyproject.toml

pmeier · 2021-09-09T07:55:56Z

I'm a bit confused, do we need pre-commit hooks in the ufmt repo in order to have pre-commit hooks in torchvision?

Just to be sure: we are talking about hooks added by the pre-commit framework to add these hooks and not provide a homebrew solution, right?

If yes, we don't need pre-commit hooks in the ufmt config, but a configuration telling pre-commit how to use the ufmt repository. The confusing thing about this is probably the file names:

.pre-commit-config.yaml defines what hooks should be run before every commit (this is the file we need)
.pre-commit-hooks.yaml defines what hooks the repository provides in case it is used by pre-commit (this is the file ufmt needs)

NicolasHug · 2021-09-09T08:05:52Z

Oh OK, thanks. I didn't know the "upstream" repo (ufmt) had to be configured as well for commit hooks to work.

I feel like we might want to wait for commit hooks to be available though. It will probably make the transition smoother for most contributors / developers, and we will just be able to run pre-commit run --all-files to make sure to avoid any potential future change

pmeier · 2021-09-10T05:59:58Z

@NicolasHug I've added support for pre-commit hooks in #4387. If that is landed and ufmt supports being used as one, we only need to add a few lines to the .pre-commit-config.yaml file.

pmeier · 2021-09-15T09:36:15Z

Blocked by omnilib/ufmt#13 and omnilib/ufmt#15. After these PRs are in we also need to wait for the next release, which I'm hoping will be quick, since we added some important functionality.

pyproject.toml

This reverts commit f2e224c.

pmeier · 2021-09-18T13:27:10Z

The PRs I sent to ufmt were landed so this PR is almost ready to go. As soon as ufmt has its next release (@jreese is there a timeline for that?) I'll push the changes. I have them locally by running ufmt's main branch as reference.

Since this PR will touch (almost?) any *.py file in our repository, we will probably get merge conflicts for every open PR. I'll comment an upgrade guide, so we can point contributors here. cc @fmassa @datumbox @NicolasHug @prabhat00155

pmeier · 2021-09-18T13:38:03Z

If you are reading this, you were probably prompted to fix the code format of your PR. Here is what you do:

$ git checkout main
$ git pull
$ git checkout $MY_FEATURE_BRANCH
$ git merge -X ours main
$ pre-commit run --all-files
$ git commit -am "fix code format"
$ git push

If you don't have pre-commit installed, you can install it with pip install pre-commit or conda install -c conda-forge pre-commit. To avoid running this manually in the future, you can pre-commit install some hooks that will be run everytime you git commit something in this repository.

prabhat00155 · 2021-10-04T10:10:31Z

.circleci/regenerate.py



 PYTHON_VERSIONS = ["3.6", "3.7", "3.8", "3.9"]

 RC_PATTERN = r"/v[0-9]+(\.[0-9]+)*-rc[0-9]+/"


-def build_workflows(prefix='', filter_branch=None, upload=False, indentation=6, windows_latest_only=False):
+def build_workflows(prefix="", filter_branch=None, upload=False, indentation=6, windows_latest_only=False):


Is the a way to configure ufmt to avoid making this ' -> " change?

I don't think so, using double quotes is black's convention

black is the tool doing this. If we don't want it, we could put a skip_string_normalization = true line into its section in the pyproject.toml.

I feel this is adding a number of unnecessary changes, given pep8 doesn't recommend one over the other.

Unfortunately this isn't something we can change, as the internal arc lint would force these back to double quotes

I thought we were not using the default internal linter. Can't we configure that the same way as pointed above by Philip?

I don't think it's possible, but even if it were I wouldn't recommend it. It's better to stick to the usual convention as much as possible. The only one we don't properly respect is the line length, for historical reasons. Are you mostly concerned about git blame noise?

I have the following concerns here:

Unnecessary changes.

I am not convinced with the idea of forcing " over '. This should be a user choice given that pep8 doesn't recommend either. Also, this is not the usual convention. Line length is something recommend by pep8.

Python by default uses ' for representing string:

>>> a = "abc" >>> a 'abc'

' uses 1 key stroke against 2 for ".

regarding 4, you're free to use single quotes if you want and let black automatically format them :)

For the rest, I understand your point, but one of the goal of using black is precisely to avoid that kind of discussion about style preferences. It may not be perfect and suit everyone's taste (it doesn't suit mine either), but at least it's a standard that we can all automatically comply to.

.circleci/unittest/linux/scripts/run-clang-format.py

NicolasHug · 2021-10-04T13:16:02Z

Sync is looking good now (it should look like D31379666 and the few diffs below).

I'll merge this when green. We can address the flake8 issues later, they're almost all unused imports

NicolasHug · 2021-10-04T13:58:14Z

Tests are green. Let's get this in.
🥵
May the syncing gods be with me

github-actions · 2021-10-04T13:59:08Z

Hey @NicolasHug!

You merged this PR, but no labels were added.

NicolasHug · 2021-10-04T13:59:44Z

A million thanks to you @pmeier for your help

Summary: * add ufmt as code formatter * cleanup * quote ufmt requirement * split imports into more groups * regenerate circleci config * fix CI * clarify local testing utils section * use ufmt pre-commit hook * split relative imports into local category * Revert "split relative imports into local category" This reverts commit f2e224c. * pin black and usort dependencies * fix local test utils detection * fix ufmt rev * add reference utils to local category * fix usort config * remove custom categories sorting * Run pre-commit without fixing flake8 * got a double import in merge Reviewed By: kazhang Differential Revision: D31380329 fbshipit-source-id: c1ee3a57589401d88877201fbf40cc968681b640 Co-authored-by: Nicolas Hug <nicolashug@fb.com>

* add ufmt as code formatter * cleanup * quote ufmt requirement * split imports into more groups * regenerate circleci config * fix CI * clarify local testing utils section * use ufmt pre-commit hook * split relative imports into local category * Revert "split relative imports into local category" This reverts commit f2e224c. * pin black and usort dependencies * fix local test utils detection * fix ufmt rev * add reference utils to local category * fix usort config * remove custom categories sorting * Run pre-commit without fixing flake8 * got a double import in merge Co-authored-by: Nicolas Hug <nicolashug@fb.com>

add ufmt as code formatter

ce040e7

pmeier requested a review from NicolasHug September 8, 2021 15:52

facebook-github-bot added the cla signed label Sep 8, 2021

pmeier mentioned this pull request Sep 8, 2021

[DO NOT MERGE] introduce isort and black as autoformatters #4178

Closed

2 tasks

pmeier linked an issue Sep 8, 2021 that may be closed by this pull request

Use black? #4174

Closed

pmeier added 2 commits September 8, 2021 17:55

cleanup

a2ea263

quote ufmt requirement

a1649e8

NicolasHug reviewed Sep 8, 2021

View reviewed changes

pmeier added 4 commits September 9, 2021 07:55

split imports into more groups

713377d

Merge branch 'main' into ufmt

837b07b

regenerate circleci config

333fb87

fix CI

10652cc

pmeier commented Sep 9, 2021

View reviewed changes

pmeier marked this pull request as ready for review September 9, 2021 06:40

NicolasHug reviewed Sep 9, 2021

View reviewed changes

clarify local testing utils section

6dcfd59

pmeier mentioned this pull request Sep 10, 2021

add pre-commit hooks for convenient formatting checks #4387

Merged

pmeier added 2 commits September 13, 2021 14:42

Merge branch 'main' into ufmt

d91665c

use ufmt pre-commit hook

705008c

split relative imports into local category

f2e224c

pmeier commented Sep 15, 2021

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

pmeier added 3 commits September 15, 2021 12:25

Revert "split relative imports into local category"

c0624f7

This reverts commit f2e224c.

pin black and usort dependencies

3e140d6

fix local test utils detection

7ed911a

Merge branch 'main' of github.com:pytorch/vision into ufmt

c27c869

NicolasHug force-pushed the ufmt branch from b92f291 to c27c869 Compare October 1, 2021 08:56

NicolasHug mentioned this pull request Oct 1, 2021

NORMG Ufmt without custom sorting categories #4523

Closed

zhiqwang mentioned this pull request Oct 1, 2021

Add pre-commit hooks zhiqwang/yolort#182

Merged

NicolasHug added 2 commits October 4, 2021 09:28

Merge branch 'main' of github.com:pytorch/vision into ufmt

6811faf

remove custom categories sorting

60fdfda

pytorch-probot bot added the ciflow/default label Oct 4, 2021

prabhat00155 reviewed Oct 4, 2021

View reviewed changes

NicolasHug mentioned this pull request Oct 4, 2021

Relative import for roi_align #4533

Merged

Run pre-commit without fixing flake8

bd91974

NicolasHug force-pushed the ufmt branch from 6a7d39a to bd91974 Compare October 4, 2021 10:51

NicolasHug added 2 commits October 4, 2021 13:25

Merge branch 'main' of github.com:pytorch/vision into ufmt

841d5c9

got a double import in merge

0dc71f4

NicolasHug merged commit 5f0edb9 into pytorch:main Oct 4, 2021

NicolasHug added the code quality label Oct 4, 2021

pmeier deleted the ufmt branch October 4, 2021 14:33

NicolasHug mentioned this pull request Oct 4, 2021

Fix all flake8 issues #4535

Merged

This was referenced Oct 5, 2021

fix import order for torchvision.prototype.datasets #4538

Merged

Resolving merge conflicts for PRs opened before Oct' 4 #4539

Open

xwang233 mentioned this pull request Oct 5, 2021

No longer able to build from source with pip install -v . #4542

Open

amyreese mentioned this pull request Feb 12, 2022

Switch formatting from black+isort to µfmt (black+µsort) pytorch/ignite#2460

Merged

3 tasks

pmeier mentioned this pull request Mar 2, 2022

Reduce unnecessary cuda sync in anchor_utils.py #5515

Merged

malfet mentioned this pull request Jul 1, 2022

vision hash update failing pytorch/pytorch#80745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `ufmt` as code formatter #4384

add `ufmt` as code formatter #4384

pmeier commented Sep 8, 2021

NicolasHug left a comment

NicolasHug Sep 8, 2021

pmeier Sep 9, 2021

NicolasHug Sep 9, 2021

kazhang Oct 6, 2021

pmeier left a comment

pmeier Sep 9, 2021

NicolasHug Sep 9, 2021

pmeier Sep 9, 2021

pmeier Sep 9, 2021

NicolasHug left a comment

NicolasHug Sep 9, 2021

NicolasHug Sep 9, 2021

pmeier commented Sep 9, 2021

NicolasHug commented Sep 9, 2021

pmeier commented Sep 10, 2021

pmeier commented Sep 15, 2021

pmeier commented Sep 18, 2021 •

edited

Loading

pmeier commented Sep 18, 2021

prabhat00155 Oct 4, 2021

NicolasHug Oct 4, 2021

pmeier Oct 4, 2021 •

edited

Loading

prabhat00155 Oct 4, 2021

NicolasHug Oct 4, 2021

prabhat00155 Oct 4, 2021

NicolasHug Oct 4, 2021

prabhat00155 Oct 4, 2021 •

edited

Loading

NicolasHug Oct 4, 2021

NicolasHug commented Oct 4, 2021

NicolasHug commented Oct 4, 2021

github-actions bot commented Oct 4, 2021

NicolasHug commented Oct 4, 2021

add ufmt as code formatter #4384

add ufmt as code formatter #4384

Conversation

pmeier commented Sep 8, 2021

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmeier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmeier commented Sep 9, 2021

NicolasHug commented Sep 9, 2021

pmeier commented Sep 10, 2021

pmeier commented Sep 15, 2021

pmeier commented Sep 18, 2021 • edited Loading

pmeier commented Sep 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmeier Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prabhat00155 Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasHug commented Oct 4, 2021

NicolasHug commented Oct 4, 2021

github-actions bot commented Oct 4, 2021

NicolasHug commented Oct 4, 2021

add `ufmt` as code formatter #4384

add `ufmt` as code formatter #4384

pmeier commented Sep 18, 2021 •

edited

Loading

pmeier Oct 4, 2021 •

edited

Loading

prabhat00155 Oct 4, 2021 •

edited

Loading