-
Notifications
You must be signed in to change notification settings - Fork 64
[Lint] Add Lint and move metadata from setup.py to pyproject.toml #120
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.
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
Changes from all commits
ce8a00a
c1bf7a6
9486154
75e4b0e
57101ee
7e60bf5
b1feb40
fd10719
8763776
8d56a86
7a742a0
66d284d
0bcca9a
87ffeda
70d32c6
89a904b
1554068
9d8a0a7
2c61dc6
8dd5314
d721a19
8d98b63
eb2783d
1ded058
bbd23c7
68fb8fa
01f2312
8604e3d
744090a
83c9409
d39b651
a223577
62d0a9b
1c79e88
94f4543
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# flake8 is currently not yet supported by pyproject.toml. | ||
# Please move this into pyproject.toml, when it does. | ||
|
||
[flake8] | ||
max-line-length = 95 | ||
### DEFAULT IGNORES FOR 4-space INDENTED PROJECTS ### | ||
# W503 talks about operator formatting which is too opinionated. | ||
# E203 is need to support black formatting. | ||
# https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html | ||
extend-ignore = E203, W503 | ||
exclude = | ||
.git, | ||
__pycache__, | ||
.eggs, | ||
**test/models/*.py, | ||
**onnx_backend_test_code/*.py, |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
name: Lint | ||
|
||
on: | ||
push: | ||
branches: | ||
- main | ||
pull_request: | ||
|
||
jobs: | ||
lint-python: | ||
name: Lint Python | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- name: Setup Python | ||
uses: actions/[email protected] | ||
with: | ||
python-version: "3.10" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can install all dev dependencies to help pylint find onnx etc. |
||
- name: misspell # Check spellings as well | ||
uses: reviewdog/action-misspell@v1 | ||
with: | ||
github_token: ${{ secrets.github_token }} | ||
locale: "US" | ||
reporter: github-pr-check | ||
level: info | ||
filter_mode: diff_context | ||
- name: shellcheck # Static check shell scripts | ||
uses: reviewdog/action-shellcheck@v1 | ||
with: | ||
github_token: ${{ secrets.github_token }} | ||
reporter: github-pr-check | ||
level: info | ||
filter_mode: diff_context | ||
|
||
enforce-style: | ||
name: Enforce style | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- name: Setup Python | ||
uses: actions/[email protected] | ||
with: | ||
# Version range or exact version of Python to use, using SemVer's version range syntax. Reads from .python-version if unset. | ||
python-version: "3.10" | ||
- name: Install ONNXScript | ||
run: | | ||
# The code is from azure-pipelines.yml | ||
# Install dependencies | ||
python -m pip install --upgrade pip | ||
python -m pip install --upgrade setuptools | ||
python -m pip install -q -r requirements-dev.txt | ||
# Install ONNX | ||
python -m pip uninstall -y onnx-function-experiment | ||
python -m pip uninstall -y ort-function-experiment-nightly | ||
python -m pip install onnx onnxruntime | ||
# install packages | ||
python -m pip install -e . | ||
- name: Run style.sh | ||
run: | | ||
bash tools/style.sh | ||
optional-style: | ||
name: Optional style | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- name: Setup Python | ||
uses: actions/[email protected] | ||
with: | ||
# Version range or exact version of Python to use, using SemVer's version range syntax. Reads from .python-version if unset. | ||
python-version: "3.10" | ||
- name: Install ONNXScript | ||
run: | | ||
# The code is from azure-pipelines.yml | ||
# Install dependencies | ||
python -m pip install --upgrade pip | ||
python -m pip install --upgrade setuptools | ||
python -m pip install -q -r requirements-dev.txt | ||
# Install ONNX | ||
python -m pip uninstall -y onnx-function-experiment | ||
python -m pip uninstall -y ort-function-experiment-nightly | ||
python -m pip install onnx onnxruntime | ||
# install packages | ||
python -m pip install -e . | ||
- name: Run style_optional.sh | ||
run: | | ||
bash tools/style_optional.sh |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,3 +7,4 @@ include requirements.txt | |
|
||
# exclude from sdist | ||
exclude *.onnx | ||
prune */__pycache__ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer ending all files with a newline. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to automate this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point. this is how pytorch does it https://github.com/pytorch/pytorch/blob/master/tools/linter/adapters/newlines_linter.py There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a temporary solution would be adding "auto-formatter" into a utils.sh (or even extend from style.sh). Some of auto-formatter that I can think of is black (included in CI), isort (included in CI), and autopep8 (required already in dev). And we can make "Linter" a long-term project? (There is also autoflake8 which I am not so familiar with, and these auto-formatter could be contradict to each other. ) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,20 +2,6 @@ | |
|
||
ONNXScript is a subset of Python that can be used to author ONNX functions (as well as ONNX models). | ||
|
||
## Contributing | ||
|
||
This project welcomes contributions and suggestions. Most contributions require you to agree to a | ||
Contributor License Agreement (CLA) declaring that you have the right to, and actually do, grant us | ||
the rights to use your contribution. For details, visit https://cla.opensource.microsoft.com. | ||
|
||
When you submit a pull request, a CLA bot will automatically determine whether you need to provide | ||
a CLA and decorate the PR appropriately (e.g., status check, comment). Simply follow the instructions | ||
provided by the bot. You will only need to do this once across all repos using our CLA. | ||
|
||
This project has adopted the [Microsoft Open Source Code of Conduct](https://opensource.microsoft.com/codeofconduct/). | ||
For more information see the [Code of Conduct FAQ](https://opensource.microsoft.com/codeofconduct/faq/) or | ||
contact [[email protected]](mailto:[email protected]) with any additional questions or comments. | ||
|
||
## Trademarks | ||
|
||
This project may contain trademarks or logos for projects, products, or services. Authorized use of Microsoft | ||
|
@@ -129,5 +115,44 @@ More examples can be found in folder [docs/examples](docs/examples). | |
|
||
## Contributing | ||
|
||
This project welcomes contributions and suggestions. Most contributions require you to agree to a | ||
Contributor License Agreement (CLA) declaring that you have the right to, and actually do, grant us | ||
the rights to use your contribution. For details, visit [Microsoft CLA](https://cla.opensource.microsoft.com). | ||
|
||
When you submit a pull request, a CLA bot will automatically determine whether you need to provide | ||
a CLA and decorate the PR appropriately (e.g., status check, comment). Simply follow the instructions | ||
provided by the bot. You will only need to do this once across all repos using our CLA. | ||
|
||
This project has adopted the [Microsoft Open Source Code of Conduct](https://opensource.microsoft.com/codeofconduct/). | ||
For more information see the [Code of Conduct FAQ](https://opensource.microsoft.com/codeofconduct/faq/) or | ||
contact [[email protected]](mailto:[email protected]) with any additional questions or comments. | ||
|
||
**Unit Test** | ||
|
||
Every change impacting the converter or the eager evaluation must be unit tested with | ||
class `OnnxScriptTestCase` to ensure both systems do return the same results with the same inputs. | ||
|
||
**Code Style** | ||
|
||
We use flake8, black, isort, and mypy to check code format. You can find their configuration in .flake8 and pyproject.toml, and | ||
|
||
run `auto-formatter` by: | ||
|
||
```bash | ||
./tools/lint.sh | ||
``` | ||
|
||
and run `check` by: | ||
|
||
```bash | ||
./tools/style.sh | ||
``` | ||
|
||
optional check: | ||
|
||
```bash | ||
./tools/style_optional.sh | ||
``` | ||
|
||
NOTE: mypy and pylint needs to be manually address | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,15 +39,16 @@ steps: | |
|
||
- script: | | ||
python -m pip install --upgrade pip | ||
pip install -r requirements-dev.txt | ||
python -m pip install --upgrade setuptools | ||
python -m pip install -r requirements-dev.txt | ||
displayName: 'Install dependencies' | ||
|
||
- script: | | ||
if [ '$(onnx.standard)' == '1' ] | ||
then | ||
pip uninstall -y onnx-function-experiment | ||
pip uninstall -y ort-function-experiment-nightly | ||
pip install onnx onnxruntime | ||
python -m pip uninstall -y onnx-function-experiment | ||
python -m pip uninstall -y ort-function-experiment-nightly | ||
python -m pip install onnx onnxruntime | ||
fi | ||
displayName: 'Install onnx' | ||
|
||
|
@@ -59,10 +60,6 @@ steps: | |
pytest -v onnxscript --cov=onnxscript --cov-report term-missing | ||
displayName: 'pytest' | ||
|
||
- script: | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice change by consolidating configs in the config file 🚀 |
||
python -m flake8 onnxscript --max-line-length 95 --exclude "**test/models/*.py,**onnx_backend_test_code/*.py" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be irrelevant to this PR: does anyone know why these files are ignored for formatter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
displayName: 'flake8' | ||
|
||
- script: | | ||
python setup.py install | ||
displayName: 'install package' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
[build-system] | ||
requires = ["setuptools", "wheel"] | ||
build-backend = "setuptools.build_meta" | ||
|
||
[project] | ||
name = "onnx-script" | ||
version = "0.1" | ||
description = "Authoring ONNX functions in Python" | ||
authors = [{ name = "Microsoft Corporation", email = "[email protected]" }] | ||
# TODO: put url/longer description into here | ||
# Get rid of longer description seems to require Poetry | ||
readme = "README.md" | ||
requires-python = ">=3.7" | ||
license = { text = 'Apache License v2.0' } | ||
classifiers = [ | ||
"Development Status :: 4 - Beta", | ||
"Environment :: Console", | ||
"Intended Audience :: Developers", | ||
"Operating System :: MacOS :: MacOS X", | ||
"Operating System :: Microsoft :: Windows", | ||
"Programming Language :: Python :: 3.7", | ||
"Programming Language :: Python :: 3.8", | ||
"Programming Language :: Python :: 3.9", | ||
"Programming Language :: Python :: 3.10", | ||
"License :: OSI Approved :: Apache Software License", | ||
] | ||
# NOTE: replace requirements.txt | ||
dependencies = ["numpy>=1.21.5", "protobuf<4", "onnx"] | ||
|
||
[project.optional-dependencies] | ||
test = ["flake8", "mypy", "black", "isort[colors]", "pylint"] | ||
|
||
[tool.distutils.bdist_wheel] | ||
universal = true | ||
|
||
[tool.pytest.ini_options] | ||
filterwarnings = ["ignore::UserWarning", "ignore::DeprecationWarning"] | ||
|
||
[tool.mypy] | ||
strict_optional = true | ||
warn_return_any = true | ||
warn_no_return = true | ||
# TODO warn_unused_ignores = true | ||
warn_redundant_casts = true | ||
warn_incomplete_stub = true | ||
# TODO disallow_untyped_calls = true | ||
check_untyped_defs = true | ||
disallow_any_generics = true | ||
no_implicit_optional = true | ||
# TODO disallow_incomplete_defs = true | ||
# TODO disallow_subclassing_any = true | ||
disallow_untyped_decorators = true | ||
warn_unused_configs = true | ||
show_error_codes = true | ||
show_column_numbers = true | ||
|
||
|
||
[[tool.mypy.overrides]] | ||
module = "tools.*" | ||
disallow_untyped_defs = true | ||
|
||
# Ignore errors in test | ||
[[tool.mypy.overrides]] | ||
module = [ | ||
"setup", | ||
"onnxscript.test.models.*", | ||
"onnxscript.test.onnx_backend_test_code.*", | ||
] | ||
ignore_errors = true | ||
|
||
[tool.black] | ||
target-version = ["py37", "py38", "py39", "py310"] | ||
extend-exclude = "^onnxscript/test/models" | ||
|
||
[tool.isort] | ||
profile = "black" | ||
extend_skip_glob = ["**test/models/*.py", "**onnx_backend_test_code/*.py"] | ||
|
||
[tool.pylint.MASTER] | ||
ignore-paths = [ | ||
"^onnxscript/test/models/", | ||
"^onnxscript/test/onnx_backend_test_code/", | ||
] | ||
|
||
[tool.pylint.'MESSAGES CONTROL'] | ||
disable = [ | ||
"missing-docstring", | ||
"import-error", | ||
"no-member", | ||
"line-too-long", | ||
"fixme", | ||
"too-few-public-methods", | ||
"no-name-in-module", | ||
"invalid-name", # TODO: Add naming guidance and enable this check. | ||
] |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried an easy fix with permission - read only on PR didn't work.
track #127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG. Can be another pr imo