For discussion: use black for code formatting

[levbishop]

Summary

Currently we rely on pycodestyle and pylint for nagging us to write well-styled code.
That's a bit annoying because it can take a couple iterations to get those tools happy (especially for inexperienced contributors) and also because pylint especially is really slow.

A good number of projects have switched over to automatic formatting using black

Good points of black:

1. Its really fast (see my benchmarking below). Like 600x faster than pylint. Really fast
2. You don't have to think - it just fixes your code for you (most editors -- eg vs code -- have a way to have it run as you type for maximum transparency)
3. The formatting choices black makes are set up to make code review easier with small diffs when your code changes in the future (eg always using trailing commas when breaking a list on multiple lines)
4. To my mind the resulting formatting is just as good looking as our current code resulting from battles between devs and the linters (see attached PR) and is a little more self-consistent in style
5. There are almost no configuration options for us to bikeshed about (unlike other auto formatters such as yapf).
6. Widely used (including by for example pylint, pytest, tox, requests, pipenv etc)

Minor downsides of black:

1. Black is not completely in agreement with other tools' opinions on proper style. See the black documentation about compatible configurations for details. I pushed small tweaks to our pylint and pycodestyle configs to make them happy
2. Black treats maximum-line-length "like a speed limit" ie if the least bad option is to exceed the line length by a little, then it will sometimes do so (I kept black using line length 100 and set the pylint/pycodestyle line length to 105 to accommodate this -- there are currently no places where this actually comes up in qiskit)
3. Black uses a little more whitespace for mathematical expressions than is used in some parts of qiskit, so overall running black adds a few lines to the codebase. I think the resulting code is plenty readable so don't see this as an issue.
4. Black only does formatting - we probably still want to run pylint to catch other issues. By disabling the code formatting checks in pylint, perhaps it can faster - i haven't explored this.

More major objections to black:

1. It will be a big churn over the codebase. You can tell git to ignore the churn and keep git blame relevant but currently the github web interface will be clogged up by the churn. Since this is one-time pain I think it's worth considering, but worth doing it carefully, probably just before tagging a stable release to minimize impact on back-porting etc.

Speed testing

On my macbook

pycodestyle 2.6.0
pylint 2.4.4
astroid 2.3.3
python 3.8.5
black 20.8b1

pycodestyle qiskit test: 34s
pylint --disable=all -rn qiskit test: 45s
pylint -rn qiskit test: 612s
black qiskit test: 415s (first run, rewriting 899 files)
black qiskit test: 1s! (up-to-date cache, no files to change)

This last black number is so good because black keeps a cache (on macos in ~/Library/Caches/black/ ) to shortcut reformatting unchanged files. So to get the time for running on a freshly cloned repo, I wiped the cache and got:
black qiskit test: 22s (empty cache, no files to change)

Even without the cache black is substantially faster than pycodestyle and around 25x faster than pylint.

See also

In #1179 there is a suggestion from @mtreinish to drop pylint completely. Linked to that discussion are numerous cases where pylint hinders arguably more than it helps. I don't have a strong feeling about whether there is value in some of the deeper analysis done by pylint (tracking unused variables etc), but I do argue that formatting issues are better handled by something like black so, if we keep using pylint we can reduce the number of checks it applies such that most pylint warnings would indicate meaningful issues actually worth fixing rather than working around/suppressing and ideally pylint would run a lot faster.

[mtreinish]

While I generally am a fan of automatic formatters like this (rustfmt for example is great) because it automates what can be a tedious task to apply uniform code style, I am personally not a fan of black. This primarily comes from what I view as weird choices around how they chose to format things (like long function signatures and similar long lists of things). Also, the fuzziness you mentioned around line length makes working on code formatted with black much harder for me (I use 80 character wide terminals, a small army of them, for doing most of my text editing). When I work on a repo that uses black I end up with this annoying flip flop between what my vim auto formatting configuration does as I write code, then what black changes to what it thinks is best and then oscillating between the two states depending whether I edited the file or ran black most recently.

As you pointed out I'm also not a fan of pylint it's super slow, overly pedantic, and super fragile in practice. In my opinion it does more harm than good for the project. But, it does give us a way to tweak which rules are enforced and how we want a lot of it's default behavior to work. For #1179 a lot of the frustration I was having was less an inherent limitation in pylint but more a poorly constructed set of rules (although the slowness and fragility are still there). This has gotten better as we've tweaked the config to make more sense. This is where I have real issues with black because it barely has any configurable options so we're pretty much stuck with the formatting it gives us whether we like it or not.

The other thing I would caution is avoid using too many tools for style, it can make working on a repository too finicky and annoying. I've contributed to a project where they used black, flake8, pylint, spell checkers, mypy, and some other tools and to get a proposed commit through CI style checkers was an exercise in itself (so much so that I haven't contributed in a long time). If we're going to pick black we should drop pycodestyle. If we're going to keep running pylint and we want to speed it up we probably should enable parallel execution again. I think it was turned off before because of incompatibilities with the copyright header plugin we used in the past, but that doesn't matter anymore because we don't use that plugin.

That all being said if the consensus is to use black, I'm fine with that. Especially because I think having an auto formatter will help our CI throughput. If you look at the job failure rates we waste a lot of electricity bouncing patches off the linter in CI: https://dev.azure.com/qiskit-ci/qiskit-terra/_pipeline/analytics/stageawareoutcome?definitionId=1&contextType=build which should be a no-brainer to check locally but currently isn't because of pylint. Which was my main issue that I opened #1179, I think in the 2.5 years I've been working on Qiskit there has only been a cumulative ~6 months where a full pylint run would work for me locally (I'm currently in a period where it doesn't work because python3.9 is the default version on my systems). So I think having a simple command to just fix things for us would hopefully make this better. Although I will say in my experience with retworkx I still forget to run cargo fmt about 20% of the time and still have linter failures in CI. I'm just not fan of the tool or the choices it makes and the inability to tune it to something that doesn't interfere with my personal development flow.

[levbishop]

If we're going to keep running pylint and we want to speed it up we probably should enable parallel execution again. I think it was turned off before because of incompatibilities with the copyright header plugin we used in the past, but that doesn't matter anymore because we don't use that plugin.

I think the problem with parallel pylint was that it stops pylint from noticing cyclic imports. Personally I'm inclined toward globally turning off cyclic import detection since historically our response to such warnings from pylint has usually been to suppress the warning rather than refactor the imports.... I don't recall that parallel pylint was very much faster than serial, however last time I checked.

Added:

See pylint-dev/pylint#374 where I think the pylint devs are saying that cyclic-import is the last remaining issue with parallel pylint.

A quick check shows about a 2.25x speedup for parallel pylint -j6 on my setup which is nice to have but still painfully slow.

[levbishop]

[...] I am personally not a fan of black. This primarily comes from what I view as weird choices around how they chose to format things (like long function signatures and similar long lists of things).

Can you highlight a couple of examples in the attached diff of places where you think black makes a bad choice?

[...] Also, the fuzziness you mentioned around line length makes working on code formatted with black much harder for me (I use 80 character wide terminals, a small army of them, for doing most of my text editing). When I work on a repo that uses black I end up with this annoying flip flop between what my vim auto formatting configuration does as I write code, then what black changes to what it thinks is best and then oscillating between the two states depending whether I edited the file or ran black most recently.

I think you're overstating this factor: although in principle black can be fuzzy with line lengths, for the current state of the qiskit repo at our chosen max-length of 100, black does not in fact generate a line longer than the max 100col. It seems like you would prefer changing max-line-length shorter than 100 and were we to go that way I guess making that jump at the same time as turning on black would minimize code churn (although black will not re-wrap docstrings and comment lines, so we would need another tool or a lot of manual work to migrate fully to an 80col width). Personally I think 100col is about the right width (Linus Torvalds rant to similar effect)

[...] This is where I have real issues with black because it barely has any configurable options so we're pretty much stuck with the formatting it gives us whether we like it or not.

You can # fmt: off completely disable formatting # fmt: on a section that needs special treatment by hand formatting. Otherwise, yes, you have to learn to like the black style, but it seems a fine style to me.

[...] I would caution is avoid using too many tools for style, it can make working on a repository too finicky and annoying. I've contributed to a project where they used black, flake8, pylint, spell checkers, mypy, and some other tools and to get a proposed commit through CI style checkers was an exercise in itself (so much so that I haven't contributed in a long time). If we're going to pick black we should drop pycodestyle.

Agree that we should consider the contributor experience. I've already heard our current setup is offputting and one reason to go to black is to make it less painful. I would say that its a good idea to provide configurations for as many tools as might be useful: e.g., provide local word lists for spellchecking,
making reasonable changes to keep those tools happy enough that they can be useful if anyone plans to run them, but otherwise I do agree on keeping only a small set of tools for CI. If we go to black, then the pycodestyle config can remain, and we should accept patches to make pycodestyle pass in the (unlikely) event that the blackened code happens to fail pycodestye (whether those are formatting tweaks or config updates) but absolutely we should drop pycodestyle from the CI.

[...] in my experience with retworkx I still forget to run cargo fmt about 20% of the time and still have linter failures in CI.

Black is so fast its totally practical to run it in a git pre-commit hook and then you'll never forget to run it :)

[mtreinish]

[...] I am personally not a fan of black. This primarily comes from what I view as weird choices around how they chose to format things (like long function signatures and similar long lists of things).

Can you highlight a couple of examples in the attached diff of places where you think black makes a bad choice?

Not a bad choice per say, but just a weird choice that I wouldn't have made; things like:

https://github.com/Qiskit/qiskit-terra/pull/5883/files#diff-4fb31a3ade5ae57cfd91ea00dbf3c5b6ab066a8234a742d91f9c09a09edca2f7R516-R524 (dagcircuit.py if the links don't work)

which I personally find a bit weird and awkward in python, but that's likely just my trained behavior of what python code looks like. It's more coupled with my comment below in that I have my vim workflow optimized to handle trivial formatting for me in a style like:

https://github.com/Qiskit/qiskit-terra/pull/5883/files#diff-f8bf228c13afac54d0487df4391520ab5b0efbe5649e447475d99dd04ce06c89L482-L484 (quantumcircuit.py if the links don't work)

which is valid pep8 too but different from black. This just means if we do adapt black I'll have to rewrite my editor configs for terra to use black's style instead.

[...] Also, the fuzziness you mentioned around line length makes working on code formatted with black much harder for me (I use 80 character wide terminals, a small army of them, for doing most of my text editing). When I work on a repo that uses black I end up with this annoying flip flop between what my vim auto formatting configuration does as I write code, then what black changes to what it thinks is best and then oscillating between the two states depending whether I edited the file or ran black most recently

I think you're overstating this factor: although in principle black can be fuzzy with line lengths, for the current state of the qiskit repo at our chosen max-length of 100, black does not in fact generate a line longer than the max 100col. It seems like you would prefer changing max-line-length shorter than 100 and were we to go that way I guess making that jump at the same time as turning on black would minimize code churn (although black will not re-wrap docstrings and comment lines, so we would need another tool or a lot of manual work to migrate fully to an 80col width). Personally I think 100col is about the right width (Linus Torvalds rant to similar effect)

Oh, I wasn't actually advocating for 80 character line limits. I agree with Torvalds there and don't think my personal preferences need to make life harder for people who don't have the same workflow as me (which I'm definitely the minority use case). I was just pointing out that it's something that makes my life a little harder not something we should really change. I was more talking about my other auto formatting rules differ from blacks style not specifically the line length thing (I should have split it up a bit more)

[...] I would caution is avoid using too many tools for style, it can make working on a repository too finicky and annoying. I've contributed to a project where they used black, flake8, pylint, spell checkers, mypy, and some other tools and to get a proposed commit through CI style checkers was an exercise in itself (so much so that I haven't contributed in a long time). If we're going to pick black we should drop pycodestyle.

Agree that we should consider the contributor experience. I've already heard our current setup is offputting and one reason to go to black is to make it less painful. I would say that its a good idea to provide configurations for as many tools as might be useful: e.g., provide local word lists for spellchecking,
making reasonable changes to keep those tools happy enough that they can be useful if anyone plans to run them, but otherwise I do agree on keeping only a small set of tools for CI. If we go to black, then the pycodestyle config can remain, and we should accept patches to make pycodestyle pass in the (unlikely) event that the blackened code happens to fail pycodestye (whether those are formatting tweaks or config updates) but absolutely we should drop pycodestyle from the CI.

+1

[jlapeyre]

I pulled this branch locally. How can I run black locally ?

EDIT: I installed black and ran 'black qiskit'.
If I understand correctly, there are some other tools that you propose using together with black. Is
there a command or commands to run all of them ?

[Cryoris]

My two cents: I think it would be nice as it simplifies contribution as developers don't have to spend a lot of time linting 👍🏻

[levbishop]

If I understand correctly, there are some other tools that you propose using together with black. Is
there a command or commands to run all of them ?

I set it up so that on the black tree you should be able to run:

$ pycodestyle qiskit test # included in qiskit CI
$ pylint -rn qiskit test # included in qiskit CI 
$ flake8 qiskit test # not currently part of qiskit CI, generates a few warnings

There are a whole slew of other checkers we can consider: mypy, pylama, pydocstyle, bandit, dodgy, prospector, vulture, and many more. Several of these just repackage other tools under a common interface to avoid the "too many checkers" problem. I don't have a concrete proposal for what combination of tools goes best with black, but I guess that should be the next step. I think it's clear that pycodestyle is redundant. mypy seems popular, is fast to run (20s same macbook as above) and very orthogonal to the formatting issues that black cares about, but I guess it only helps to the extent we are willing to spread the codebase with type annotations.

[Cryoris]

We used MyPy in Aqua and to be honest, to me, it caused more trouble than help. E.g. it often didn't properly resolve types if they weren't straightforward forcing us to add a lot of comments to ignore typechecks.

[manoelmarques]

Mypy is not perfect specially in opflow that has complicated interconnections. However I am a fan of type hints for documentation and browsing the code and if there is a better tool to validate type hints, I am all for it. In the absence of some type hint verification tool, I have seen code with all kinds of type hint discrepancies: 'int' instead of 'bool', List where should be a Dict etc. Type hints get stale and not everybody updates them properly and it is hard for a PR reviewer to check on all those things without a tool. Also I have seen mypy and pylint catch a few bugs before they happened.

[mtreinish]

I'm going to close this as it's been superseded by #6361 (the outcome of the discussion here is that we're using black which that PR migrates us to)