Build Windows CPU wheel #753
Conversation
facebook-github-bot
added
the
CLA Signed
…into add-windows-cmake
|
Note for self: things are working OK in eaae823 |
| PUBLIC | ||
| "-fvisibility=hidden" | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
I am not entirely sure that this needs to be skipped on Windows. But the compilation is working so far. This may be the reason the tests were failing, I'm not sure. I'll investigate in a follow-up.
There was a problem hiding this comment.
As far as I know, -fvisibility=hidden is a GCC and Clang option, so if you are using a compiler that support MSVC style options (so cl.exe or the clang fronted clang-cl.exe) passing -fvisibility=hidden will raise an error as it is not a supported flag. Furthermore, visibility is always hidden by default on Windows/MSVC, so anyhow this should not be needed.
| PUBLIC | ||
| "LINKER:-undefined,dynamic_lookup" | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
Sort of a drive-by, but this shouldn't be needed for linux either, and those linker options won't work on windows anyway.
There was a problem hiding this comment.
Agreed, and I think you're applying a good principle: platform-specific tweaks should have a platform-specific guard.
| fail-fast: false | ||
| name: Build and Upload Windows wheel | ||
| # TODO: use @main | ||
| uses: nicolashug/test-infra/.github/workflows/build_wheels_windows.yml@build-platform-windows |
There was a problem hiding this comment.
We'll be able to use @main when pytorch/test-infra#6888 is merged. Reviewer, please assume pytorch/test-infra#6888 is accepted and merged when reviewing this PR (753)
| build-platform: "python-build-package" | ||
| # The BUILD_AGAINST_ALL_FFMPEG_FROM_S3 var, needed to build the wheel, is | ||
| # set in vc_env_helper.bat Couldn't find a way to set it from here. | ||
| build-command: "python -m build --wheel -vvv --no-isolation" |
There was a problem hiding this comment.
This command gets executed in test-infra as
${CONDA_RUN} ${ENV_SCRIPT} ${{ inputs.build-command }}
where ENV_SCRIPT is our own packaging/vc_env_helper.bat. Setting something like
build-command: "set BUILD_AGAINST_ALL_FFMPEG_FROM_S3=1 && python -m build --wheel -vvv --no-isolation"
doesn't work because, I think, vc_env_helper.bat expects a single command to be passed, not a succession of those. I couldn't find any easy way to make it work, so I gave up. Now the BUILD_AGAINST_ALL_FFMPEG_FROM_S3 is set within vc_env_helper.bat, which isn't great, but it works.
| install( | ||
| TARGETS ${all_libraries} | ||
| LIBRARY DESTINATION ${CMAKE_INSTALL_PREFIX} | ||
| RUNTIME DESTINATION ${CMAKE_INSTALL_PREFIX} # For Windows DLLs |
There was a problem hiding this comment.
| # 65535 symbols, which (apparently) will not work for CUDA. | ||
| # https://github.com/pytorch/pytorch/pull/3650 | ||
| set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) | ||
| endif() |
There was a problem hiding this comment.
This PR adds a job that builds CPU wheels on Windows. Example: https://github.com/pytorch/torchcodec/actions/runs/16706115347/job/47284447434?pr=753
It just builds the wheels! It doesn't test them. I had a job to test them, and it failed. We will need more work before we can fully support Windows, but it'll be easier to provide fixes as follow-ups.