Repository navigation
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/191629
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (4 Unrelated Failures)As of commit ad84124 with merge base 3022368 ( NEW FAILURES SUPPRESSED BY AI (non-blocking) - The following jobs failed, but the AI CI Advisor judged the failures not to be evidence against this change:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
ScottTodd
left a comment
There was a problem hiding this comment.
lgtm from ROCm / TheRock's perspective
| # MSVC OpenMP (libomp140) - TheRock copies this into torch/lib/ before the | ||
| # wheel build when using MSVC OpenMP instead of MKL libiomp. | ||
| set(_libomp140 "${TORCH_SRC_DIR}/lib/libomp140.x86_64.dll") | ||
| if(EXISTS "${_libomp140}") | ||
| install(FILES "${_libomp140}" DESTINATION "${TORCH_INSTALL_LIB_DIR}") | ||
| endif() |
There was a problem hiding this comment.
Similar to #191625, we could consider a USE_ROCM help here to limit when others building pytorch would trigger this code path. Looking at how libiomp5md.dll is used above though, I think not adding a USE_ROCM check is probably fine?
Note that I'm not thrilled about using OpenMP from MSVC in our downstream build scripts, see the comments at https://github.com/ROCm/TheRock/blob/6b2138632e722857796e4b662c64eec7ae21a3ce/external-builds/pytorch/build_prod_wheels.py#L905-L940
There was a problem hiding this comment.
Agreed — I left it without a USE_ROCM guard to match the libiomp5md.dll block above. The EXISTS "${TORCH_SRC_DIR}/lib/libomp140.x86_64.dll" check already scopes this to builds that actually staged that DLL (i.e. TheRock's MSVC-OpenMP path), so non-ROCm builds won't be affected. Point taken on the broader MSVC-OpenMP concern in the TheRock build scripts; I'll follow up on that separately on the TheRock side rather than in this wheel-packaging fix.
| # _rocm_init.py is generated by TheRock/ROCm packaging before the wheel build | ||
| # and gitignored. scikit-build-core's package walk skips it unless we install | ||
| # it explicitly (same pattern as version.py above). | ||
| if(EXISTS "${TORCH_SRC_DIR}/_rocm_init.py") | ||
| install(FILES "${TORCH_SRC_DIR}/_rocm_init.py" DESTINATION ".") | ||
| endif() |
There was a problem hiding this comment.
FYI this change is also made by #191625, so you may have merge conflicts depending on review/merge ordering.
There was a problem hiding this comment.
… PRs land (#7009) ## Motivation This passes scikit-build-core force-include settings for our generated and .gitignore'd `_rocm_init.py` file and Windows OpenMP runtime files. * Local patch for #6965 * This should be compatible with * pytorch/pytorch#191625 * pytorch/pytorch#191632 * pytorch/pytorch#191629 ## Technical Details * `python -m build` backend configuration (https://build.pypa.io/en/stable/how-to/config-settings.html) documents `-C` as shorthand for `--config-setting`. It forwards `KEY=VALUE` settings to the selected PEP 517 backend. * scikit-build-core configuration (https://scikit-build-core.readthedocs.io/en/stable/configuration/#configuration) says every setting can be supplied through `pyproject.toml`, PEP 517 config settings, or environment variables. Its examples use dotted CLI keys such as `-Ccmake.build-type=Debug` (see https://scikit-build-core.readthedocs.io/en/stable/reference/configs.html#confval-cmake.build-type) * scikit-build-core’s `wheel.force-include` section (https://scikit-build-core.readthedocs.io/en/stable/configuration/#force-including-files) documents the source-to-destination mapping and notes that exact force-includes override exclusions. ## Test Plan Trigger builds of pytorch on linux and windows using 'nightly', look for `_rocm_init.py` and library load succeeding | Config | Logs | Result | -- | -- | -- Linux 'nightly' | https://github.com/ROCm/TheRock/actions/runs/30588591418 | Pending Linux 'release/2.12' | https://github.com/ROCm/TheRock/actions/runs/30589526077 | Pending Windows 'nightly' | https://github.com/ROCm/TheRock/actions/runs/30588680552 | Pending ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md. Co-authored-by: Codex <codex@openai.com>
jithunnair-amd
left a comment
There was a problem hiding this comment.
Looks like this change is using an established precedent for dlls.
My concern was the specificity of the 140 in libomp140.x86_64.dll, but this is what my search turned up:
It indicates that the DLL uses the VC14 (Visual C++ 14.x) ABI, not a particular compiler release. Thanks to Microsoft's binary compatibility promise, the same ABI is shared across:
Visual Studio 2015 (v140)
Visual Studio 2017 (v141)
Visual Studio 2019 (v142)
Visual Studio 2022 (v143)
So a libomp140.x86_64.dll might have been built with Visual Studio 2022 and still be named libomp140.x86_64.dll.
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
049ccf3 to
ab24940
Compare
scikit-build-core drops TheRock-staged torch/lib/libomp140.x86_64.dll from the Windows ROCm wheel. Install it via CMake when present, mirroring the existing libiomp5md.dll / uv.dll handling. EXISTS-guarded, so non-ROCm and non-MSVC-OpenMP builds are unaffected. Fixes ROCm/TheRock#6965
ab24940 to
ad84124
Compare
|
|
|
@pytorchbot merge -i |
Merge failedReason: This PR needs a If not, please add the To add a label, you can comment to pytorchbot, for example For more information, see Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -i |
Merge startedYour change will be merged while ignoring the following 4 checks: trunk / macos-py3-arm64 / test (default, 3, 3, macos-m1-stable), linux-aarch64-binary-manywheel / manywheel-cuda-aarch64-cu130-build, linux-aarch64-binary-manywheel / manywheel-cuda-aarch64-cu134-build, linux-aarch64-binary-manywheel / manywheel-cuda-aarch64-cu132-build Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Both PRs add a Windows DLL install block at the same spot in cmake/PostBuildSteps.cmake. Keep both: the aotriton_v2/liblzma OPTIONAL install from this PR, then the libomp140 install from pytorch#191629, the same order as the release/2.14 cherry-pick (#3835). Test Plan: a standalone CMake project that includes the resolved if(WIN32 AND BUILD_PYTHON) block, with the FA DLLs generated at build time by a custom target, configured, built and installed with Ninja: all three DLLs installed when present; only the FA DLLs when libomp140 is absent; nothing and no error when none exist. Authored with Claude.
…w upstream) (#9058) ## Problem TheRock force-includes `aotriton_v2.dll` and its `liblzma.dll` dependency into the Windows ROCm wheel as a scikit-build-core workaround. pytorch/pytorch#200931 installs them via CMake instead, so the force-include becomes redundant. ## Changes Remove the `aotriton_v2.dll` + `liblzma.dll` force-includes from `build_prod_wheels.py`. The `libomp140.x86_64.dll` force-include stays until its own follow-up. ## Validation https://github.com/ROCm/TheRock/actions/runs/38930228545 ran this PR's head (`429936c345`) against release/2.14 with the install rule cherry-picked (ROCm/pytorch#3835). With the force-include gone, the DLLs could only ship via the CMake rule; build and the `import torch` sanity check passed. Merge only after pytorch/pytorch#200931 is on main and ROCm/pytorch#3835 / ROCm/pytorch#3836 are on release/2.14 / release/2.15; any scikit-build-core branch without the install rule would lose the DLLs (WinError 126). release/2.13 uses setuptools and is unaffected. Merges cleanly alongside #9055. Related PR: - pytorch/pytorch#191629: install `libomp140.x86_64.dll` via CMake - pytorch/pytorch#200931: install `aotriton_v2.dll` + `liblzma.dll` via CMake - #9055: drop the `_rocm_init.py` wheel force-include - #9058: drop the `aotriton_v2.dll` + `liblzma.dll` wheel force-includes (this PR) - ROCm/pytorch#3835: release/2.14 cherry-pick of pytorch/pytorch#191629 + pytorch/pytorch#200931 - ROCm/pytorch#3836: release/2.15 cherry-pick of pytorch/pytorch#191629 + pytorch/pytorch#200931 - Issue: #6965 Tracked in AIPYTORCH-1516.
…uild ROCm wheels (#200931) When Flash Attention is built on Windows, `torch_hip.dll` needs `aotriton_v2.dll` and its `liblzma.dll` dependency. aotriton's ExternalProject generates these into `torch/lib/` during the build (after CMake configure) with no install rule, so scikit-build-core omits them from the wheel and `import torch` fails with WinError 126. This adds CMake install rules for both DLLs with `OPTIONAL`, so the existence check runs at install time, after the build has produced them. A configure-time `EXISTS` guard would be false and silently drop them. Flash-Attention-off and non-ROCm builds are unaffected because the files won't exist at install time. It replaces the temporary force-include TheRock carries. Validation: https://github.com/ROCm/TheRock/actions/runs/38930228545 built a Windows ROCm wheel (gfx1100, py3.12) from release/2.14 with this change cherry-picked (ROCm#3835, identical cmake) and with TheRock's aotriton/liblzma force-include removed (ROCm/TheRock#9058), so the DLLs could only ship via this rule. Build and the `import torch` sanity check passed. The earlier `EXISTS`-guarded revision failed that check with WinError 126 (https://github.com/ROCm/TheRock/actions/runs/38924233036). Related PR: - #191629: install `libomp140.x86_64.dll` via CMake - #200931: install `aotriton_v2.dll` + `liblzma.dll` via CMake (this PR) - ROCm/TheRock#9055: drop the `_rocm_init.py` wheel force-include - ROCm/TheRock#9058: drop the `aotriton_v2.dll` + `liblzma.dll` wheel force-includes - ROCm/TheRock#9360: drop the `libomp140.x86_64.dll` wheel force-include - ROCm#3835: release/2.14 cherry-pick of #191629 + #200931 - ROCm#3836: release/2.15 cherry-pick of #191629 + #200931 - Issue: ROCm/TheRock#6965 Fixes ROCm/TheRock#6965 Supported by @jeffdaily Tracked in AIPYTORCH-1516. Pull Request resolved: #200931 Approved by: https://github.com/ScottTodd, https://github.com/jeffdaily Co-authored-by: Jeff Daily <jeff.daily@amd.com>
… libomp140 (#3835) Cherry-picks the two upstream Windows ROCm wheel DLL install fixes onto `release/2.14` (scikit-build-core backend): `aotriton_v2.dll` + `liblzma.dll` (pytorch#200931) and `libomp140.x86_64.dll` (pytorch#191629). Without them, once TheRock's temporary force-includes are removed the DLLs drop out of the wheel and `import torch` fails with WinError 126. The aotriton/liblzma install uses `OPTIONAL` so the existence check runs at install time, because aotriton's ExternalProject only generates those DLLs during the build. ### Validation https://github.com/ROCm/TheRock/actions/runs/38930228545 built this branch's head (`268bf8ed4be`) with ROCm/TheRock#9058 (aotriton/liblzma force-include removed), so those DLLs could only ship via the install rule. Build and the `import torch` sanity check passed. Related PR: - pytorch#191629: install `libomp140.x86_64.dll` via CMake - pytorch#200931: install `aotriton_v2.dll` + `liblzma.dll` via CMake - ROCm/TheRock#9055: drop the `_rocm_init.py` wheel force-include - ROCm/TheRock#9058: drop the `aotriton_v2.dll` + `liblzma.dll` wheel force-includes - ROCm/TheRock#9360: drop the `libomp140.x86_64.dll` wheel force-include - #3835: release/2.14 cherry-pick of pytorch#191629 + pytorch#200931 (this PR) - #3836: release/2.15 cherry-pick of pytorch#191629 + pytorch#200931 - Issue: ROCm/TheRock#6965 Tracked in AIPYTORCH-1516.
… libomp140 (#3836) Cherry-picks the two upstream Windows ROCm wheel DLL install fixes onto `release/2.15` (scikit-build-core backend): `aotriton_v2.dll` + `liblzma.dll` (pytorch#200931) and `libomp140.x86_64.dll` (pytorch#191629). Without them, once TheRock's temporary force-includes are removed the DLLs drop out of the wheel and `import torch` fails with WinError 126. The aotriton/liblzma install uses `OPTIONAL` so the existence check runs at install time, because aotriton's ExternalProject only generates those DLLs during the build. ### Validation Branch merged with current `release/2.15` (now includes `related_commits` from #3816). Windows ROCm wheel build (gfx1100, py3.12) on this head (`a119523c5bc`) with ROCm/TheRock#9360, which passes no force-include flags, so every DLL can only ship via CMake install: https://github.com/ROCm/TheRock/actions/runs/39287495719. Build and the `import torch` sanity check passed. Related PR: - pytorch#191629: install `libomp140.x86_64.dll` via CMake - pytorch#200931: install `aotriton_v2.dll` + `liblzma.dll` via CMake - ROCm/TheRock#9055: drop the `_rocm_init.py` wheel force-include - ROCm/TheRock#9058: drop the `aotriton_v2.dll` + `liblzma.dll` wheel force-includes - ROCm/TheRock#9360: drop the `libomp140.x86_64.dll` wheel force-include - #3835: release/2.14 cherry-pick of pytorch#191629 + pytorch#200931 - #3836: release/2.15 cherry-pick of pytorch#191629 + pytorch#200931 (this PR) - Issue: ROCm/TheRock#6965 Tracked in AIPYTORCH-1516.
… libomp140 (#3836) Cherry-picks the two upstream Windows ROCm wheel DLL install fixes onto `release/2.15` (scikit-build-core backend): `aotriton_v2.dll` + `liblzma.dll` (pytorch#200931) and `libomp140.x86_64.dll` (pytorch#191629). Without them, once TheRock's temporary force-includes are removed the DLLs drop out of the wheel and `import torch` fails with WinError 126. The aotriton/liblzma install uses `OPTIONAL` so the existence check runs at install time, because aotriton's ExternalProject only generates those DLLs during the build. ### Validation Branch merged with current `release/2.15` (now includes `related_commits` from #3816). Windows ROCm wheel build (gfx1100, py3.12) on this head (`a119523c5bc`) with ROCm/TheRock#9360, which passes no force-include flags, so every DLL can only ship via CMake install: https://github.com/ROCm/TheRock/actions/runs/39287495719. Build and the `import torch` sanity check passed. Related PR: - pytorch#191629: install `libomp140.x86_64.dll` via CMake - pytorch#200931: install `aotriton_v2.dll` + `liblzma.dll` via CMake - ROCm/TheRock#9055: drop the `_rocm_init.py` wheel force-include - ROCm/TheRock#9058: drop the `aotriton_v2.dll` + `liblzma.dll` wheel force-includes - ROCm/TheRock#9360: drop the `libomp140.x86_64.dll` wheel force-include - #3835: release/2.14 cherry-pick of pytorch#191629 + pytorch#200931 - #3836: release/2.15 cherry-pick of pytorch#191629 + pytorch#200931 (this PR) - Issue: ROCm/TheRock#6965 Tracked in AIPYTORCH-1516.
After the scikit-build-core wheel migration, the Windows ROCm wheel drops
torch/lib/libomp140.x86_64.dll, which TheRock stages for MSVC OpenMP. This installs it via CMake when present, mirroring the existinglibiomp5md.dll/uv.dllhandling. TheRock copies the DLL intotorch/lib/before CMake configure, so theEXISTSguard sees it, and non-ROCm builds are unaffected. It replaces the temporary force-include TheRock carries.Validation: https://github.com/ROCm/TheRock/actions/runs/38930228545 built a Windows ROCm wheel (gfx1100, py3.12) from release/2.14 with this change cherry-picked (ROCm#3835,
268bf8ed4be); build and theimport torchsanity check passed. TheRock still force-includes libomp140 in that build, so it confirms no regression rather than exercising this rule in isolation.Related PR:
libomp140.x86_64.dllvia CMake (this PR)aotriton_v2.dll+liblzma.dllvia CMake_rocm_init.pywheel force-includeaotriton_v2.dll+liblzma.dllwheel force-includeslibomp140.x86_64.dllwheel force-include_rocm_init.py. Windows fails to loadc10_hip.dll, Linux fails to loadlibMIOpen.so.1ROCm/TheRock#6965Fixes ROCm/TheRock#6965
Supported by @jeffdaily
Tracked in AIPYTORCH-1516.
cc @jeffdaily @sunway513 @jithunnair-amd @pruthvistony @ROCmSupport @jataylo @hongxiayang @naromero77amd @pragupta @jerrymannil @xinyazhang