🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 4eb607d with merge base 6c5227b ():

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

@pytorchbot label "topic: not user facing"

hip-sdk isn't the only windows build. we should avoid adding if(UNIX) and if(WIN32) to the rocm logic. We should find something that works on both platforms. Best-effort, reasonable fallbacks, etc.

Thanks, TheRock includes rocm-core on Windows: https://github.com/ROCm/TheRock. We also have a rather involved patch for this LoadHIP.cmake file here: https://github.com/ROCm/TheRock/blob/main/external-builds/pytorch/patches/pytorch/v2.7.0/pytorch/hipified/0001-Rework-LoadHIP.cmake-to-be-based-purely-on-CMAKE_PRE.patch which I think was originally authored by @stellaraccident on ROCm/TheRock#101.

Thank you for explaining that we shouldn't use if(UNIX) and if(WIN32) in the ROCm logic in the future.

Here is the code change and explanation for the fix:

The following line caused issues on ROCm on Windows. This line sets ROCM_LIB_NAME to ROCM and changes it to HIP in case rocm-core/rocm_version.h is not found and hip/hip_version.h is found.

This is fine in case that we are passing once through this code. On Windows this part of code is executed twice during configuration, so on the second time when we are reaching this line ROCM_VERSION_HEADER_PATH is not empty and it has correct path to hip_version.h. In that case ROCM_LIB_NAME is not changed to HIP because it doesn't satisfy if statement which leads to error.

I committed fix without if(UNIX) code which solves this issue on Windows.

Thank you for explaining that we shouldn't use if(UNIX) and if(WIN32) in the ROCm logic in the future.

Here is the code change and explanation for the fix:

The following line caused issues on ROCm on Windows. This line sets ROCM_LIB_NAME to ROCM and changes it to HIP in case rocm-core/rocm_version.h is not found and hip/hip_version.h is found.

This is fine in case that we are passing once through this code. On Windows this part of code is executed twice during configuration, so on the second time when we are reaching this line ROCM_VERSION_HEADER_PATH is not empty and it has correct path to hip_version.h. In that case ROCM_LIB_NAME is not changed to HIP because it doesn't satisfy if statement which leads to error.

I committed fix without if(UNIX) code which solves this issue on Windows.

It may solve the issue on hipsdk, but not on windows. I am willing to acknowledge that there is a bug, but the solution cannot be to fork around it for windows generally, as I believe this is only a problem with hipsdk.

As an aside, we will be removing the other if WIN32 diversions in this file today.

Thank you for explaining that we shouldn't use if(UNIX) and if(WIN32) in the ROCm logic in the future.
Here is the code change and explanation for the fix:
The following line caused issues on ROCm on Windows. This line sets ROCM_LIB_NAME to ROCM and changes it to HIP in case rocm-core/rocm_version.h is not found and hip/hip_version.h is found.
This is fine in case that we are passing once through this code. On Windows this part of code is executed twice during configuration, so on the second time when we are reaching this line ROCM_VERSION_HEADER_PATH is not empty and it has correct path to hip_version.h. In that case ROCM_LIB_NAME is not changed to HIP because it doesn't satisfy if statement which leads to error.
I committed fix without if(UNIX) code which solves this issue on Windows.

It may solve the issue on hipsdk, but not on windows. I am willing to acknowledge that there is a bug, but the solution cannot be to fork around it for windows generally, as I believe this is only a problem with hipsdk.

As an aside, we will be removing the other if WIN32 diversions in this file today.

I agree with you, so as you can see on the last commit we are not checking the OS. Instead, we are first searching for rocm_version.h and if it doesn't exist we are searching for hip_version.h and set ROCM_LIB_NAME to ROCM or HIP according to rocm_version.h or hip_version.h presence

@pytorchbot merge

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Merge failed

Reason: 1 jobs have failed, first few of them are: linux-binary-manywheel / manywheel-py3_9-rocm6_4-build / build

@pytorchbot merge

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

• pull / linux-jammy-rocm-py3.10 / build

Dig deeper by viewing the failures on hud

rocm linux build failed with these changes, must fix.

@pytorchbot merge

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / linux-jammy-rocm-py3.10 / test (default, 2, 2, linux.rocm.gpu.2)

@pytorchbot merge

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team