[BEAM-9175] Introduce an autoformatting tool to Python SDK #10684

kamilwu · 2020-01-24T14:03:23Z

This PR adds yapf configuration and all the changes made after running it over the entire Python SDK codebase.
The result is fully compliant with PEP8 (lint check passes, the issues were manually resolved).

Some of the examples and rules of the newly enforced style (they can be put under discussion and reviewed):

Multiple dict entries can be put in the same line.
before:

beam/sdks/python/apache_beam/io/avroio_test.py

Lines 72 to 91 in b689fff

    
           RECORDS = [ 
        
               {'name': 'Thomas', 
        
                'favorite_number': 1, 
        
                'favorite_color': 'blue'}, 
        
               {'name': 'Henry', 
        
                'favorite_number': 3, 
        
                'favorite_color': 'green'}, 
        
               {'name': 'Toby', 
        
                'favorite_number': 7, 
        
                'favorite_color': 'brown'}, 
        
               {'name': 'Gordon', 
        
                'favorite_number': 4, 
        
                'favorite_color': 'blue'}, 
        
               {'name': 'Emily', 
        
                'favorite_number': -1, 
        
                'favorite_color': 'Red'}, 
        
               {'name': 'Percy', 
        
                'favorite_number': 6, 
        
                'favorite_color': 'Green'} 
        
           ]

after:

beam/sdks/python/apache_beam/io/avroio_test.py

Lines 71 to 76 in cfa9034

    
           RECORDS = [{'name': 'Thomas', 'favorite_number': 1, 'favorite_color': 'blue'}, 
        
                      {'name': 'Henry', 'favorite_number': 3, 'favorite_color': 'green'}, 
        
                      {'name': 'Toby', 'favorite_number': 7, 'favorite_color': 'brown'}, 
        
                      {'name': 'Gordon', 'favorite_number': 4, 'favorite_color': 'blue'}, 
        
                      {'name': 'Emily', 'favorite_number': -1, 'favorite_color': 'Red'}, 
        
                      {'name': 'Percy', 'favorite_number': 6, 'favorite_color': 'Green'}]

With this setting, it seems the result conforms more to the already used style (lower number of lines changed).

Yapf tries to put the first parameter on the same line where the function begins:

beam/sdks/python/apache_beam/utils/annotations_test.py

Lines 56 to 61 in cfa9034

    
           self.check_annotation(warning=w, 
        
                                 warning_type=BeamDeprecationWarning, 
        
                                 obj_name='fnc_test_deprecated_with_since_current', 
        
                                 annotation_type='deprecated', 
        
                                 label_check_list=[('since', True), 
        
                                                   ('instead', True)])

beam/sdks/python/apache_beam/io/gcp/bigquery.py

Lines 398 to 407 in cfa9034

    
           def __init__(self, 
        
                        table=None, 
        
                        dataset=None, 
        
                        project=None, 
        
                        query=None, 
        
                        validate=False, 
        
                        coder=None, 
        
                        use_standard_sql=False, 
        
                        flatten_results=True, 
        
                        kms_key=None):

...unless some of parameters have type annotation or are too long (then it does the split before the first argument):

beam/sdks/python/apache_beam/runners/direct/watermark_manager.py

Lines 129 to 136 in cfa9034

    
           def _update_pending( 
        
               self, 
        
               input_committed_bundle, 
        
               applied_ptransform,  # type: AppliedPTransform 
        
               completed_timers, 
        
               output_committed_bundles,  # type: Iterable[_Bundle] 
        
               unprocessed_bundles  # type: Iterable[_Bundle] 
        
           ):

Indent width is 2 and continuation indent is 4.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

sdks/python/apache_beam/typehints/opcodes.py

sdks/python/apache_beam/typehints/trivial_inference.py

sdks/python/apache_beam/typehints/trivial_inference_test_py3.py

mwalenia · 2020-01-27T10:45:09Z

retest this please

chadrik · 2020-01-27T18:37:55Z

We should add a pre-commit lint job to run yapf, otherwise the codebase will drift from the proper yapf formatting over time, and will be rectified all at once in some unrelated PR when a contributor finally runs it. I think we can all agree we don't want to be reviewing unrelated style changes in a PR.

From the docs:

If --diff is supplied, YAPF returns zero when no changes were necessary, non-zero otherwise (including program error). You can use this in a CI workflow to test that code has been YAPF-formatted.

Once this is in place we can dismantle the pylint and flake8 checks that overlap with yapf.

robertwb · 2020-01-27T19:01:52Z

I would suggest COALESCE_BRACKETS , SPLIT_BEFORE_FIRST_ARGUMENT, SPLIT_ALL_TOP_LEVEL_COMMA_SEPARATED_VALUES, and possibly SPLIT_BEFORE_EXPRESSION_AFTER_OPENING_PAREN to reduce churn.

SPLIT_ARGUMENTS_WHEN_COMMA_TERMINATED could also give authors a bit more freedom.

robertwb · 2020-01-27T19:02:34Z

Why was SPLIT_BEFORE_LOGICAL_OPERATOR disabled? Also, +1 to a precommit.

udim · 2020-01-28T01:19:29Z

Why was SPLIT_BEFORE_LOGICAL_OPERATOR disabled? Also, +1 to a precommit.

I asked for that: https://github.com/apache/beam/pull/10684/files/1250a914122615df951619361cc7f961f51e60b9..6baa4d4d2d8cf6e14348c55a521e38e9f84b8d21#diff-e710cc10a73c533a8ef446f99dce944e

kamilwu · 2020-01-28T15:30:13Z

@robertwb I managed to add all knobs you asked for. I think the code looks better now (much less weird lines, that's for sure).

I also added a pre-commit job that runs yapf with --diff option.

kamilwu · 2020-01-28T15:33:44Z

There is still a number of pylint issues Wrong continued indentation. Most of them appear because of how lambda is formatted. For example:

beam/sdks/python/apache_beam/io/gcp/bigquery_test.py

Lines 908 to 910 in 7db61fb

    
           table=lambda x, 
        
           tables: 
        
           (tables['table1'] if 'language' in x else tables['table2']),

I don't know yet how to deal with it. Unless there is a knob for this, I'll just put a # yapf: disable comments in these places.

Ardagan · 2020-01-28T19:22:03Z

Is it possible to split change to one that adds formatter and one that shows what formatting actually looks like?

Ardagan · 2020-01-28T19:24:05Z

+1 for running yapf on pre-commit. (Can be done in separate PR)

udim · 2020-01-28T19:25:59Z

There is still a number of pylint issues Wrong continued indentation. Most of them appear because of how lambda is formatted. For example:

beam/sdks/python/apache_beam/io/gcp/bigquery_test.py

Lines 908 to 910 in 7db61fb

table=lambda x,

tables:

(tables['table1'] if 'language' in x else tables['table2']),

I don't know yet how to deal with it. Unless there is a knob for this, I'll just put a # yapf: disable comments in these places.

If we adopt yapf's indentation style, then we can disable the same check in pylint.

Ardagan · 2020-01-28T19:27:39Z

There is still a number of pylint issues Wrong continued indentation. Most of them appear because of how lambda is formatted. For example:

beam/sdks/python/apache_beam/io/gcp/bigquery_test.py

Lines 908 to 910 in 7db61fb

table=lambda x,

tables:

(tables['table1'] if 'language' in x else tables['table2']),

I don't know yet how to deal with it. Unless there is a knob for this, I'll just put a # yapf: disable comments in these places.

If we adopt yapf's indentation style, then we can disable the same check in pylint.

If we utilize autoformatter, especially on pre-commit, it would be feasible to disable all formatting checks on linter. They become obsolete.

kamilwu · 2020-01-29T11:04:58Z

Is it possible to split change to one that adds formatter and one that shows what formatting actually looks like?

It's already split like this. The first commit only adds the formatter, and the second commit is the one that applies formatting changes to the code. Or do you mean creating a separate PR?

kamilwu · 2020-01-29T12:10:14Z

If we adopt yapf's indentation style, then we can disable the same check in pylint.

That's true. If that's so, I can disable that particular pylint check which causes problems. The question is, are we OK with how some lambdas are formatted?

mwalenia · 2020-01-29T12:20:19Z

retest this please

mwalenia · 2020-01-29T12:33:36Z

retest this please

mwalenia · 2020-01-29T12:52:32Z

retest this please

kamilwu · 2020-01-29T14:29:51Z

R: @udim @chadrik @robertwb Please take a look once again.

mwalenia · 2020-01-29T14:46:43Z

Run Java_Examples_Dataflow PreCommit

mwalenia · 2020-01-29T14:47:45Z

Run Java_Examples_Dataflow PreCommit

mwalenia · 2020-01-29T14:49:30Z

Run seed job

mwalenia · 2020-01-29T15:04:52Z

run seed job

mwalenia · 2020-01-29T15:06:49Z

Run PythonFormatter PreCommit

robertwb · 2020-01-30T20:11:38Z

I'm not a huge fan of how lambdas are formatted (tempted to file a bug against yapf), but there aren't too many of them so I'd be fine either disabling lint or disabling yapf for those lines.

kamilwu · 2020-01-31T10:12:35Z

Pylint shows 67 bad-continuation warnings, but some of them refer to the same malformed lambda. This means that we have 30-40 malformed lambdas in the whole project. I agree this isn't a large number.

@robertwb I've already disable lint for those lines in this PR

kamilwu · 2020-01-31T10:14:01Z

Java_Examples_Dataflow failure is unrelated (this check is flaky)

udim · 2020-01-31T22:52:30Z

LGTM

mwalenia · 2020-02-03T10:52:16Z

Run seed job

mwalenia · 2020-02-03T11:02:34Z

retest this please

mwalenia · 2020-02-03T11:06:38Z

Run PythonFormatter PreCommit

mwalenia · 2020-02-03T12:13:32Z

Run Python PreCommit

mwalenia · 2020-02-03T13:02:05Z

Run Python PreCommit

aaltay · 2020-02-05T23:54:33Z

Is it possible to update some docs/wiki, explaining how users could run this locally?

kamilwu · 2020-02-06T09:13:56Z

Is it possible to update some docs/wiki, explaining how users could run this locally?

Yes, luckily I have edit access to the wiki. I'll prepare an instruction. I think Python Tips is the best place for this.

mwalenia · 2020-02-06T10:24:06Z

run seed job

mwalenia · 2020-02-06T10:25:22Z

retest this please

mwalenia · 2020-02-06T10:27:00Z

run seed job

mwalenia · 2020-02-06T10:29:23Z

run seed job

mwalenia · 2020-02-06T10:36:31Z

retest this please

mwalenia · 2020-02-06T10:49:16Z

Run PythonFormatter PreCommit

kamilwu · 2020-02-06T12:00:42Z

Thanks everyone!

I've added an appropriate instruction here: https://cwiki.apache.org/confluence/display/BEAM/Python+Tips

udim · 2020-02-07T19:46:12Z

sdks/python/test-suites/tox/py37/build.gradle

@@ -35,6 +35,9 @@ lint.dependsOn lintPy37
 toxTask "mypyPy37", "py37-mypy"
 lint.dependsOn mypyPy37

+toxTask "formatter", "py3-yapf-check"
+check.dependsOn formatter


I missed this in my review. The lint precommit runs :pythonLintPreCommit, which depends on lint tasks.
Suggestion:

lint.dependsOn formatter

Nevermind, I see there's a new job named PythonFormatter