velox
velox copied to clipboard
facebookincubator
Fix Thrift dependency when using system Arrow
With this https://github.com/facebookincubator/velox/commit/0d80228463e4fb73f9fbf6d61660e155a3115cb3 commit included, velox will resolve dependency by firstly finding lib arrow from system. In this path, linking with lib thrift is lacking, which causes thrift header not found issue.
With this pr, velox will try to find thrift lib. If it is not found, velox will build arrow from source with thrift bundled. If found, lib arrow will be linked with it. This pr also lets setup scripts install thrift bundled in arrow.
Deploy Preview for meta-velox canceled.
| Name | Link |
|---|---|
| Latest commit | a547494a34b88d4bbbd37b5de9608346211f977d |
| Latest deploy log | https://app.netlify.com/sites/meta-velox/deploys/668dd7cc4128990008058520 |
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
@assignUser, @majetideepak, @pedroerp, this is an urgent fix for CI issue. Please prioritize to review it.
@PHILO-HE why was Thrift not installed from source in the original PR?
@PHILO-HE why was Thrift not installed from source in the original PR?
@majetideepak, thanks for your review. CMake only installs arrow libs, not its dependency libs. I will fix it in another pr. We can firstly land this patch to avoid blocking other pr.
BTW, just fixed your comment.
@PHILO-HE a simpler workaround to unblock CI is to bundle Arrow like before. I opened a PR here https://github.com/facebookincubator/velox/pull/10360 Can you help fix the broken arrow thrift dependency? Thanks!
My worry with installing thrift is that it might conflict with fbthrift. Let's first validate the thrift install before we enable auto for Arrow source in CI.
My worry with installing thrift is that it might conflict with fbthrift. Let's first validate the thrift install before we enable auto for Arrow source in CI.
@majetideepak, I see. I will continue the fix for thrift dependency here. Thanks!
Hello, if this commit, as it is, is merged then it will break the ubuntu builds. In setup-ubuntu.sh, the package libthrift-dev needs to be removed from the list of packages that are installed. Otherwise, the build will use the wrong thrift.
Hello, if this commit, as it is, is merged then it will break the ubuntu builds. In setup-ubuntu.sh, the package libthrift-dev needs to be removed from the list of packages that are installed. Otherwise, the build will use the wrong thrift.
@tigrux, I just removed libthrift-dev from setup-ubuntu.sh. Thanks for your comment!
@PHILO-HE changes look good to me. Were you able to verify the fix locally? We can revert the CI job for Linux release with adapters to use auto for Arrow.
@majetideepak, yes, I have verified locally. And just reverted Arrow from BUNDLED to AUTO. Thanks!
Looks good to me. I would like @assignUser to make another pass as well.
Looks good to me. I would like @assignUser to make another pass as well.
@majetideepak, I note assignUser's Github state is on vacation. @rui-mo, do you have any comment?
@PHILO-HE can you address this issue as well? I want to avoid a conflict. https://github.com/facebookincubator/velox/pull/10378#discussion_r1663365076 CC: @czentgr
@PHILO-HE can you address this issue as well? I want to avoid a conflict. #10378 (comment) CC: @czentgr
@majetideepak, just fixed that comment in the latest commit. Thanks!
@PHILO-HE do you verified this PR in Gluten?
@Yohahaha, just tested in my local. I'm creating a Gluten PR to see whether Gluten CI can pass. Thanks for your reminding!
No issue is found so far. @bikramSingh91, could you merge this pr if you have no comment? Thanks!
@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@pedroerp, could you please merge this pr? The CI failure is not relevant to this pr. Thanks!
@pedroerp merged this pull request in facebookincubator/velox@c3dd27421e8e1656d81ca44bf65bef8807dbcd39.
Conbench analyzed the 1 benchmark run on commit c3dd2742.
There were no benchmark performance regressions. 🎉
The full Conbench report has more details.
![conbench-facebook[bot] avatar](https://avatars.githubusercontent.com/in/241235?v=4 "Published by a pub.dev verified publisher"){kind=small}