-
Notifications
You must be signed in to change notification settings - Fork 84
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
Utilize Python build package instead of setup.py #5071
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
- name: Build the wheel (bdist_wheel) | ||
working-directory: ${{ inputs.repository }} | ||
shell: bash -l {0} | ||
run: | | ||
set -euxo pipefail | ||
source "${BUILD_ENV_FILE}" | ||
export PYTORCH_VERSION="$(${CONDA_RUN} pip show torch | grep ^Version: | sed 's/Version: *//' | sed 's/+.\+//')" | ||
${CONDA_RUN} python setup.py bdist_wheel | ||
${CONDA_RUN} python -m build --wheel -n |
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.
-n
stands for "--no-isolation", which we use b/c we already setup a conda environment.
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.
Nitpick: should use long form "--no-isolation" instead of "-n" for readability,
source "${BUILD_ENV_FILE}" | ||
${CONDA_RUN} python setup.py clean | ||
# shellcheck disable=SC2086 |
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.
Do we need these shellcheck commands? I just copied from above.
@atalman @seemethere Could we add a backward-compatible flag in this case so it could be set and the build command fallback to @spcyppt mentions that the new approach doesn't work with the FBGEMM atm, i.e. https://github.com/pytorch/FBGEMM/actions/runs/8623229536/job/23636003710#step:14:103. The team wants to have an option to keep the old way till they can figure out a fix after this upcoming release. @spcyppt I'm not sure how release branch cut is done on fbgemm, but you should consider using Nova workflows from the release branch https://github.com/pytorch/test-infra/tree/release/2.3 on fbgemm release instead of
This would shield your release from changes coming from Nova main branch. |
Summary: Changes in main does not work with our current script. pytorch/test-infra#5071 (comment) Change to use release branch of Nova workflow to guarantee that the workflow is stable throughout the release cycle. Differential Revision: D55947486
Summary: Changes in Nova framwork does not work with our current script. pytorch/test-infra#5071 (comment) Switch to use release branch of Nova workflow to guarantee that the workflow is stable throughout the release cycle. Differential Revision: D55947486
Summary: Changes in Nova framwork does not work with our current script. pytorch/test-infra#5071 (comment) Switch to use release branch of Nova workflow to guarantee that the workflow is stable throughout the release cycle. Differential Revision: D55947486
Thank you so much @huydhn! We will use the release branch of the Nova workflow for this upcoming release. |
@atalman We could also allow users to input their own build command with the default set to cc: @huydhn @seemethere |
Summary: Changes in Nova framwork does not work with our current script. pytorch/test-infra#5071 (comment) Switch to use release branch of Nova workflow to guarantee that the workflow is stable throughout the release cycle. Pull Request resolved: #2489 Reviewed By: q10, huydhn Differential Revision: D55947486 fbshipit-source-id: e12552776bccb443cd1983fc037e8c08715c0749
It was brought up [here](#5071 (comment)) that various workflows may still be reliant on the old way of installing python packages (invoking setup.py directly). While these workflows *should* enable their packages to work with `build` eventually, they should have ample time to do so. Therefore, this change established a default for the build command on Linux workflows. Therefore, newer packages can start to specify `python -m build --wheel`, without disrupting the other workflows reliant on Nova.
Summary: Changes in Nova framwork does not work with our current script. pytorch/test-infra#5071 (comment) Switch to use release branch of Nova workflow to guarantee that the workflow is stable throughout the release cycle. Pull Request resolved: #2489 Reviewed By: q10, huydhn Differential Revision: D55947486 fbshipit-source-id: e12552776bccb443cd1983fc037e8c08715c0749
This PR enable the build of M1 wheels to rely on `python -m build` instead of `python setup.py`. The default is still `python setup.py`. This is the same as #5071 and #5076 that were landed for linux wheels. We need this for torchcodec to support M1 wheels pytorch/torchcodec#230 CC @scotts @atalman
This PR enable the build of M1 wheels to rely on `python -m build` instead of `python setup.py`. The default is still `python setup.py`. This is the same as pytorch#5071 and pytorch#5076 that were landed for linux wheels. We need this for torchcodec to support M1 wheels pytorch/torchcodec#230 CC @scotts @atalman
Context
The current method by which NOVA workflows build Python wheels is deprecated. Instead of calling
python setup.py bdist_wheel
, all building should be done by the Python Packaging Authority's build tool. Here are a couple more links about the topic:This only fixes the NOVA Linux workflow, not macOS or Windows
Changelog
build
toolTesting
Considerations
build
is only compatible with Python>=3.8