RFC: Document and standardize Python code style

LLVM Project
1

In a recent review, we discussed Python coding style. I realized that we hadn’t documented the Python coding style we want the scripts in our repo to use.

The most logical thing is to document that we should follow PEP-8 in our python scripts.

We should also use Black to format Python code. It doesn’t require any configuration, follows PEP-8 (except in some cases) and is officially “blessed” by the PSF.

In the future we could add automatic formatting with black for Python scripts in pre-commit checks, but for now I think it’s enough to recommend it.

If no one disagrees, I will add this to the CodingStandard document.

2 Likes
2

+1 to having an official Python coding style. I don’t have any particular comment on what the style should be, but an official one seems as good a one as any (as long as our existing code doesn’t stray too far from it).

1 Like
3

UTC uses a 2-space indent like our C++ style

4

What’s UTC in this context?

I have no strong opinion about 2 vs. 4 spaces in Python, I personally think 2 spaces are nicer, but all PEP-8 tools enforce 4 by default, so I usually just go with the flow here.

5

I’d second the recommendation for black for newly added files at least. Internally we use it in conjunction with isort and just go with whatever formatting they impose and have never had a problem. In our case internally though we just reformatted our entire project when we switched to black. I’m not sure whether it allows partial formatting of selections or not. The LLVM project went through some quite large discussions for whether to reformat the entire LLVM codebase with clang-format when that became available but decided not to. Maybe the arguments are different for the more limited amount of python code in the project?

One question would be whether to accept the default 88 columns or to configure it for 80 columns to match the rest of the project. Beyond that it can’t really be configured (which I personally regard as a good thing).

While it’s still very early days, I’ve been having a really good time with ruff for linting, so we might want to consider recommending that new code is run through that, although I don’t know if we’d want to go so far as to enforce it just yet.

6

UpdateTestChecks. Which isn’t to say that we shouldn’t go with 4 spaces, just pointing out we have a chunk of regularly-used code that doesn’t adhere to that.

7

As long as there’s a used-everywhere tool to enforce a style, I’m not too fussed about what the exact style is.

+1 to do a bulk update of everything. Mixed styles is worse than any specific style. I hate that LLVM has consistently failed to do bulk updates whenever we actually manage to agree on a style.

Don’t forget the lit.cfg files peppered all through the test directories. They’re all Python, even if they don’t all have a .py extension.

1 Like
8

+1 on what’s been said here regarding consistency and having a tool to automate the process.

The majority of LLDB tests are written in Python. At some point in the past it was formatted but it was never maintained or enforced. I’m supportive of bulk formatting everything, but I’d like at least the opportunity to see if we can find a configuration that reduces the churn in our test suite. If we’re all using a tool (such as black or yapf) it shouldn’t matter if the configuration for LLDB is slightly different.

9

+1 for black, with one comment - black breaks compatibility between versions from different calendaric years: The Black Code Style - Black 23.1.0 documentation

I’ve seen this cause some formatting ping-pong in practice.

With clang format that’s less of a problem because we just vendor our own version :slight_smile:

Could we maybe vendor an official version of black through the existing CMake infrastructure to avoid this issue?

10

Did some checks with black - with default configuration (the only one that really counts in black) I ran:

black $(find . -name \*.py)
2035 files reformatted, 232 files left unchanged, 6 files failed to reformat.

git diffstat for that:

2035 files changed, 124430 insertions(+), 97959 deletions(-)

so that’s a lot of churn, let’s try black but with two spaces instead (there are no configuration options in black to set the indent, so you actually have to install another tool (black) that monkey patches that into black… sigh)

2078 files reformatted, 189 files left unchanged, 6 files failed to reformat.

diff stat:

2072 files changed, 221808 insertions(+), 201602 deletions(-)

so it would seem like 4 spaces are the more common indent style in our tree.

What about the files that couldn’t be parsed? One file seems to have out right parse error:

error: cannot format llvm/utils/lit/tests/shtest-encoding.py: 'utf-8' codec can't decode byte 0xc2 in position 69: invalid continuation byte

Aha, that file has an INTENTIONAL parse error :slight_smile:

5 files are still using python 2 syntax:

error: cannot format polly/utils/pyscop/jscop2iscc.py: Cannot parse: 12:8:   print "D :=",
Python 2 support was removed in version 22.0.
error: cannot format polly/utils/jscop2cloog.py: Cannot parse: 53:8:   print template % (context, domains, schedules)
Python 2 support was removed in version 22.0.
error: cannot format compiler-rt/lib/dfsan/scripts/build-libc-list.py: Cannot parse: 92:8:   print 'fun:%s=uninstrumented' % f
Python 2 support was removed in version 22.0.
error: cannot format polly/lib/External/isl/imath/tools/findthreshold.py: Cannot parse: 89:14:         print "%d\t%d\t%.3f\t%.3f\t%.4f" % (prec, thresh, trec, tnorm,
Python 2 support was removed in version 22.0.
error: cannot format openmp/runtime/tools/summarizeStats.py: Cannot parse: 146:14:         print "Cannot open " + fname
Python 2 support was removed in version 22.0.

So those needs to be fixed, in any case.

I also tried yapf with --style=pep8 and it seems way less picky:

1861 files changed, 75453 insertions(+), 62234 deletions(-)

Quickly looking at the files I still prefer black over yapf - but I don’t care that much, the important part is that we have a standard tool and that we enforce it - similar to what people have said above.

11

Note that yapf allows configuration of the style, so we could have a yapf config file in our repo similar to a .clang-format file to potentially reduce the churn even more. But since there hasn’t been any enforced standard before I wouldn’t expect there to be a single style that would fit all of our files.

12

It sounds like we have consensus around adding A code-standard.

It seems like we have consensus around that coding standard should be PEP-8. The only argument against seems to be that some code is using 2 spaces and not 4. But it also didn’t sound like @jrtc27 was totally against switching, did I read that correct?

About mass-reformatting. From what I read most seem to be in favour - but the diff seems huge, especially considering tests in lldb for example. Not sure we have a consensus around this yet?

I propose a document patch that codifies PEP-8 as our python coding style and recommends formatting code with black. I feel this step can be taken without deciding on mass reformat.

I would love to hear what people think about my stats on the patch size above and more arguments for and against mass reformatting.

1 Like
13

I’m curious what proportion of the changed files from your experiment are from the lldb tests? I count about 1200 *.py files under lldb/test. If most of them need formatting, that’s like half the total number of files. In that case, a mass-reformat of everything except lldb/test feels viable. The lldb folks can catch up as they go, and nobody else is really affected.

2 Likes
14

Yup, that was my original suggestion. My suspicion is that it’s exactly like that, but it would be good to confirm.

@tobiashieta Could you collect those numbers?

15

Running black on everything and then resetting the lldb subdirectory gives the following stats:

776 files changed, 68835 insertions(+), 55888 deletions(-)

So yeah it’s safe to say that more than half the files formatted was in lldb subdirectory.

I have posted three phabricator diffs to fix the syntax problems I found when running black, reviews of those would be great:

16

I’m happy to bulk update the cross-project-tests/debuginfo-tests/dexter directory, if it is useful to help break it up? FWIW using black version 22.8.0 that’s:

 79 files changed, 3621 insertions(+), 2994 deletions(-)

How does the decision become official? It looks like there’s one outstanding concern still:

17

I think the easiest is that we recommend black 23.x for now and maybe bump that next year if we feel there is a need for it.

1 Like
18

I think we’ll start with landing the Policy.

I am planning to submit that to Phab this weekend.

19

I have pushed an initial draft of the change to the documentation here: :gear: D143852 [docs] Add Python coding standard to documentation (llvm.org)

Not sure who I should add as reviewers, so please feel free to add yourselves.

20

I don’t think we came to a conclusion about 80-char vs 88-char lines. The reset of the project uses 80 (for code, anyway) and it seems a little odd to make that different for Python; the only justification being “that’s black’s default.”