Use black?

[NicolasHug]

Formatting the codebase with black would help in a few ways:

• avoid review nitpicks regarding style
• make signatures easier to read (when type annotations are involved, things can get really ugly)
• avoid PRs where contributors IDE automatically re-format the imports etc, and we always have to tell them to revert such unrelated changes
• help avoiding the CI linting issues
• overall style consistency

Cons:

• Adds noise to git blame, namely 1 extra click. This is for GitHub only, locally nothing is changed (see below)
• Adds a lot of vertical length to the code. It can be a bit unsettling/ugly at first, but you get used to it
• a bit more setup/work for new contributors, but black is becoming more and more of a standard anyway so people are mostly used to it, and pre-commit hooks are very easy to setup.
• will create conflicts with ongoing PRs. Not really a big deal, there aren't too many PRs and resolving conflicts should be fairly simple
• As discovered with @pmeier, this forces to add a new --no-build-isolation to all pip install --editable commands [1], as done in 3d1cd1d. This isn't a big deal either as long as we provide good guidelines, and we recommend conda over pip anyway

Using black has already been discussed in #2025, with general positive sentiment towards adoption. The main concern seems to be that black brakes git blame, but it's actually not the case: we can avoid breaking git glame thanks to the ignore-revs-file option (unfortunately, GitHub doesn't support it yet, so the Github blame may still be a bit noisy).

[1] The reason for this nonsense is that black relies (only) on a pyproject.toml file for its configuration. This file was initially intended to declare build-time dependencies (something completely unrelated to code formatting, but oh well), and it can make pip behave differently in editable mode psf/black#683 (comment), as illustrated in https://app.circleci.com/pipelines/github/pytorch/vision/9457/workflows/4007ced4-684c-498d-8c78-f6ee83853a7d/jobs/697466 . We need the --no-build-isolation flag to preserve pip's sane behaviour, now that this file is added.

---

Adopting black would involve:

• a black-formatting PR (duh). Might consider make the PR author a bot or some dummy account.
• A PR that adds the ignore-revs-file file, referencing the previous PR for git-blame to be preserved.
• some instructions in the contributing guide to use black, edit the "install from source" docs with pip install -e . to use the --no-build-isolation flag, and optionally setup a pre-commit hook as done e.g. in pandas
• some instructions e.g. as a sticky issue to resolve potential merge conflicts
• a new CI check. This could just be part of the current job that checks for flake8.

I'm happy to work on this if we agree to rely on black from now on.

CC @fmassa @pmeier

[pmeier]

For me the only con is the noise on GitHub blame. I imagine a lot of projects are hesitant to add it due to this very reason. Unfortunately, GH does not seem to see it as a feature to add something like this. Is FB a GitHub pro user? If yes, maybe we can build some pressure there?

I could live with the additional noise if we keep it to a single commit. If we do this we should at the same time look into some more auto formatting like isort and flynt to avoid having multiple refactor commits that need to manually ignored in blame view.

Might consider make the PR author a bot or some dummy account.

I don't understand. Why can't we merge this the normal way?

[NicolasHug]

I could live with the additional noise if we keep it to a single commit

Same here

some more auto formatting like isort and flynt

Good point, I think it would be OK to add isort as part of the pre-commit hook as it integrates fine with black (and they're both part of the internal fb linting FWIW). For flynt we can keep this as a one-time thing in the formatting PR.

Why can't we merge this the normal way?

I'd just like to keep my contribution history clean, ideally :)

[pmeier]

I'd just like to keep my contribution history clean, ideally :)

If that is the only reason, I'm happy to create the PR. No need to bother setting up a new account or bot for this.

[pmeier]

@NicolasHug

The reason for this nonsense is that black relies (only) on a pyproject.toml file for its configuration.

This seems not to be true. From the same thread:

You can already run black --config black.toml today if this really is blocking you

In the proof-of-concept PR #4178 this is working well.

[NicolasHug]

I'd rather not though, specifying a --config file all the time black is invoked is going to be annoying, even with pre-commit hooks. We need to be able to use black ., not black --config black.toml .

[pmeier]

I'd rather not though, specifying a --config file all the time black is invoked is going to be annoying, even with pre-commit hooks.

If we have pre-commit hooks, why do you want to call black manually?

We need to be able to use black ., not black --config black.toml .

I agree that would be preferred, but compared to --no-build-isolation I think it is the better choice. Note that we also need to do flake8 --config=setup.cfg . instead of flake8 ..

[NicolasHug]

If we have pre-commit hooks, why do you want to call black manually?

Because sometimes one may want to apply black without committing. I also wonder how easy/painful it will be to configure IDEs to properly look for the non-default black.toml file.

Note that we also need to do flake8 --config=setup.cfg . instead of flake8 ..

Yup, that's one of the reason I never use it locally ^^

compared to --no-build-isolation I think it is the better choice

Very few people will be using pip install --editable, it's not even something we recommend: we recommend conda and python setup.py develop, so this is affecting much less users.

[NicolasHug]

There's another thing to consider that @mthrok pointed out (thanks Moto!): we'll need to keep GH and fbcode in sync regarding the linting.

We don't currently lint torchvision on fbcode, but if we start linting on GH we'll have to lint on fbcode as well. Otherwise, things will get nasty when people submit (unlinted) code from withing fbcode: CI will complain when we import those changes on GH. We could apply the linting while importing to GH, but I'm pretty sure we would need to re-export those linting changes to fbcode to avoid conflicts.

The internal fb linter relies on https://github.com/omnilib/ufmt, so we'll have to use that instead of individually apply black and isort (which should actually be μsort now), and we'll need to keep the pinned down version synced.

I'll make some tests next week to make sure this will be working fine, find out which versions we need to pin down, and that we can still make this part of the pre-commit hooks etc. Thanks a lot @pmeier for your efforts so far!

[NicolasHug]

OK, so I tried internally and applying pyfmt is working fine and is consistent with the internal fb linter.

For it to be fully compliant we'll need to add

[tool.usort]
first_party_detection = false

in the pyproject.toml.

Also, we need to pin down the dependencies as such:

➜  ~ pyfmt -V
fb-pyfmt built mode/opt (fastzip) on Python 3.8.3 (macosx-x86_64_minimal_xcode)
black version 21.4b2
usort version 0.6.4
libcst version 0.3.19

(This command can be run from any fb machine, whether it's a laptop or a devvm).

[pmeier]

I'll update the PR accordingly.