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.

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

pyproject.toml #2981

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

AlexanderDokuchaev
Copy link
Collaborator

@AlexanderDokuchaev AlexanderDokuchaev commented Sep 23, 2024

Changes

Use pyproject.toml instead of setup.py

Reason for changes

  • PEP 517/518
  • Tools like Black, Isort, Ruff, and Mypy can automatically use settings defined in a pyproject.toml file, allowing these tools to apply configurations directly from the repository without requiring additional configuration in text editor or extra arguments in terminal.

Tests

nightly/job/install_onnx/543/
nightly/job/install_ov/567/
nightly/job/install_pt_cpu/566/
nightly/job/ubuntu20_install_tf/672/

@github-actions github-actions bot added dependencies Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM NNCF TF Pull requests that updates NNCF TensorFlow NNCF PT Pull requests that updates NNCF PyTorch experimental labels Sep 23, 2024
@AlexanderDokuchaev
Copy link
Collaborator Author

AlexanderDokuchaev commented Sep 24, 2024

whl package

setup.py:

unzip -l dist/*whl | tail -n7
    11357  2024-09-24 10:37   nncf-2.14.0.dev0+unknown.version.dist-info/LICENSE
    10220  2024-09-24 10:37   nncf-2.14.0.dev0+unknown.version.dist-info/METADATA
       91  2024-09-24 10:37   nncf-2.14.0.dev0+unknown.version.dist-info/WHEEL
        5  2024-09-24 10:37   nncf-2.14.0.dev0+unknown.version.dist-info/top_level.txt
    60943  2024-09-24 10:37   nncf-2.14.0.dev0+unknown.version.dist-info/RECORD
---------                     -------
  4583702                     615 files

pyproject.toml:

unzip -l dist/*whl | tail -n 7
    11357  2024-09-24 13:11   nncf-2.14.0.dev0+unknown.version.dist-info/LICENSE
    10279  2024-09-24 13:11   nncf-2.14.0.dev0+unknown.version.dist-info/METADATA
       91  2024-09-24 13:11   nncf-2.14.0.dev0+unknown.version.dist-info/WHEEL
        5  2024-09-24 13:11   nncf-2.14.0.dev0+unknown.version.dist-info/top_level.txt
    60943  2024-09-24 13:11   nncf-2.14.0.dev0+unknown.version.dist-info/RECORD
---------                     -------
  4581438                     615 files

pip show

setup.py

pip show nncf
Name: nncf
Version: 2.14.0.dev0+unknown.version
Summary: Neural Networks Compression Framework
Home-page: https://github.com/openvinotoolkit/nncf
Author: Intel
Author-email: alexander.kozlov@intel.com
License: Apache-2.0
Location: /home/adokucha/work/venv/lib/python3.10/site-packages
Requires: jsonschema, jstyleson, natsort, networkx, ninja, numpy, openvino-telemetry, packaging, pandas, psutil, pydot, pymoo, rich, scikit-learn, scipy, tabulate, tqdm
Required-by: 

pyproject.toml

Name: nncf
Version: 2.14.0.dev0+unknown.version
Summary: Neural Networks Compression Framework
Home-page: https://github.com/openvinotoolkit/nncf
Author: Intel
Author-email: alexander.kozlov@intel.com
License: Apache-2.0
Location: /home/adokucha/work/venv/lib/python3.10/site-packages
Requires: jsonschema, jstyleson, natsort, networkx, ninja, numpy, openvino-telemetry, packaging, pandas, psutil, pydot, pymoo, rich, scikit-learn, scipy, tabulate, tqdm
Required-by: 

@AlexanderDokuchaev AlexanderDokuchaev marked this pull request as ready for review September 25, 2024 08:29
@AlexanderDokuchaev AlexanderDokuchaev requested a review from a team as a code owner September 25, 2024 08:29
run: |
python -m build -C--global-option=--release
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no way to pass global option to control format of version, that used env variable instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was more convenient to pass this option into CLI.

repo_root = os.path.dirname(os.path.realpath(__file__))
run = subprocess.run(["git", "diff-index", "--quiet", "HEAD"], cwd=repo_root) # nosec
if run.returncode == 1:
dev_version_id += "dirty"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Detect editable mode pip install -e is not possible to check here, instead of it add dirty suffix if code was modified.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous editable suffix was more correct.

elif package_type == "build_w":
run_cmd_line = f"{python_executable_with_venv} -m build -n -w"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

 --no-isolation, -n    disable building the project in an isolated virtual environment. Build dependencies must be installed
                        separately when this option is used

Copy link
Contributor

@alexsu52 alexsu52 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -39,8 +39,9 @@ jobs:
env:
TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }}
TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }}
NNCF_RELEASE_BUILD: "1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes need to be placed in our release procedure documentation.

run: |
python -m build -C--global-option=--release
Copy link
Collaborator

Choose a reason for hiding this comment

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

It was more convenient to pass this option into CLI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From my perspective, it is not correct to place helper-like files named with _ at the root of the project.
@alexsu52, @AlexKoff88, could you please comment on this?

Comment on lines +13 to +16
# NOTE 1: This module generates the dynamic version for the package during the build process.
# It provides the version attribute for setuptools as specified in pyproject.toml:
# [tool.setuptools.dynamic]
# version = { attr = "_version_helper.version" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct NOTE and behaviour in general, since trying to build without the NNCF_RELEASE_BUILD variable leads to incorrect file naming:
image
Need to be fixed despite the fact that this issue exists on the develop.

repo_root = os.path.dirname(os.path.realpath(__file__))
run = subprocess.run(["git", "diff-index", "--quiet", "HEAD"], cwd=repo_root) # nosec
if run.returncode == 1:
dev_version_id += "dirty"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous editable suffix was more correct.

@@ -0,0 +1,150 @@
[build-system]
requires = ["setuptools>=61.0"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the recommended setuptools version was bupmed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM experimental NNCF PT Pull requests that updates NNCF PyTorch NNCF TF Pull requests that updates NNCF TensorFlow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants