Skip to content

Conversation

Aurelius84
Copy link
Contributor

@Aurelius84 Aurelius84 commented Oct 28, 2022

PR types

New features

PR changes

Others

Describe

What's new?

I found a great tool: autoflake, it has two import features for Paddle development:

  • Remove unused-imports

image

  • Remove unused-variables

image

@luotao1
Copy link
Contributor

luotao1 commented Oct 31, 2022

@SigureMo
Copy link
Member

SigureMo commented Oct 31, 2022

原来不将 autoflake 直接添加到 pre-commit 是因为 autoflake 自动删除是有可能导致原本能跑通的代码删除后跑不通的,因此只让 flake8 提示而不自动删除,这样可以选择是加 # noqa: F401 还是手动删除掉,考虑到存量已经修复,所以这个工作量应该不大,如果自动修复的影响认为是可接受的我觉得可以引入

由于 unused variable 存量没清,所以还不能加进来(大概试了下,应该有 500+ 文件需要清理),这个可以等存量清理后再加上

@SigureMo 看下,autoflake8的配置是否要添加进来

配置的话貌似 autoflake 有点 bug,无法禁用掉 remove-unused-variables,所以暂时直接在 pre-commit-config 里 args 修改下就好:

-   repo: https://github.com/PyCQA/autoflake
    rev: v1.7.7
    hooks:
    -   id: autoflake
        args:
            - --in-place
            - --remove-all-unused-imports
            - --ignore-pass-after-docstring
            - --ignore-init-module-imports
            - --exclude=python/paddle/fluid/[!t]**,python/paddle/fluid/tra**

这里 fluid 下非单测需要 exclude 掉,因为没有清理存量

@Aurelius84
Copy link
Contributor Author

-   repo: https://github.com/PyCQA/autoflake
    rev: v1.7.7
    hooks:
    -   id: autoflake
        args:
            - --in-place
            - --remove-all-unused-imports
            - --ignore-pass-after-docstring
            - --ignore-init-module-imports
            - --exclude=python/paddle/fluid/[!t]**,python/paddle/fluid/tra**

@SigureMo 我按照你上面这样修改,重新commit下?

@SigureMo
Copy link
Member

@SigureMo 我按照你上面这样修改,重新commit下?

嗯嗯,仅修改配置的话可以加一下 test=document_fix

@Aurelius84
Copy link
Contributor Author

exclude=python/paddle/fluid/[!t]**,python/paddle/fluid/tra**

@SigureMo 这一行的意思是仅仅对python/paddle/fluidtest 目录文件生效?

@SigureMo
Copy link
Member

@SigureMo 这一行的意思是仅仅对python/paddle/fluid 的 test 目录文件生效?

嗯嗯,对的,同时是和 flake8 配置保持一致

Paddle/.flake8

Lines 5 to 8 in 60e0c50

# A trick to exclude fluid/ but keep fluid/tests/, see more at
# https://github.com/PaddlePaddle/Paddle/pull/46290#discussion_r976392010
./python/paddle/fluid/[!t]**,
./python/paddle/fluid/tra**,

@Aurelius84
Copy link
Contributor Author

好的,非常感谢 @SigureMo 同学解答 ✿✿ヽ(°▽°)ノ✿

Copy link
Contributor

@luotao1 luotao1 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 再review下

无法禁用掉 remove-unused-variables

@SigureMo 这个是哪个错误码呢?

@luotao1
Copy link
Contributor

luotao1 commented Nov 1, 2022

因此只让 flake8 提示而不自动删除,这样可以选择是加 # noqa: F401 还是手动删除掉,考虑到存量已经修复,所以这个工作量应该不大,如果自动修复的影响认为是可接受的我觉得可以引入

如果加了autoflake8,# noqa: F401还生效么?因为当前有1700多处豁免的部分。 @SigureMo
image

@SigureMo
Copy link
Member

SigureMo commented Nov 1, 2022

@SigureMo 这个是哪个错误码呢?

unused variable 是错误码 F841(local variable name is assigned to but never used),存量 2124 处,需要之后考虑修复方案

如果加了autoflake8,# noqa: F401还生效么?因为当前有1700多处豁免的部分。 @SigureMo

autoflake 遵守 noqa 的规则,所以是生效的,可以通过运行 pre-commit run autoflake --all-files 来测试对全部文件的影响(除去 exclude 的),目前的话会将 python/paddle/fluid/tests/unittests/mkldnn/test_onnx_format_quantization_mobilenetv1.py 一个 pass 删除,这个修改是没有任何问题的,此外无任何其他影响

image

Copy link
Member

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

LGTM~

@Aurelius84 Aurelius84 merged commit fece00d into PaddlePaddle:develop Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants