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

Support finding installed arrow libraries from system #9992

Closed
wants to merge 11 commits into from

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented May 31, 2024

Gluten project needs to pre-build arrow before building velox. And it's also possible that arrow
libs have already been installed, e.g., via vcpkg. Then, velox doesn't need to build arrow from
source again.

@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 May 31, 2024
Copy link

netlify bot commented May 31, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit fd84b5c
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/667a38fe3091ca0007489699

@PHILO-HE
Copy link
Contributor Author

@majetideepak, @assignUser, do you have any comment?

@PHILO-HE PHILO-HE changed the title Find existing arrow libraries before building from source Add find arrow libraries support before building from source May 31, 2024
@PHILO-HE PHILO-HE changed the title Add find arrow libraries support before building from source Allow finding arrow libraries before building from source May 31, 2024
@majetideepak
Copy link
Collaborator

@assignUser can you please help review this? Thanks!

Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I do agree that we should be able to use an existing arrow install but I think we should implement it a bit differently.

There is no real reason for arrow to be in thirdparty it's just a remnant and everything else was moved to resolve_dependency already so I think arrow should follow.

We can then use the usual find_package/build-if-not-found approach utilizing arrowConfig.cmake instead hard coding things like you did here.

There might be a few quirks to keep in mind when using arrow with fetch content, there is an example here: https://github.com/amoeba/arrow-cmake-fetchcontent/

It's probably best to utilize the subdir approach we use for folly and boost in CMake/resolve_dependency for arrow too.

assignUser

This comment was marked as duplicate.

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Jun 21, 2024

Thanks for the contribution! I do agree that we should be able to use an existing arrow install but I think we should implement it a bit differently.

There is no real reason for arrow to be in thirdparty it's just a remnant and everything else was moved to resolve_dependency already so I think arrow should follow.

We can then use the usual find_package/build-if-not-found approach utilizing arrowConfig.cmake instead hard coding things like you did here.

There might be a few quirks to keep in mind when using arrow with fetch content, there is an example here: https://github.com/amoeba/arrow-cmake-fetchcontent/

It's probably best to utilize the subdir approach we use for folly and boost in CMake/resolve_dependency for arrow too.

@assignUser, thanks so much for your comment. Just updated the pr with resolve_dependency.

To find arrow from system, I tried to let arrowConfig.cmake be used. It was installed under system path. But target re-definition error for some third-party lib is reported. So currently I added FindArrow.cmake in velox for finding arrow lib.

I also tried using FetchContent_MakeAvailable to get and build arrow source code. But there are also some target conflict issues with the main project. So I keep using the existing ExternalProject_Add to isolate arrow build.

Please kindly let me know your suggestions. Thanks!

@PHILO-HE
Copy link
Contributor Author

Hi @assignUser, could you help review again? Thanks!

@PHILO-HE PHILO-HE changed the title Allow finding arrow libraries before building from source Support finding installed arrow libraries from system Jun 26, 2024
Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

Thanks, this lookgs good! While I would like to re-work this to use FetchContent, we do share a number of dependencies and I can see that leading to trouble.

This finally aligns the existing bundled Arrow build with resolve_dependency and additionally allows for use of system Arrow. So we should merge this for now and we can try FetchContent again later in a follow up :)

CMake/FindArrow.cmake Show resolved Hide resolved
@assignUser assignUser added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jun 26, 2024
@PHILO-HE
Copy link
Contributor Author

Thanks, this lookgs good! While I would like to re-work this to use FetchContent, we do share a number of dependencies and I can see that leading to trouble.

This finally aligns the existing bundled Arrow build with resolve_dependency and additionally allows for use of system Arrow. So we should merge this for now and we can try FetchContent again later in a follow up :)

@assignUser, sounds good! Thanks so much for your review!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 merged this pull request in 0d80228.

Copy link

Conbench analyzed the 1 benchmark run on commit 0d802284.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

marin-ma added a commit to oap-project/velox that referenced this pull request Jul 1, 2024
rui-mo added a commit to rui-mo/velox that referenced this pull request Jul 1, 2024
rui-mo added a commit to rui-mo/velox that referenced this pull request Jul 1, 2024
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. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants