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

Clean up setup.py in favor of pyproject.toml #4879

Merged
merged 19 commits into from
Sep 5, 2023

Conversation

justinchuby
Copy link
Contributor

@justinchuby justinchuby commented Feb 9, 2023

Description

  • Move much of the metadata from setup.py to pyproject.toml. Setuptools commands are left unchanged.
  • Replace --weekly_build with the env var ONNX_PREVIEW_BUILD. Version number verified locally and in CI.

    Note
    Due to pyproject.toml limitation, we need to use a script to rename the package for the weekly build.
    On Windows, it is (Get-Content -Path 'pyproject.toml') | ForEach-Object { $_ -replace 'name = "onnx"', 'name = "onnx-weekly"' } | Set-Content -Path 'pyproject.toml'
    On linux, it is sed -i 's/name = "onnx"/name = "onnx-weekly"/' 'pyproject.toml'
    Tested locally.

  • Replace all setup.py xxx usage with pip install or python -m build according to setuptools' suggestions.
  • The MacOS wheel's platform name needs to be tweaked. Previously it was modified by supplying the -p or --plat-name option to setup.py. Now that we use python -m build, I created an env var ONNX_WHEEL_PLATFORM_NAME and supply it via the setup call:
    setuptools.setup(
        ...
        options={"bdist_wheel": {"plat_name": ONNX_WHEEL_PLATFORM_NAME}}
        if ONNX_WHEEL_PLATFORM_NAME is not None
        else {},
    )
    

Motivation and Context

https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html

Starting with PEP 621, the Python community selected pyproject.toml as a standard way of specifying project metadata. Setuptools has adopted this standard and will use the information contained in this file as an input in the build process.

https://pypi.org/project/pytest-runner/

It is recommended that you:
Remove 'pytest-runner' from your setup_requires, preferably removing the setup_requires option.
Remove 'pytest' and any other testing requirements from tests_require, preferably removing the tests_requires option.
Select a tool to bootstrap and then run tests such as tox.

fix #4878 and #4539

@justinchuby justinchuby marked this pull request as ready for review February 9, 2023 20:58
@justinchuby justinchuby requested a review from a team as a code owner February 9, 2023 20:58
pyproject.toml Outdated Show resolved Hide resolved
@justinchuby
Copy link
Contributor Author

cannot import name 'TensorProto' from 'onnx' (unknown location)

package find may need update

@justinchuby justinchuby marked this pull request as draft February 9, 2023 21:03
@justinchuby
Copy link
Contributor Author

@jcwchen build_ext was not called in setup.py. How does it originally work?

@jcwchen
Copy link
Member

jcwchen commented Feb 9, 2023

@jcwchen build_ext was not called in setup.py. How does it originally work?

Good question. Although I am not very familiar with setuptools, I guess by default build_ext will be run under some circumstances?

@justinchuby
Copy link
Contributor Author

Looks like only macOS is failing. Could you trigger the release pipelines?

@justinchuby justinchuby marked this pull request as ready for review February 9, 2023 23:13
@justinchuby
Copy link
Contributor Author

[100%] Built target onnx_cpp2py_export
1862
copying /home/runner/work/onnx/onnx/.setuptools-cmake-build/onnx_cpp2py_export.cpython-310-x86_64-linux-gnu.so -> /home/runner/work/onnx/onnx/build/lib.linux-x86_64-3.10/onnx
1863
error: could not create '/home/runner/work/onnx/onnx/build/lib.linux-x86_64-3.10/onnx/onnx_cpp2py_export.cpython-310-x86_64-linux-gnu.so': No such file or directory

@justinchuby justinchuby marked this pull request as draft March 16, 2023 14:40
@justinchuby justinchuby marked this pull request as ready for review March 16, 2023 14:43
@jcwchen jcwchen added the run release CIs Use this label to trigger release tests in CI label Mar 16, 2023
@jcwchen
Copy link
Member

jcwchen commented Mar 16, 2023

Retrigger release CIs

@jcwchen jcwchen closed this Mar 16, 2023
@jcwchen jcwchen reopened this Mar 16, 2023
@justinchuby
Copy link
Contributor Author

              If you are seeing this warning it is very likely that a setuptools
              plugin or customization overrides the `build_py` command, without
              taking into consideration how editable installs run build steps
              starting from v64.0.0.
  
              Plugin authors and developers relying on custom build steps are encouraged
              to update their `build_py` implementation considering the information in
              https://setuptools.pypa.io/en/latest/userguide/extension.html
              about editable installs.
  
              For the time being `setuptools` will silence this error and ignore
              the faulty command, but this behaviour will change in future versions.

@justinchuby justinchuby marked this pull request as draft March 20, 2023 22:03
@justinchuby justinchuby closed this Apr 5, 2023
@justinchuby justinchuby reopened this Apr 5, 2023
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@github-advanced-security
Copy link

You have successfully added a new lintrunner configuration lintrunner. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
@justinchuby justinchuby added this to the 1.15 milestone Aug 31, 2023
Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchu@microsoft.com>
pyproject.toml Outdated Show resolved Hide resolved
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
@justinchuby
Copy link
Contributor Author

Bump @jcwchen

Copy link
Member

@jcwchen jcwchen left a comment

Choose a reason for hiding this comment

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

One nit comment. Others look good to me.

setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@jcwchen jcwchen added build Issues related to ONNX builds and packages CI pipelines Issues related to the CI pipeline labels Sep 5, 2023
@jcwchen
Copy link
Member

jcwchen commented Sep 5, 2023

To confirm, will this PR resolve #4539? Or it will need further fix?

@justinchuby
Copy link
Contributor Author

To confirm, will this PR resolve #4539? Or it will need further fix?

It will fix it

@jcwchen jcwchen linked an issue Sep 5, 2023 that may be closed by this pull request
@justinchuby justinchuby added this pull request to the merge queue Sep 5, 2023
@justinchuby
Copy link
Contributor Author

justinchuby added a commit that referenced this pull request Sep 5, 2023
### Description
<!-- - Describe your changes. -->

- Move much of the metadata from setup.py to pyproject.toml. Setuptools
commands are left unchanged.
- Replace `--weekly_build` with the env var `ONNX_PREVIEW_BUILD`.
Version number verified locally and in CI.
	> **Note**
> Due to `pyproject.toml` limitation, we need to use a script to rename
the package for the weekly build.
> On Windows, it is `(Get-Content -Path 'pyproject.toml') |
ForEach-Object { $_ -replace 'name = "onnx"', 'name = "onnx-weekly"' } |
Set-Content -Path 'pyproject.toml'`
> On linux, it is `sed -i 's/name = "onnx"/name = "onnx-weekly"/'
'pyproject.toml'`
	> Tested locally.
- Replace all `setup.py xxx` usage with `pip install` or `python -m
build` according to setuptools' suggestions.
- The MacOS wheel's platform name needs to be tweaked. Previously it was
modified by supplying the `-p` or `--plat-name` option to `setup.py`.
Now that we use `python -m build`, I created an env var
`ONNX_WHEEL_PLATFORM_NAME` and supply it via the `setup` call:
    ```
	setuptools.setup(
	    ...
	    options={"bdist_wheel": {"plat_name": ONNX_WHEEL_PLATFORM_NAME}}
	    if ONNX_WHEEL_PLATFORM_NAME is not None
	    else {},
	)
	```

### Motivation and Context
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->

https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html

> Starting with [PEP 621](https://peps.python.org/pep-0621/), the Python
community selected pyproject.toml as a standard way of specifying
project metadata. Setuptools has adopted this standard and will use the
information contained in this file as an input in the build process.

https://pypi.org/project/pytest-runner/

> It is recommended that you:
> Remove 'pytest-runner' from your setup_requires, preferably removing
the setup_requires option.
> Remove 'pytest' and any other testing requirements from tests_require,
preferably removing the tests_requires option.
> Select a tool to bootstrap and then run tests such as tox.

fix #4878 and #4539

---------

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Merged via the queue into onnx:main with commit 8e2a216 Sep 5, 2023
53 checks passed
@justinchuby justinchuby deleted the justinchu/cleanup-setup branch September 5, 2023 16:42
Merged via the queue into onnx:main with commit bd60a39 Sep 5, 2023
53 checks passed
@justinchuby
Copy link
Contributor Author

justinchuby commented Sep 5, 2023

I misspoke - editable build support will require #5558 @jcwchen

corwinjoy pushed a commit to corwinjoy/onnx that referenced this pull request Sep 5, 2023
### Description
<!-- - Describe your changes. -->

- Move much of the metadata from setup.py to pyproject.toml. Setuptools
commands are left unchanged.
- Replace `--weekly_build` with the env var `ONNX_PREVIEW_BUILD`.
Version number verified locally and in CI.
	> **Note**
> Due to `pyproject.toml` limitation, we need to use a script to rename
the package for the weekly build.
> On Windows, it is `(Get-Content -Path 'pyproject.toml') |
ForEach-Object { $_ -replace 'name = "onnx"', 'name = "onnx-weekly"' } |
Set-Content -Path 'pyproject.toml'`
> On linux, it is `sed -i 's/name = "onnx"/name = "onnx-weekly"/'
'pyproject.toml'`
	> Tested locally.
- Replace all `setup.py xxx` usage with `pip install` or `python -m
build` according to setuptools' suggestions.
- The MacOS wheel's platform name needs to be tweaked. Previously it was
modified by supplying the `-p` or `--plat-name` option to `setup.py`.
Now that we use `python -m build`, I created an env var
`ONNX_WHEEL_PLATFORM_NAME` and supply it via the `setup` call:
    ```
	setuptools.setup(
	    ...
	    options={"bdist_wheel": {"plat_name": ONNX_WHEEL_PLATFORM_NAME}}
	    if ONNX_WHEEL_PLATFORM_NAME is not None
	    else {},
	)
	```

### Motivation and Context
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->

https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html

> Starting with [PEP 621](https://peps.python.org/pep-0621/), the Python
community selected pyproject.toml as a standard way of specifying
project metadata. Setuptools has adopted this standard and will use the
information contained in this file as an input in the build process.

https://pypi.org/project/pytest-runner/

> It is recommended that you:
> Remove 'pytest-runner' from your setup_requires, preferably removing
the setup_requires option.
> Remove 'pytest' and any other testing requirements from tests_require,
preferably removing the tests_requires option.
> Select a tool to bootstrap and then run tests such as tox.

fix onnx#4878 and onnx#4539

---------

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Corwin Joy <corwinjoy@gmail.com>
corwinjoy pushed a commit to corwinjoy/onnx that referenced this pull request Sep 5, 2023
### Description
<!-- - Describe your changes. -->

- Move much of the metadata from setup.py to pyproject.toml. Setuptools
commands are left unchanged.
- Replace `--weekly_build` with the env var `ONNX_PREVIEW_BUILD`.
Version number verified locally and in CI.
	> **Note**
> Due to `pyproject.toml` limitation, we need to use a script to rename
the package for the weekly build.
> On Windows, it is `(Get-Content -Path 'pyproject.toml') |
ForEach-Object { $_ -replace 'name = "onnx"', 'name = "onnx-weekly"' } |
Set-Content -Path 'pyproject.toml'`
> On linux, it is `sed -i 's/name = "onnx"/name = "onnx-weekly"/'
'pyproject.toml'`
	> Tested locally.
- Replace all `setup.py xxx` usage with `pip install` or `python -m
build` according to setuptools' suggestions.
- The MacOS wheel's platform name needs to be tweaked. Previously it was
modified by supplying the `-p` or `--plat-name` option to `setup.py`.
Now that we use `python -m build`, I created an env var
`ONNX_WHEEL_PLATFORM_NAME` and supply it via the `setup` call:
    ```
	setuptools.setup(
	    ...
	    options={"bdist_wheel": {"plat_name": ONNX_WHEEL_PLATFORM_NAME}}
	    if ONNX_WHEEL_PLATFORM_NAME is not None
	    else {},
	)
	```

### Motivation and Context
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->

https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html

> Starting with [PEP 621](https://peps.python.org/pep-0621/), the Python
community selected pyproject.toml as a standard way of specifying
project metadata. Setuptools has adopted this standard and will use the
information contained in this file as an input in the build process.

https://pypi.org/project/pytest-runner/

> It is recommended that you:
> Remove 'pytest-runner' from your setup_requires, preferably removing
the setup_requires option.
> Remove 'pytest' and any other testing requirements from tests_require,
preferably removing the tests_requires option.
> Select a tool to bootstrap and then run tests such as tox.

fix onnx#4878 and onnx#4539

---------

Signed-off-by: Justin Chu <justinchu@microsoft.com>
Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com>
Signed-off-by: Corwin Joy <corwinjoy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to ONNX builds and packages CI pipelines Issues related to the CI pipeline run release CIs Use this label to trigger release tests in CI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

pip install in an environment without setuptools fails Build error with the latest setuptool
2 participants