Skip to content
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

Fix multiple find_package(Torch) calls #8407

Merged
merged 3 commits into from
Feb 12, 2025
Merged

Conversation

huydhn
Copy link
Contributor

@huydhn huydhn commented Feb 12, 2025

While debugging the build issue on #8322 w.r.t mkl, I undercover a complex interaction between #8322, #8248 (to install mkl), and https://github.com/pytorch/pytorch/blob/main/cmake/public/mkl.cmake from PyTorch. The error is as follows:

CMake Error at /opt/conda/envs/py_3.10/lib/cmake/mkl/MKLConfig.cmake:744 (add_library): <-- This file comes from conda mkl
  add_library cannot create imported target "MKL::MKL" because another target
  with the same name already exists.
Call Stack (most recent call first):
  /opt/conda/envs/py_3.10/lib/python3.10/site-packages/torch/share/cmake/Caffe2/public/mkl.cmake:1 (find_package) <-- this is from PyTorch
  /opt/conda/envs/py_3.10/lib/python3.10/site-packages/torch/share/cmake/Caffe2/Caffe2Config.cmake:106 (include)
  /opt/conda/envs/py_3.10/lib/python3.10/site-packages/torch/share/cmake/Torch/TorchConfig.cmake:68 (find_package)
  CMakeLists.txt:753 (find_package)

The conclusion is that, with mkl installed, there should be just one find_package(Torch) call because the mkl target is defined globally. The torch target, on the other hand, is only defined locally.

So, this change adds if(NOT TARGET torch) check to only call find_package(Torch) if needed.

Testing

The change on top of #8322 looks like this f705b01

https://github.com/pytorch/executorch/actions/runs/13278590926?pr=8399

@huydhn huydhn requested a review from swolchok February 12, 2025 05:22
Copy link

pytorch-bot bot commented Feb 12, 2025

🔗 Helpful Links

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

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

✅ No Failures

As of commit 69105d0 with merge base 4e3a8bd (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot facebook-github-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 Feb 12, 2025
@huydhn
Copy link
Contributor Author

huydhn commented Feb 12, 2025

This issue looks strange though and I think it might better fix it upstream in https://github.com/pytorch/pytorch/blob/main/cmake/public/mkl.cmake. @malfet Have you seen this issue before?

Copy link
Contributor

@swolchok swolchok left a comment

Choose a reason for hiding this comment

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

thank you!!!

@swolchok swolchok merged commit 1308d4d into main Feb 12, 2025
46 of 48 checks passed
@swolchok swolchok deleted the fix-multiple-find-package-torch branch February 12, 2025 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants