Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CodeStyle][black] use black instead of yapf #46014

Merged
merged 4 commits into from
Oct 23, 2022
Merged

[CodeStyle][black] use black instead of yapf #46014

merged 4 commits into from
Oct 23, 2022

Conversation

SigureMo
Copy link
Member

@SigureMo SigureMo commented Sep 13, 2022 •
edited
Loading

PR types

Others

PR changes

Others

Describe

Use black as the new code formatting tool and format existing code.

本 PR 做了如下修改：

  1. 替换配置文件
# remove .style.yapf
# add pyproject.toml
[tool.black]
exclude = "build"
line-length = 80
skip-string-normalization = true
  • 参数 line-length 沿用了 yapf 的配置，以尽可能少地变动代码
  • 参数 skip-string-normalization 是允许字符串以 ' 包裹，black 默认是强制使用 " 包裹字符串的，后来做出了妥协添加了这一参数，这里也是为了尽可能减少代码变动而添加的参数（如不添加则 30 万行 changes、添加后为 24 万行）
  1. 替换 pre-commit hook
# .pre-commit-config.yaml
-   repo: https://github.com/psf/black.git
    rev: 22.8.0
    hooks:
    -   id: black
        files: (.*\.(py|pyi|bzl)|BUILD|.*\.BUILD|WORKSPACE)$
        # Temporary exclude, will be formatted in a separate PR
        exclude: |
            (?x)^(
                python/paddle/fluid/tests/unittests/dygraph_to_static/test_error.py|
                python/paddle/fluid/tests/unittests/dygraph_to_static/test_origin_info.py
            )$
  1. 全量格式化
pip install black==22.8.0
black .

git checkout develop -- 'python/paddle/fluid/tests/unittests/dygraph_to_static/test_error.py'
git checkout develop -- 'python/paddle/fluid/tests/unittests/dygraph_to_static/test_origin_info.py'

@paddle-bot
Copy link

paddle-bot bot commented Sep 13, 2022

你的PR提交成功，感谢你对开源项目的贡献!
请关注后续CI自动化测试结果，详情请参考Paddle-CI手册。
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@SigureMo

This comment was marked as outdated.

Sign in to view
@paddle-bot-old paddle-bot-old bot added the contributor External developers label Sep 13, 2022
@SigureMo SigureMo closed this Sep 13, 2022
@paddle-bot-old paddle-bot-old bot removed the contributor External developers label Sep 15, 2022
@SigureMo SigureMo reopened this Sep 23, 2022
@SigureMo
Copy link
Member Author

发现了 yapf 若干问题，这里再次尝试评估下收益

@SigureMo SigureMo closed this Sep 23, 2022
@paddle-bot
Copy link

paddle-bot bot commented Sep 23, 2022

很抱歉，经过我们的反复讨论，你的PR暂未达到合入标准，请阅读飞桨原生算子开发规范，你可以重新提交新的PR，我们先将此PR关闭，感谢你的贡献。
Sorry to inform you that through our discussion, your PR fails to meet the merging standard (Reference: Paddle Custom Operator Design Doc). You can also submit an new one. Thank you.

@SigureMo SigureMo reopened this Sep 23, 2022
@SigureMo SigureMo mentioned this pull request Sep 23, 2022
Merged
@SigureMo
Copy link
Member Author

SigureMo commented Sep 23, 2022 •
edited
Loading

在解决 trailing-whitespace 和 tabs 后的新统计（全部错误码，之前统计是默认配置，有 11 个错误码没统计）：

全量统计：

code    yapf    black   diff    percent autopep8        diff    percent

Type: E (8768 -> black 4784 & autopep8 1814) <--- 未统计 E501
E101    587     67      -520    -89%    19      -568    -97%
E121(*) 8       0       -8      -100%   0       -8      -100%
E122    98      0       -98     -100%   0       -98     -100%
E123(*) 13      0       -13     -100%   0       -13     -100%
E124    0       0       0       0%      4       4       0%
E125    214     0       -214    -100%   0       -214    -100%
E126(*) 837     0       -837    -100%   0       -837    -100%
E127    218     0       -218    -100%   1       -217    -100%
E128    246     0       -246    -100%   0       -246    -100%
E129    11      0       -11     -100%   0       -11     -100%
E131    56      0       -56     -100%   11      -45     -80%
E201    35      0       -35     -100%   0       -35     -100%
E202    17      0       -17     -100%   0       -17     -100%
E203    33      621     588     1782%   0       -33     -100%
E221    3       0       -3      -100%   0       -3      -100%
E225    67      0       -67     -100%   0       -67     -100%
E226(*) 96      0       -96     -100%   0       -96     -100%
E228    5       0       -5      -100%   0       -5      -100%
E231    61      3       -58     -95%    0       -61     -100%
E241(*) 4       0       -4      -100%   0       -4      -100%
E251    144     0       -144    -100%   0       -144    -100%
E261    11      0       -11     -100%   0       -11     -100%
E262    246     20      -226    -92%    0       -246    -100%
E265    1011    48      -963    -95%    238     -773    -76%
E266    129     129     0       0%      58      -71     -55%
E271    4       0       -4      -100%   0       -4      -100%
E272    3       0       -3      -100%   0       -3      -100%
E301    7       0       -7      -100%   5       -2      -29%
E302    3       0       -3      -100%   0       -3      -100%
E303    7       0       -7      -100%   2       -5      -71%
E305    2       0       -2      -100%   0       -2      -100%
E306    1       1       0       0%      0       -1      -100%
E401    19      19      0       0%      0       -19     -100%
E402    2744    2744    0       0%      341     -2403   -88%
E501    28953   25260   -3693   -13%    27267   -1686   -6%
E502    496     0       -496    -100%   0       -496    -100%
E701    197     0       -197    -100%   0       -197    -100%
E704(*) 0       0       0       0%      73      73      0%
E711    269     269     0       0%      269     0       0%
E712    394     394     0       0%      394     0       0%
E713    34      34      0       0%      34      0       0%
E714    5       5       0       0%      5       0       0%
E721    9       9       0       0%      9       0       0%
E722    187     187     0       0%      187     0       0%
E731    73      70      -3      -4%     0       -73     -100%
E741    164     164     0       0%      164     0       0%

Type: F (10910 -> black 10928 & autopep8 10879)
F401    7365    7383    18      0%      7353    -12     -0%
F402    1       1       0       0%      1       0       0%
F403    149     149     0       0%      149     0       0%
F404    0       0       0       0%      7       7       0%
F405    685     685     0       0%      685     0       0%
F522    11      11      0       0%      11      0       0%
F524    1       1       0       0%      1       0       0%
F541    33      33      0       0%      33      0       0%
F601    13      13      0       0%      13      0       0%
F631    2       2       0       0%      2       0       0%
F632    20      20      0       0%      20      0       0%
F811    201     201     0       0%      175     -26     -13%
F821    94      94      0       0%      94      0       0%
F823    1       1       0       0%      1       0       0%
F841    2334    2334    0       0%      2334    0       0%

Type: W (2247 -> black 4378 & autopep8 1758)
W191    587     67      -520    -89%    19      -568    -97%
W503(*) 377     4130    3753    995%    2       -375    -99%
W504(*) 1102    0       -1102   -100%   1556    454     41%
W601    8       8       0       0%      8       0       0%
W605    173     173     0       0%      173     0       0%

排除 fluid（但包含 fluid/tests）：

code    yapf    black   diff    percent autopep8        diff    percent

Type: E (7216 -> black 4347 & autopep8 1549) <--- 未统计 E501
E101    11      10      -1      -9%     10      -1      -9%
E121(*) 8       0       -8      -100%   0       -8      -100%
E122    81      0       -81     -100%   0       -81     -100%
E123(*) 12      0       -12     -100%   0       -12     -100%
E124    0       0       0       0%      1       1       0%
E125    168     0       -168    -100%   0       -168    -100%
E126(*) 723     0       -723    -100%   0       -723    -100%
E127    140     0       -140    -100%   1       -139    -99%
E128    207     0       -207    -100%   0       -207    -100%
E129    9       0       -9      -100%   0       -9      -100%
E131    45      0       -45     -100%   10      -35     -78%
E201    29      0       -29     -100%   0       -29     -100%
E202    11      0       -11     -100%   0       -11     -100%
E203    32      563     531     1659%   0       -32     -100%
E225    61      0       -61     -100%   0       -61     -100%
E226(*) 93      0       -93     -100%   0       -93     -100%
E228    3       0       -3      -100%   0       -3      -100%
E231    60      3       -57     -95%    0       -60     -100%
E241(*) 2       0       -2      -100%   0       -2      -100%
E251    109     0       -109    -100%   0       -109    -100%
E261    11      0       -11     -100%   0       -11     -100%
E262    238     17      -221    -93%    0       -238    -100%
E265    925     48      -877    -95%    218     -707    -76%
E266    116     116     0       0%      57      -59     -51%
E271    4       0       -4      -100%   0       -4      -100%
E272    1       0       -1      -100%   0       -1      -100%
E301    7       0       -7      -100%   5       -2      -29%
E302    3       0       -3      -100%   0       -3      -100%
E303    7       0       -7      -100%   2       -5      -71%
E305    2       0       -2      -100%   0       -2      -100%
E306    1       1       0       0%      0       -1      -100%
E401    19      19      0       0%      0       -19     -100%
E402    2666    2666    0       0%      341     -2325   -87%
E501    19252   16239   -3013   -16%    17714   -1538   -8%
E502    400     0       -400    -100%   0       -400    -100%
E701    108     0       -108    -100%   0       -108    -100%
E704(*) 0       0       0       0%      62      62      0%
E711    166     166     0       0%      166     0       0%
E712    340     340     0       0%      340     0       0%
E713    22      22      0       0%      22      0       0%
E714    4       4       0       0%      4       0       0%
E721    8       8       0       0%      8       0       0%
E722    149     149     0       0%      149     0       0%
E731    62      62      0       0%      0       -62     -100%
E741    153     153     0       0%      153     0       0%

Type: F (9895 -> black 9913 & autopep8 9865)
F401    6750    6768    18      0%      6739    -11     -0%
F402    1       1       0       0%      1       0       0%
F403    57      57      0       0%      57      0       0%
F404    0       0       0       0%      7       7       0%
F405    556     556     0       0%      556     0       0%
F522    1       1       0       0%      1       0       0%
F524    1       1       0       0%      1       0       0%
F541    33      33      0       0%      33      0       0%
F601    7       7       0       0%      7       0       0%
F631    2       2       0       0%      2       0       0%
F632    18      18      0       0%      18      0       0%
F811    177     177     0       0%      151     -26     -15%
F821    88      88      0       0%      88      0       0%
F841    2204    2204    0       0%      2204    0       0%

Type: W (1414 -> black 3490 & autopep8 1484)
W191    11      10      -1      -9%     10      -1      -9%
W503(*) 279     3305    3026    1085%   2       -277    -99% （非默认配置项）
W504(*) 949     0       -949    -100%   1297    348     37%
W601    3       3       0       0%      3       0       0%
W605    172     172     0       0%      172     0       0%

注：标 * 为非默认配置，未被普遍接受的错误码

这次 black 对 E 和 W 的修复程度媲美 autopep8 了，估计是因为 #46410 移除了部分 disable 后 black 在相关文件里进行了格式化的效果

仅有 4 个 CI 未通过（无单测相关，单测均已通过）

image

@SigureMo SigureMo mentioned this pull request Sep 23, 2022
Closed
@paddle-bot-old paddle-bot-old bot added the contributor External developers label Sep 23, 2022
@luotao1 luotao1 self-assigned this Sep 27, 2022
@luotao1
Copy link
Contributor

luotao1 commented Sep 27, 2022 •
edited by SigureMo
Loading

black 替换 yapf 的优势

1. Github 使用情况

根据 #46014 (comment) ，black 的 stars 数、使用量、速度都远超 yapf。

yapf black
GitHub Stars 12.8k 29.2k
GitHub Used by 37.1k 181k
社区活跃度 不活跃，接近不维护的状态，除去 9.24 处理了几个 PR 外，上次 commit 已经是 3 月的事了，社区积累了大量未处理的 issue 非常活跃，基本每天都有 commit
速度 慢（Paddle 全量格式化需 13min） 快（同全量格式化 1min）
格式化力度 不注重 PEP 8，主要注重代码格式的统一 兼顾 PEP 8 和代码格式的统一
可配置性 各式各样的配置，可以设置各式各样的风格，有点类似 clang-format 基本没有配置项

2. 业内调研

pytorch 的 python 工具为：flake8==3.8.2, black==22.3.0, mypy==0.950，没有使用 yapf

3. Error 数量和项目大大减少

根据 #46014 (comment) ，整理：在解决 trailing-whitespace 和 tabs 后的新统计（除 E501 的全部错误码，排除 fluid，但包含 fluid/tests）：

yapf black 备注
E 7216行，42子项 4347 处，16子项 black 优势很明显
F 9895行，13子项 9913 处，13子项 F 可能会影响语义，不应该被格式化，两者效果类似
W 1135行，4子项 185 行，3子项 black 优势很明显

大大减少的错误码可以让开发者专注于编写代码逻辑，而不是浪费时间在调整代码格式上

4. 生态更好

有很多工具对 black 支持/兼容性更好，比如 isort 专门有个 profile 就是针对 black 的，可以保证与 black 没有冲突，而 yapf 则很难与 isort 一起工作（见 #46475 (comment) ），以及 docstring、rst 文档示例代码格式化工具 https://github.com/asottile/blacken-docs 也是集成了 black

black 替换 yapf 的不足（代码风格）

  • 唯一的问题是代码风格不可调节，如果不能接受 black 格式化后的代码风格的话可能想调节都没办法。（代码风格的变化见本 PR files changed）
  • 但虽然yapf 的可配置性非常高，可以配置出任意需要的风格，不过目前 Paddle 只是使用了默认配置，这一优点可以忽略。

black 替换 yapf 的可行性操作

  1. develop 分支修复存量问题，并将配置从 yapf 改成 black：即 merge 本 PR
    • 由于本 PR 较大，会提前发出通知，并在周末合入。
  2. release/2.4 分支只改配置，不修存量问题

@SigureMo SigureMo closed this Sep 28, 2022
@SigureMo SigureMo reopened this Sep 28, 2022
luotao1
luotao1 reviewed Sep 29, 2022
View reviewed changes
pyproject.toml
[tool.black]
exclude = "build"
line-length = 80
skip-string-normalization = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以加一下skip-string-normalization参数的说明

@SigureMo SigureMo changed the title [Dont merge][CodeStyle] use black instead of yapf [WIP][CodeStyle] use black instead of yapf Oct 17, 2022
@SigureMo SigureMo force-pushed the blacken-python-code branch from 084a66a to ba941ad Compare October 17, 2022 10:37
XiaoguangHu01
XiaoguangHu01 previously approved these changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SigureMo
Copy link
Member Author

release/2.4 的仅配置修改 PR 已提交：#47146

@SigureMo SigureMo force-pushed the blacken-python-code branch from 794e895 to 9ed57dc Compare October 22, 2022 13:10
@SigureMo SigureMo changed the title [WIP][CodeStyle] use black instead of yapf [CodeStyle][black] use black instead of yapf Oct 22, 2022
@SigureMo SigureMo force-pushed the blacken-python-code branch from afddd50 to da46870 Compare October 22, 2022 14:36
@SigureMo
Copy link
Member Author

SigureMo commented Oct 23, 2022 •
edited
Loading

image

PR-CI-Coverage 流水线由于通过 git diff 计算得到的 diff_py_file 太长导致 Argument list too long（同 #42944），按照 #42944 方式先临时注释掉一些行，在 #47266 进行了恢复

@SigureMo SigureMo mentioned this pull request Oct 23, 2022
Merged
@luotao1 luotao1 merged commit 7097630 into PaddlePaddle:develop Oct 23, 2022
@SigureMo SigureMo deleted the blacken-python-code branch October 23, 2022 12:04
luotao1 pushed a commit that referenced this pull request Oct 23, 2022
@SigureMo
[CodeStyle][black] restore changes of PR-CI-Coverage in #46014 (#47266)
a5f556f
@luotao1 luotao1 mentioned this pull request Oct 24, 2022
Merged
@SigureMo SigureMo mentioned this pull request Oct 24, 2022
Merged
@SigureMo SigureMo mentioned this pull request Nov 3, 2022
Closed
@SigureMo SigureMo mentioned this pull request Jun 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luotao1 luotao1 luotao1 approved these changes

@XiaoguangHu01 XiaoguangHu01 XiaoguangHu01 left review comments

Assignees

@luotao1 luotao1

Labels
contributor External developers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SigureMo @luotao1 @XiaoguangHu01