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

cmake: introduce USE_SYSTEM_* flags #1859

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ConnorBaker
Copy link

@ConnorBaker ConnorBaker commented Jul 1, 2023

Similar to the work done in pytorch/pytorch#37137, this adds the following CMake options:

  • USE_SYSTEM_LIBS
  • USE_SYSTEM_ASMJIT
  • USE_SYSTEM_CPUINFO
  • USE_SYSTEM_GOOGLETEST

This is particularly useful in the context of Nix, where we can build these libraries once and then re-use them elsewhere to avoid rebuilding vendors dependencies.

I'm unsure of how to best enable this for fbgemm_gpu.

What's the relationship between FBGEMM and libtorch? Does libtorch require fbgemm, fbgemm_gpu, or both?

@netlify
Copy link

netlify bot commented Jul 1, 2023

Deploy Preview for pytorch-fbgemm-docs canceled.

Name Link
🔨 Latest commit 8dc96b9
🔍 Latest deploy log https://app.netlify.com/sites/pytorch-fbgemm-docs/deploys/64e43cb85fc3e90008b17749

@ConnorBaker
Copy link
Author

@q10 I see that you've worked with the CMake in this repo before, would you have the bandwidth to review this?

@ConnorBaker ConnorBaker force-pushed the feat/use-system-libs branch from 0fd8f32 to a78f826 Compare July 1, 2023 23:27
@q10
Copy link
Contributor

q10 commented Jul 2, 2023

@q10 I see that you've worked with the CMake in this repo before, would you have the bandwidth to review this?

Yes I can. Let me run it through the OSS CI system before reviewing this more thoroughly on Monday.

@ConnorBaker
Copy link
Author

Much appreciated! And any insight on changes I'd need to make for the GPU variant would be awesome :)

@facebook-github-bot
Copy link
Contributor

@q10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ConnorBaker ConnorBaker force-pushed the feat/use-system-libs branch 2 times, most recently from b08bac7 to 2a0fb67 Compare July 4, 2023 20:53
@facebook-github-bot
Copy link
Contributor

@q10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ConnorBaker
Copy link
Author

As a followup, any thoughts on offering an fbgemm-config.cmake for users who would like to be able to do find_package(fbgemm)?

I know that fbgemmLibraryConfig.cmake is produced, but that's also located in /share/cmake/fbgemm and so I need to use find_package(fbgemmLibrary) and pass "-DfbgemmLibrary_DIR:PATH=${fbgemm}/share/cmake/fbgemm" to have CMake find it.

Although, I'm not very proficient in writing or interpreting CMake, so that may be working as intended or a best practice.

@q10
Copy link
Contributor

q10 commented Jul 5, 2023

As a followup, any thoughts on offering an fbgemm-config.cmake for users who would like to be able to do find_package(fbgemm)?

I know that fbgemmLibraryConfig.cmake is produced, but that's also located in /share/cmake/fbgemm and so I need to use find_package(fbgemmLibrary) and pass "-DfbgemmLibrary_DIR:PATH=${fbgemm}/share/cmake/fbgemm" to have CMake find it.

Although, I'm not very proficient in writing or interpreting CMake, so that may be working as intended or a best practice.

I think it's a good idea to offer an fbgemm-config.cmake, though we are not experts on CMake either, which is why we have not gotten around to working on this.

I'm looking at the CI builds, and it appears that the CMake changes made in this PR have broken all the CMake-based builds of FBGEMM (see here)...

@ConnorBaker ConnorBaker force-pushed the feat/use-system-libs branch from ff1ed83 to 93cdf9b Compare July 9, 2023 07:36
@ConnorBaker
Copy link
Author

@q10 would you mind triggering CI again to see if it's fixed?

@q10
Copy link
Contributor

q10 commented Jul 10, 2023

@q10 would you mind triggering CI again to see if it's fixed?

Looks to be broken still :(

@ConnorBaker ConnorBaker force-pushed the feat/use-system-libs branch from 9f635c8 to eea5f2a Compare July 14, 2023 21:21
@ConnorBaker ConnorBaker force-pushed the feat/use-system-libs branch 4 times, most recently from 6375da8 to 87b615c Compare August 22, 2023 04:25
@ConnorBaker ConnorBaker force-pushed the feat/use-system-libs branch from 87b615c to 8dc96b9 Compare August 22, 2023 04:42
@ConnorBaker
Copy link
Author

Alrighty, @q10 would you mind running the CI again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants