| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 24.7k
Export XPU libs to be public #136974
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
Export XPU libs to be public #136974
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/136974
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 20e71fd with merge base 5c3ba6f ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
guangyey
added
topic: not user facing
cmake/TorchConfig.cmake.in
Outdated
| @@ -132,12 +135,16 @@ if(@USE_CUDA@) | |||
| endif() | |||
|
|
|||
| if(@BUILD_SHARED_LIBS@) | |||
| find_library(C10_CUDA_LIBRARY c10_cuda PATHS "${TORCH_INSTALL_PREFIX}/lib") | |||
| list(APPEND TORCH_CUDA_LIBRARIES ${C10_CUDA_LIBRARY} ${Caffe2_PUBLIC_CUDA_DEPENDENCY_LIBS}) | |||
| append_torchlib_if_found(c10_cuda torch_cuda) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously here miss libtorch_cuda.so
cmake/TorchConfig.cmake.in
Outdated
| @@ -133,11 +136,16 @@ if(@USE_CUDA@) | |||
|
|
|||
| if(@BUILD_SHARED_LIBS@) | |||
| find_library(C10_CUDA_LIBRARY c10_cuda PATHS "${TORCH_INSTALL_PREFIX}/lib") | |||
| list(APPEND TORCH_CUDA_LIBRARIES ${C10_CUDA_LIBRARY} ${Caffe2_PUBLIC_CUDA_DEPENDENCY_LIBS}) | |||
| find_library(TORCH_CUDA_LIBRARY c10_cuda PATHS "${TORCH_INSTALL_PREFIX}/lib") | |||
| list(APPEND TORCH_CUDA_LIBRARIES ${C10_CUDA_LIBRARY} ${TORCH_CUDA_LIBRARY} ${Caffe2_PUBLIC_CUDA_DEPENDENCY_LIBS}) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep TORCH_CUDA_LIBRARIES BC, here doesn't directly use append_torchlib_if_found. Because append_torchlib_if_found doesn't update TORCH_CUDA_LIBRARIES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unrelated to the PR title. Do you mind proposing it in separate PR (and posting some sort of examples of what behavior it fixes)
|
@pytorchbot rebase |
|
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
|
Successfully rebased |
|
@pytorchbot rebase |
|
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
|
Successfully rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please help me understand why find_library(TORCH_CUDA_LIBRARY is needed?
Sorry about that. Previously, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XPU changes looks ok to me, please make changes to TORCH_CUDA_LIBRARIES in separate PR (and perhaps create an issue to explain what problem does it solve)
cmake/TorchConfig.cmake.in
Outdated
| @@ -133,11 +136,16 @@ if(@USE_CUDA@) | |||
|
|
|||
| if(@BUILD_SHARED_LIBS@) | |||
| find_library(C10_CUDA_LIBRARY c10_cuda PATHS "${TORCH_INSTALL_PREFIX}/lib") | |||
| list(APPEND TORCH_CUDA_LIBRARIES ${C10_CUDA_LIBRARY} ${Caffe2_PUBLIC_CUDA_DEPENDENCY_LIBS}) | |||
| find_library(TORCH_CUDA_LIBRARY c10_cuda PATHS "${TORCH_INSTALL_PREFIX}/lib") | |||
| list(APPEND TORCH_CUDA_LIBRARIES ${C10_CUDA_LIBRARY} ${TORCH_CUDA_LIBRARY} ${Caffe2_PUBLIC_CUDA_DEPENDENCY_LIBS}) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unrelated to the PR title. Do you mind proposing it in separate PR (and posting some sort of examples of what behavior it fixes)
|
@malfet May I know if I have addressed your comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
|
@pytorchbot merge |
Merge startedYour 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 |
Thanks very much for your approval. |
# Motivation Export XPU-related libs to be public. Now they are included in `TORCH_LIBRARIES` Pull Request resolved: #136974 Approved by: https://github.com/EikanWang, https://github.com/malfet
ghstack-source-id: a4a9966 Pull Request resolved: pytorch/pytorch#136974
Stack from ghstack (oldest at bottom):
Motivation
Export XPU-related libs to be public. Now they are included in
TORCH_LIBRARIES