first onnx neural compressor commit

[chensuyue]

First onnx neural compressor commit

[justinchuby]

For CI, I would prefer github actions when possible.

[chensuyue]

Ok, we can have a migrate plan. May I know the reason to choose GHA instead of Azure pipeline?

[justinchuby]

Sure - ONNX has moved to use github actions because they are easier to manage (doesn't need to belong to an ado organization which would require additional permissions) and they run faster. So that's two wins. I use chatgpt to translate the yaml files and it was pretty straightforward: onnx/onnx#6075

[thuang6]

I saw there are several comments on CI tools, those are good suggestions. In our plan, this PR intends to land a MVP scope code migration to community first and enhance the CI/CD continuously during maintenance. I suggest to get this PR merged first and address these items progressively. does it make sense? @freddychiu @justinchuby

[thuang6]

Add issue Suggest to use github actions for CI when possible to track this comment

[justinchuby]

Consider leveraging lintrunner for the linting needs:

https://github.com/onnx/onnx/blob/main/CONTRIBUTING.md#coding-style
https://github.com/onnx/onnx/blob/main/.lintrunner.toml

[chensuyue]

The pre-commit.ci [Bot] is able to auto fix the format issue in the PR, do you want us to replace with lintrunner?

[justinchuby]

Yes, please consider lintrunner since it is a standard check in the ONNX repository. It will provide a uniform linting experience in CI and in local environments, even though there is no autofix-in-CI features yet (can be implemented if needed). In practice, the local autofix experience has been good.

[thuang6]

Add issue Consider leveraging lintrunner for the linting needs to track this comment

[justinchuby]

Please see https://github.com/onnx/onnx/blob/d6f87121ba256ac6cc4d1da0463c300c278339d2/pyproject.toml#L118 and enable more rules.

[justinchuby]

Suggested change

	"E402", # Module level import not at top of file
	"E501", # Line too long (121 > 120 characters)
	"E721", # Do not compare types, use isinstance()
	"E722", # Do not use bare except
	"E731", # Do not assign a lambda expression, use a def
	"E741", # Do not use variables named ‘l’, ‘O’, or ‘I’
	"F401", # {name} imported but unused
	"F403", # from {name} import * used; unable to detect undefined names
	"F405", # {name} may be undefined, or defined from star imports
	"F841", # Local variable is assigned to but never used{name}
	"E501", # Line too long (121 > 120 characters)
	"E731", # Do not assign a lambda expression, use a def
	"E741", # Do not use variables named ‘l’, ‘O’, or ‘I’

nit: I would refer to https://github.com/onnx/onnx/blob/d6f87121ba256ac6cc4d1da0463c300c278339d2/pyproject.toml#L154. For violating files, consider running auto-fix and create inline ignores using ruff check /path/to/file.py --add-noqa https://docs.astral.sh/ruff/linter/#inserting-necessary-suppression-comments so that the rules are not disabled for the rest of the codebase.

[justinchuby]

I suggest using the ruff formatter over black, because it is much faster: https://docs.astral.sh/ruff/formatter/

[justinchuby]

Suggested change

	exclude = [
	".bzr",
	".direnv",
	".eggs",
	".git",
	".git-rewrite",
	".hg",
	".ipynb_checkpoints",
	".mypy_cache",
	".nox",
	".pants.d",
	".pyenv",
	".pytest_cache",
	".pytype",
	".ruff_cache",
	".svn",
	".tox",
	".venv",
	".vscode",
	"__pypackages__",
	"_build",
	"buck-out",
	"build",
	"dist",
	"node_modules",
	"site-packages",
	"venv",
	]

ruff already excludes directories in gitignore by default https://docs.astral.sh/ruff/settings/#respect-gitignore

I suggest adding https://github.com/github/gitignore/blob/main/Python.gitignore to the current gitignore

[justinchuby]

Suggested change

# Exclude a variety of commonly ignored directories.

[justinchuby]

Suggested change

extend_skip_glob = ["**/__init__.py"]

Wondering why init files should be excluded?

[justinchuby]

On style: ONNX doesn't declare explicitly that it follows the Google Python style, so there's some wiggle room here. In practice, we enforce the Google style in code reviews (you may find some inconsistencies from old code). If there is a choice between "adopting the Google style guide and creating some exceptions" and "removing it altogether", I recommend the former. If the Google style guide doesn't fit the need, I would find a style guide that is comprehensive and easy to enforce enough that the code will look consistent across. (As an example, pep8 is not comprehensive enough such that many variations are complied with the guide)

Import: We are moving the ONNX project to be more compliant to the Google style, for good reasons: https://gist.github.com/justinchuby/9085242a53158f2fd7ae7aa650e55ee3#3-import-only-modules Feel free to share your rationale and create exceptions if needed though!

My recommendations:

1. Pick and adhere to a strong style guide, preferably Google style.
2. Create exceptions to the style guide if needed; justify them.
3. Use tools to automatically enforce them in CI. In practice ruff has been very good at doing so; we need to enable more ruff rules.

Sorry for being pedantic about style. Since this is the start of the project, it is a perfect time that we nail this down.

[thuang6]

On style: ONNX doesn't declare explicitly that it follows the Google Python style, so there's some wiggle room here. In practice, we enforce the Google style in code reviews (you may find some inconsistencies from old code). If there is a choice between "adopting the Google style guide and creating some exceptions" and "removing it altogether", I recommend the former. If the Google style guide doesn't fit the need, I would find a style guide that is comprehensive and easy to enforce enough that the code will look consistent across. (As an example, pep8 is not comprehensive enough such that many variations are complied with the guide)

Import: We are moving the ONNX project to be more compliant to the Google style, for good reasons: https://gist.github.com/justinchuby/9085242a53158f2fd7ae7aa650e55ee3#3-import-only-modules Feel free to share your rationale and create exceptions if needed though!

My recommendations:

1. Pick and adhere to a strong style guide, preferably Google style.
2. Create exceptions to the style guide if needed; justify them.
3. Use tools to automatically enforce them in CI. In practice ruff has been very good at doing so; we need to enable more ruff rules.

Sorry for being pedantic about style. Since this is the start of the project, it is a perfect time that we nail this down.

Thanks for recommendation. we understood your points on style part, specifically for import style, also understood the pros (avoid potential name conflict) and cons (a little bit tedious for long package/module name). In fact, There are all existing code migrated from original repo though it is the first PR in current repo. Anyway, we will take some time to check each import and revise or raise exception if there is issue found.

[justinchuby]

LGTM thanks for addressing the comments! Happy to see the unresolved items tracked by issues if the intention is to merge the migrated code first.

[thuang6]

I suggest using the ruff formatter over black, because it is much faster: https://docs.astral.sh/ruff/formatter/

tracked issue Suggest using the ruff formatter over black for this comment

[hshen14]

Thanks @justinchuby @freddychiu for your review!