-
Notifications
You must be signed in to change notification settings - Fork 24.9k
[build] pin setuptools>=77
to enable PEP 639
#158104
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
base: gh/XuehaiPan/365/base
Are you sure you want to change the base?
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/158104
Note: Links to docs will display an error until the docs builds have been completed. ❌ 9 New Failures, 2 Cancelled Jobs, 16 Pending, 1 Unrelated FailureAs of commit 75c8c6c with merge base b219ca2 ( NEW FAILURES - The following jobs have failed:
CANCELLED JOBS - The following jobs were cancelled. Please retry:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
ghstack-source-id: 9a3c1e9 Pull-Request: pytorch#158104
ghstack-source-id: b1f18b3 Pull-Request: pytorch#158104
Adding |
ghstack-source-id: 8ffb2cf Pull-Request: pytorch#158104
ghstack-source-id: 79378ae Pull-Request: pytorch#158104
ghstack-source-id: 443804a Pull-Request: pytorch#158104
Since PyTorch 2.8.0 has been released, I wonder if there are any blockers or unresolved concerns about this PR? cc @albanD |
To me there are two main concers:
|
See the PR description #158104 (comment), we will need to pin either
I will try to remove the pins as much as I can in this PR, and then get back to you. |
ghstack-source-id: 005d39e Pull-Request: pytorch#158104
ghstack-source-id: 8429e5c Pull-Request: pytorch#158104
ghstack-source-id: d199478 Pull-Request: pytorch#158104
ghstack-source-id: 991eb1c Pull-Request: pytorch#158104
ghstack-source-id: f19b9a7 Pull-Request: pytorch#158104
ghstack-source-id: a8e74a0 Pull-Request: pytorch#158104
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.
I would like to get someone from Infra, either @atalman or @seemethere or @malfet comment on adding a new "pip install -r requirements-build.txt" to all the build/test scripts. It feels to me like these are at least partially redundant and we should remove whatever is trying (and failing) to install these dependencies if we're adding this now.
Also a couple nits to keep everything <80 to avoid changing the build frontend between easy_install and pip.
@@ -16,7 +17,7 @@ if "%DESIRED_PYTHON%" == "3.9" %PYTHON_EXEC% -m pip install numpy==2.0.2 cmake | |||
|
|||
%PYTHON_EXEC% -m pip install pyyaml | |||
%PYTHON_EXEC% -m pip install mkl-include mkl-static | |||
%PYTHON_EXEC% -m pip install boto3 ninja typing_extensions setuptools==72.1.0 | |||
%PYTHON_EXEC% -m pip install boto3 ninja typing-extensions setuptools packaging |
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.
Why not use the requirements-build.txt file for this one?
@@ -26,7 +26,7 @@ pytest-xdist==3.3.1 | |||
pytest==7.3.2 | |||
pyyaml==6.0.2 | |||
scipy==1.12.0 | |||
setuptools==72.1.0 | |||
setuptools==80.9.0 |
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.
Let's keep it <80
# 77.0.0: min version for SPDX expression support for project.license | ||
"setuptools>=70.1.0,<80.0", | ||
"setuptools>=77.0.0,<80.0", |
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.
Aren't these duplicate of requirements-build.txt? Can we remove one of the two?
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.
It feels like we have an explosion of requirements-something-or-other
files in PyTorch repo
May be before we start relying using another one, propose a separate PR that adds comment to each of them explaining when they should vs should not be used
Also, at least for linux, to avoid waisting CI resources, all requirements should be installed as part of the container setup, defined here: .ci/docker/requirements-ci.txt
And I know it's way too late to complain, but I'm confused about the purpose of #158111 Requirements should be installed once, if we need custom test configs that lower or raise versions, it should be done differently |
PEP 639: peps.python.org/pep-0639
setuptools
v77 adds PEP 639 support for SPDX expression aspackage.license
inpyproject.toml
and also deprecated TOML-table-basedpackage.license
. That depression will be converted to an error in the future.Note that
setuptools<77
does not support SPDX-basedpackage.license
whilesetuptools>=77
will error for non-SPDX-basedpackage.license
in the future. We need an explicit version pin forsetuptools>=77
(or pinsetuptools<77
).See also:
project.license
deprecation if v77 unavailable pypa/setuptools#4903Stack from ghstack (oldest at bottom):
setuptools<80.0
#156049setuptools>=77
to enable PEP 639 #158104cc @malfet @seemethere @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela