[Lint] Add Lint and move metadata from setup.py to pyproject.toml #120

titaiwangms · 2022-09-16T16:22:28Z

This PR basically migrates the lint and CI from ONNX to here.

Add lint: black, isort, mypy, and flake8
- flake8 remains basic setting as main branch.
- auto-formatter will be introduced in the next PR ([Lint] Apply lint #126 )
Add pyproject.toml (includes what was in requirements.txt, setup.cfg, and metadata in setup.py)
- Avoiding parsing documents in setup.py can reduce bugs (version_str was -0.1-)
- Put metadata all in one file (pyproject.toml )
- benefit: toml becomes more important in python, check here
Add lint CI to align coding style
- Lint / Enforce style (pull_request) CI utilizes style.sh to do the test
- We can only make Lint / Enforce style (pull_request) as required after a reasonable transition time period
Continue work (actually apply lint on files) on [Lint] Apply lint #126

fixes #105

Move metadata into pyproject.toml [Lint] Add CI

docs/conf.py

gramalingam · 2022-09-20T21:28:14Z

A generic question (on the broader topic): Is there any way for a reviewer to turn off the github warning messages in the UI? They seem appropriate for the person who created the PR, but takes up a lot of screen space for the reviewer.

gramalingam · 2022-09-20T21:29:43Z

tools/style.sh

@@ -0,0 +1,33 @@
+#!/usr/bin/env bash


I assume that this is only for checking? How many of these can be fixed automatically? Can we create a utility for that?

Yes, I will see if I can include some useful auto-formatter and put them into a bash script.

titaiwangms · 2022-09-20T21:47:48Z

A generic question (on the broader topic): Is there any way for a reviewer to turn off the github warning messages in the UI? They seem appropriate for the person who created the PR, but takes up a lot of screen space for the reviewer.

Sorry, that's my fault that I used ONNX setting, but didn't include pylint for now (since it warns a lot.). Ususally the warning message wouldn't be this much. The warning message are all about pylint, and I haven't adjusted that (In normal case, you get the warning in local, and address it). But some checks I would suggest we keep it on UI, like misspelling. I will add pylint in the next commit.

onnxscript/__init__.py

pyproject.toml

justinchuby · 2022-09-21T16:51:38Z

.github/workflows/lint.yaml

+      - name: Setup Python
+        uses: actions/[email protected]
+        with:
+          python-version: "3.10"


We can install all dev dependencies to help pylint find onnx etc.

azure-pipelines.yml

jcwchen · 2022-09-21T19:00:57Z

azure-pipelines.yml

@@ -59,10 +60,6 @@ steps:
    pytest -v onnxscript --cov=onnxscript --cov-report term-missing
  displayName: 'pytest'

- script: |
-    python -m flake8 onnxscript --max-line-length 95 --exclude "**test/models/*.py,**onnx_backend_test_code/*.py"


Might be irrelevant to this PR: does anyone know why these files are ignored for formatter?

I guess the tests are about how you can use onnx_script, and those writings doesn't have to follow formatting as it's "user-side code". but cc @gramalingam

justinchuby · 2022-09-21T23:03:18Z

.github/workflows/lint.yaml

+
+jobs:
+  lint-python:
+    name: Lint Python


We should consider limiting the github token's permission for this job:

https://docs.github.com/en/actions/security-guides/automatic-token-authentication#example-1-passing-the-github_token-as-an-input

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions

Tried an easy fix with permission - read only on PR didn't work.
track #127

SG. Can be another pr imo

titaiwangms · 2022-09-21T23:58:12Z

cc @gramalingam for final review.

gramalingam · 2022-09-23T15:27:16Z

tools/style_optional.sh

@@ -0,0 +1,25 @@
+#!/usr/bin/env bash


Would it be possible to add something for windows? That would be great, thanks!

I will have .bat version in another PR. #127

xadupre · 2022-09-23T15:30:11Z

.flake8

@@ -0,0 +1,16 @@
+# flake8 is currebtly not yet supported by pyproject.toml.


misspelling

xadupre · 2022-09-23T15:35:36Z

pyproject.toml

+  "License :: OSI Approved :: Apache Software License",
+]
+# NOTE: replace requirements.txt
+dependencies = ["numpy==1.21.5", "protobuf<4", "onnx"]


maybe we can replace == by >= ?

Should be merged after #120 fixes #125 1. add `auto-formatter` **lint.sh** (feel free to add on others) 2. flake8, black, and isort formatting is clean now 3. pylint and mypy is a lot of work to address - need everyone address these step by step (also take them out from style.sh, and make style_optional.sh CI test for them.) 4. relative path addressed (import module instead of class and function) 5. We should enable Lint/Enforce as mandatory 6. Move debuginfo stand alone as it caused circular import. NOTE: I hope this one can be quickly merged, as there are PRs being merged constantly which means endless merge conflict for this one. Co-authored-by: Ubuntu <titaiwang@titaiwanglinuxcpudev.y3zdd0j2xrqelnmezcgpqgmnte.jx.internal.cloudapp.net>

Ubuntu and others added 12 commits September 14, 2022 19:59

modified a line in example

ce8a00a

doc inside script

c1bf7a6

[Lint] Add flake8+back+isort

9486154

Move metadata into pyproject.toml [Lint] Add CI

[Lint] Add flake8+back+isort

75e4b0e

Move metadata into pyproject.toml [Lint] Add CI

modified .flake8

57101ee

modified .flake8

7e60bf5

CI without install

b1feb40

CI install package

fd10719

__version__ single-source

8763776

add py3.7 __version__ support

8d56a86

use pip install in CI

7a742a0

importlib.metadata

66d284d

github-advanced-security bot found potential problems Sep 16, 2022

View reviewed changes

docs/conf.py Fixed Show fixed Hide fixed

titaiwangms added 11 commits September 16, 2022 19:30

CI-patch1

0bcca9a

CI patch-2

87ffeda

CI-patch3

70d32c6

CI patch-4

89a904b

CI patch-5

1554068

CI patch-6

9d8a0a7

exclude tests

2c61dc6

fix import bugs

8dd5314

input package name

d721a19

update README.md with lint

8d98b63

Merge branch 'main' into titaiwang/add_lint

eb2783d

titaiwangms changed the title ~~[WIP][Lint] Add Lint and move metadata from setup.py to pyproject.toml~~ [Lint] Add Lint and move metadata from setup.py to pyproject.toml Sep 20, 2022

titaiwangms added the enhancement New feature or request label Sep 20, 2022

titaiwangms requested review from xadupre, gramalingam, justinchuby and liqunfu September 20, 2022 15:35

gramalingam reviewed Sep 20, 2022

View reviewed changes

titaiwangms added 2 commits September 21, 2022 13:59

test on changes

1ded058

add pylint into toml and check importError

bbd23c7

github-advanced-security bot found potential problems Sep 21, 2022

View reviewed changes

onnxscript/__init__.py Fixed Show fixed Hide fixed

titaiwangms added 2 commits September 21, 2022 14:46

added pylint into style.sh and test __package__

68fb8fa

pylint addressed

01f2312

justinchuby reviewed Sep 21, 2022

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

titaiwangms added 3 commits September 21, 2022 15:27

update .flake8 comment

8604e3d

modified follow warnings on UI

744090a

typo patch-1

83c9409

justinchuby approved these changes Sep 21, 2022

View reviewed changes

format toml and update .flake8

d39b651

justinchuby reviewed Sep 21, 2022

View reviewed changes

titaiwangms requested review from gramalingam and jcwchen September 21, 2022 18:39

jcwchen approved these changes Sep 21, 2022

View reviewed changes

update azure-ci

a223577

justinchuby reviewed Sep 21, 2022

View reviewed changes

titaiwangms mentioned this pull request Sep 21, 2022

[Lint] Apply lint #126

Merged

titaiwangms added 2 commits September 22, 2022 00:11

Merge branch 'main' into titaiwang/add_lint

62d0a9b

add auto-formatter

1c79e88

gramalingam reviewed Sep 23, 2022

View reviewed changes

xadupre reviewed Sep 23, 2022

View reviewed changes

minor fixes

94f4543

titaiwangms merged commit a48de87 into microsoft:main Sep 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lint] Add Lint and move metadata from setup.py to pyproject.toml #120

[Lint] Add Lint and move metadata from setup.py to pyproject.toml #120

titaiwangms commented Sep 16, 2022 •

edited

Loading

gramalingam commented Sep 20, 2022

gramalingam Sep 20, 2022

titaiwangms Sep 21, 2022

titaiwangms commented Sep 20, 2022 •

edited

Loading

justinchuby Sep 21, 2022

jcwchen Sep 21, 2022

titaiwangms Sep 21, 2022

justinchuby Sep 21, 2022

titaiwangms Sep 22, 2022

justinchuby Sep 22, 2022

titaiwangms commented Sep 21, 2022

gramalingam Sep 23, 2022

titaiwangms Sep 23, 2022 •

edited

Loading

xadupre Sep 23, 2022

titaiwangms Sep 23, 2022

xadupre Sep 23, 2022

titaiwangms Sep 23, 2022

		@@ -0,0 +1,16 @@
		# flake8 is currebtly not yet supported by pyproject.toml.

[Lint] Add Lint and move metadata from setup.py to pyproject.toml #120

[Lint] Add Lint and move metadata from setup.py to pyproject.toml #120

Conversation

titaiwangms commented Sep 16, 2022 • edited Loading

gramalingam commented Sep 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titaiwangms commented Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titaiwangms commented Sep 21, 2022

Choose a reason for hiding this comment

titaiwangms Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

titaiwangms commented Sep 16, 2022 •

edited

Loading

titaiwangms commented Sep 20, 2022 •

edited

Loading

titaiwangms Sep 23, 2022 •

edited

Loading