Skip to content

Replace setup.py bdist_wheel with python -m build --wheel #156712

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

Draft
wants to merge 7 commits into
base: gh/zklaus/11/base
Choose a base branch
from

Conversation

zklaus
Copy link
Collaborator

@zklaus zklaus commented Jun 24, 2025

Previously we already replaced most use of python setup.py develop/install.

This PR also replaces the use of setup.py bdist_wheel with the modern python -m build --wheel alternative.

Stack from ghstack (oldest at bottom):

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Jun 24, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/156712

Note: Links to docs will display an error until the docs builds have been completed.

❌ 13 New Failures

As of commit b11c3ac with merge base c184cb3 (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[ghstack-poisoned]
zklaus added a commit to zklaus/pytorch that referenced this pull request Jun 25, 2025
zklaus added 3 commits August 11, 2025 13:00
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
else
# Explicitly set USE_DISTRIBUTED=0 to align with the default build config on mac. This also serves as the sole CI config that tests
# that building with USE_DISTRIBUTED=0 works at all. See https://github.com/pytorch/pytorch/issues/86448
USE_DISTRIBUTED=0 USE_OPENMP=1 MACOSX_DEPLOYMENT_TARGET=11.0 WERROR=1 BUILD_TEST=OFF USE_PYTORCH_METAL=1 python setup.py bdist_wheel --plat-name macosx_11_0_arm64
USE_DISTRIBUTED=0 USE_OPENMP=1 MACOSX_DEPLOYMENT_TARGET=11.0 WERROR=1 BUILD_TEST=OFF USE_PYTORCH_METAL=1 python -m build --wheel --no-isolation -C--build-option=--plat-name=macosx_11_0_arm64
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, build 1.3.0 was released last week, which added a new --config-json option. That might be better than --config-setting / -C.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, that's an interesting new facility. It seems clear that we could move to --config-json="{'--build-option': '--platform-name=macosx_11_0_arm64'}", but given the support in setuptools, I am afraid we can't go all the way to --config-json="{'build-option': {'platform-name': 'macosx_11_0_arm64'}}". With the lack of support in older build versions, I think the best way forward is to stick with --config-setting for now and to reevaluate with a larger build system overhaul.

@@ -10,6 +10,11 @@ boto3==1.35.42
#Pinned versions: 1.19.12, 1.16.34
#test that import:

build
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
build
build==1.3.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As long as we stick with --config-setting/-C, I think we can leave this pin open.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is used to keep our CI reproducible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense and I have no objection to pinning. However, not all packages in the file are pinned, c.f. click, psutil, pyyaml, and others. How is it decided which ones to pin and which ones to leave open? And when should the actual pin deviate from the comment as with fbscribelogger and others?

zklaus added 2 commits August 11, 2025 18:18
[ghstack-poisoned]
[ghstack-poisoned]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants