Skip to content

Fix setup.py to skip CPU kernels on Windows #3187

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 5, 2025
Merged

Fix setup.py to skip CPU kernels on Windows #3187

merged 6 commits into from
Nov 5, 2025

Conversation

@Xia-Weiwen
Copy link
Collaborator

@Xia-Weiwen Xia-Weiwen commented Oct 16, 2025 •
edited
Loading

Fixes #3175

CPU kernel files in the torchao/csrc/cpu/aten_kernels/ directory are not skipped correctly on Windows because the path string matching fails (\ vs /). This PR fixes this issue.

Test plan
We have verified this patch in our local environment (Processor: 12th Gen Intel(R) Core(TM) i7-1270P, OS: Windows 11 Enterprise).

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 16, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 8036d0e with merge base 9266734 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 16, 2025
@Xia-Weiwen Xia-Weiwen added topic: not user facing Use this tag if you don't want this PR to show up in release notes and removed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 16, 2025
@Xia-Weiwen Xia-Weiwen requested a review from vkuzo October 16, 2025 05:08
@Xia-Weiwen Xia-Weiwen marked this pull request as ready for review October 16, 2025 05:08
@Xia-Weiwen Xia-Weiwen mentioned this pull request Oct 16, 2025
Closed
@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 16, 2025
@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 17, 2025

CI is broken. Failures are not related to this PR.

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 17, 2025

@mingfeima Please review. Thanks.

@Xia-Weiwen Xia-Weiwen requested a review from jerryzh168 October 17, 2025 01:17
@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 20, 2025

Hi @vkuzo @jerryzh168 @metascroy @mingfeima @Valentine233 Please review this PR. Thanks.

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 22, 2025

Hi @vkuzo @jerryzh168 @metascroy Can you please review this PR? Thanks.

1 similar comment
@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 23, 2025

Hi @vkuzo @jerryzh168 @metascroy Can you please review this PR? Thanks.

@vkuzo
Copy link
Contributor

vkuzo commented Oct 23, 2025

could you add a test plan detailing how this was tested for correctness? Thank you!

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 24, 2025 •
edited
Loading

could you add a test plan detailing how this was tested for correctness? Thank you!

Hi @vkuzo If there is a CI workflow that builds Torchao on Windows, this will be tested. Do you have a plan to enable that in CI? Thanks.

@Xia-Weiwen Xia-Weiwen mentioned this pull request Oct 24, 2025
Closed
@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 24, 2025

Hi @vkuzo I have submitted an issue here #3237 Can we add such a workflow in CI to test it? Thanks.

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 27, 2025

Hi @vkuzo How do you think about adding Windows building in CI? Or do you have more suggestions? Thanks.

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 28, 2025

Hi @metascroy @vkuzo @jerryzh168 @cpuhrsch @atalman @msaroufim @janeyx99 @drisspg Could you please review this PR? Thanks.

@vkuzo
Copy link
Contributor

vkuzo commented Oct 28, 2025

yeah, I think having XPU in CI would help with more confidence on PRs like this one, it's not ideal to rely on other people testing the changes without a clear way to reproduce them

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 28, 2025

yeah, I think having XPU in CI would help with more confidence on PRs like this one, it's not ideal to rely on other people testing the changes without a clear way to reproduce them

The change is not about XPU but Windows. Do you mean Windows in CI? Do you have any suggestions on adding a workflow to test building on Windows? Thanks.

@Xia-Weiwen Xia-Weiwen mentioned this pull request Oct 29, 2025
Open
@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 29, 2025 •
edited
Loading

Hi @vkuzo I understand setting up a workflow for Windows build in CI is not a simple task for you. We have verified this patch on our local machine. I have also update the summary above. Do you think we can land it now? Thanks.

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 30, 2025

Hi @vkuzo Would you mind if we land this? This fix is needed to resolve #3175 . Without this fix, building on Windows will still fail even with 30082cb . And we have verified the fix in our local environment. Thanks.

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Oct 31, 2025

Hi @metascroy @vkuzo @jerryzh168 @cpuhrsch @atalman @msaroufim @janeyx99 @drisspg do you think we can land this now? Thanks.

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Nov 3, 2025

Hi @metascroy @vkuzo @jerryzh168 @cpuhrsch @atalman @msaroufim @janeyx99 @drisspg Would you mind if we land this now? Thanks.

@Xia-Weiwen
Copy link
Collaborator Author

Xia-Weiwen commented Nov 3, 2025

Hi @metascroy @vkuzo @jerryzh168 @cpuhrsch @atalman @msaroufim @janeyx99 @drisspg Would you mind if we land this now? Thanks.

It's a bug in setup.py and I guess you want to fix it too.

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 4, 2025

Warning: Unknown label ciflow/binaries/all.
Currently recognized labels are

  • ciflow/benchmark
  • ciflow/tutorials
  • ciflow/rocm
  • ciflow/4xh100
  • ciflow/xpu

Please add the new label to .github/pytorch-probot.yml

@Xia-Weiwen Xia-Weiwen merged commit 1c89061 into pytorch:main Nov 5, 2025
37 of 38 checks passed
@Xia-Weiwen Xia-Weiwen deleted the fix_build_win branch November 5, 2025 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atalman atalman atalman approved these changes

@Valentine233 Valentine233 Valentine233 approved these changes

@vkuzo vkuzo Awaiting requested review from vkuzo

@metascroy metascroy Awaiting requested review from metascroy

@jerryzh168 jerryzh168 Awaiting requested review from jerryzh168

@cpuhrsch cpuhrsch Awaiting requested review from cpuhrsch

@msaroufim msaroufim Awaiting requested review from msaroufim

@janeyx99 janeyx99 Awaiting requested review from janeyx99

@drisspg drisspg Awaiting requested review from drisspg

+1 more reviewer

@mingfeima mingfeima mingfeima approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ciflow/binaries/all CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: not user facing Use this tag if you don't want this PR to show up in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Windows][build]it will meet build failures when build with pytorch xpu version on windows

5 participants

@Xia-Weiwen @vkuzo @atalman @mingfeima @Valentine233