[RFC][Formatting] Add scripts for applying Black to the Python code. by jroesch · Pull Request #6437 · apache/tvm

jroesch · 2020-09-10T05:33:09Z

As per the recent RFC https://discuss.tvm.apache.org/t/rfc-introduce-automatic-formatting-of-python-code/7843/10.

jroesch · 2020-09-10T05:40:04Z

cc @tqchen @areusch @comaniac @u99127 @jwfromm @mbrookhart @junrushao1994

junrushao · 2020-09-10T17:04:29Z

It is a bit hard to review 1000 files...maybe just take a look at the pyproject.toml file and assume other parts are correct?

comaniac

Tried to review the first 3 commits because I cannot even open the changes after the formatting commit...

I'd suggest separating two 2 PRs. The first one focuses on the scripts and CI flow (but disable the checking to pass the CI). The second one then formats all Python codes. In this way, as long as the second PR passes the CI we should be be good.

comaniac · 2020-09-10T17:21:08Z

+ exit 0
+fi
+
+echo "Running git-black against" $1


Suggested change
echo"Running git-black against"$1
echo"Running git-black on Python files against"$1

areusch

+1 to splitting the review into a scripts part and a format part.

jroesch · 2020-09-10T18:13:26Z

@areusch @tqchen @comaniac I can rollback the formatting, the first 3 or 4 commits were focused on formatting then I went through the process to see if it would actually work.

jroesch · 2020-09-10T19:43:08Z

@junrushao1994 @comaniac @areusch I just added the scripts and cleaned some things up, take another pass if you can

comaniac

LGTM. Just some nits. Thanks!

jroesch · 2020-09-11T01:38:19Z

@tqchen recommended that we first format the entire code base using these settings then try to land the CI parts, going to open a second PR with the fully formatted repo.

jroesch · 2020-09-11T07:37:04Z

This is blocked on #6448 and #6451, once we land those two it should be possible to add the checking to the CI, format once more and land this.

areusch · 2020-09-10T20:34:36Z

+ | tests\/
+ | vta\/
+ | web\/
+ )/|tests/lint/add_asf_header.py|tests/lint/check_file_type.py|python/tvm/topi/testing/pool3d_python.py|python/topi/python/test_topi_pooling.py


can you break these line by line and sort?

These two files actually trigger an internal error with black and I was going to open up a follow up issue to fix these.

areusch · 2020-09-10T20:34:47Z

 # under the License.
-
-'''Utility for Hexagon backend'''
+# pylint: disable=invalid-name


how come we need this?

areusch · 2020-09-10T20:35:35Z

 The dictionary of compile-time info.
 """
 return {k: v for k, v in GetLibInfo().items()} # pylint: disable=unnecessary-comprehension
+ }


there are a lot of these extra closing braces or insertions that look like function_call( with no close ) added. merge mistake? seems like nothing can possibly pass til they're deleted...

areusch · 2020-09-10T20:35:49Z

 from .util import _internal_assert

-# pylint: disable=redefined-builtin
+# pylint: disable=redefined-builtin,invalid-name


same question here--do we need to update pylint?

areusch · 2020-09-11T15:26:53Z

 max_output_range,
 out_dtype):
+ """Get the MKLDNN requantized scale."""
+ quantized_out_range = (


what's up with this line?

These are a bunch of random edits that made the results of black happier.

areusch · 2020-09-11T15:31:58Z

+fi
+
+echo "Running black in checking mode"
+black --diff --check


I think you either want --diff and assert on an empty output or --check, I don't know what both do?

I think diff and check at same time give that behavior if I understand correctly, check makes it return a -1 status code which should fail this entire script.

jroesch · 2020-09-11T23:48:54Z

I think I addressed all the comments, and bumped the CI.

junrushao · 2020-09-12T15:33:37Z

There is some flakiness in the CI. Please retrigger and let's merge it in

…pache#6437)

jroesch force-pushed the blackd branch from ba65ee3 to 34921bbCompare September 10, 2020 05:38

comaniac requested changes Sep 10, 2020
View reviewed changes

areusch requested changes Sep 10, 2020
View reviewed changes

Comment threadtests/lint/git-black.sh Outdated

jroesch changed the title ~~[RFC][Formatting] Apply black to entire Python code base.~~ [RFC][Formatting] Add scripts for applying Black to the Python code.Sep 10, 2020

comaniac approved these changes Sep 10, 2020
View reviewed changes

Comment threadtests/lint/git-black.sh Outdated

leandron reviewed Sep 11, 2020
View reviewed changes

Comment threadtests/lint/git-black.sh Outdated

areusch requested changes Sep 11, 2020
View reviewed changes

Add black to CI
c717c3b

jroesch force-pushed the blackd branch from b12efa2 to c717c3bCompare September 11, 2020 23:35

jroesch added 5 commits September 11, 2020 16:39

Clean up
2be9b9d

Add
1666389

Tweak script
38644c8

Update regexx
8db2b61

Tweak ignore list
e1c30f4

tqchen approved these changes Sep 12, 2020
View reviewed changes

junrushao approved these changes Sep 12, 2020
View reviewed changes

Bump lint container
2caeda1

jroesch force-pushed the blackd branch from 01e2adf to 2caeda1Compare September 12, 2020 01:42

tqchen merged commit cdd3206 into apache:masterSep 13, 2020

kevinthesun pushed a commit to kevinthesun/tvm that referenced this pull request Sep 17, 2020
[RFC][Formatting] Add scripts for applying Black to the Python code. (a…
542cb34
…pache#6437)

kevinthesun pushed a commit to kevinthesun/tvm that referenced this pull request Sep 18, 2020
[RFC][Formatting] Add scripts for applying Black to the Python code. (a…
7585921
…pache#6437)

trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 18, 2020
[RFC][Formatting] Add scripts for applying Black to the Python code. (a…
02b6e77
…pache#6437)

jroesch deleted the blackd branch February 4, 2021 04:42

	echo"Running git-black against"$1
	echo"Running git-black on Python files against"$1

Uh oh!

Conversation

jroesch commented Sep 10, 2020

Uh oh!

jroesch commented Sep 10, 2020

Uh oh!

junrushao commented Sep 10, 2020

Uh oh!

comaniac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jroesch commented Sep 10, 2020

Uh oh!

jroesch commented Sep 10, 2020

Uh oh!

comaniac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jroesch commented Sep 11, 2020

Uh oh!

jroesch commented Sep 11, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jroesch commented Sep 11, 2020

Uh oh!

junrushao commented Sep 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants