Fix Thrift dependency when using system Arrow #10355
Conversation
✅ Deploy Preview for meta-velox canceled.
|
|
@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? |
@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. |
|
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. |
There was a problem hiding this 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.
99365f8 to
a28deb3
Compare
@tigrux, I just removed libthrift-dev from setup-ubuntu.sh. Thanks for your comment! |
@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. |
@majetideepak, I note assignUser's Github state is on vacation. |
|
@PHILO-HE can you address this issue as well? I want to avoid a conflict. #10378 (comment) |
@majetideepak, just fixed that comment in the latest commit. Thanks! |
|
@PHILO-HE do you verified this PR in Gluten? |
|
No issue is found so far. |
scripts/setup-centos9.sh
Outdated
|
|
||
| # Install thrift. | ||
| cd _build/thrift_ep-prefix/src/thrift_ep-build | ||
| $SUDO cmake --install ./ --prefix /usr/local/ |
There was a problem hiding this comment.
$SUDO isn't defined here. Don't you also need to add the definition in this script like it is done for Ubuntu (and in the functions that call cmake etc)? It isn't inherited from setup-helper-functions.sh.
On Ubuntu it does this:
SUDO="${SUDO:-"sudo --preserve-env"}"
Nit: do we want to use ${SUDO} like in the other scripts?
There was a problem hiding this comment.
@czentgr, I just removed the use of ${SUDO} in this setup-centos9.sh and ${SUDO} is still not defined with sudo, consistent with other places where cmake_install is used in this script.
BTW, I find sudo is not installed in some centos-9 images used for velox CI. So just defining ${SUDO} without installing sudo can cause execution failure.
ba9382b to
188cf14
Compare
|
@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! |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: With this facebookincubator@0d80228 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. Pull Request resolved: facebookincubator#10355 Reviewed By: bikramSingh91 Differential Revision: D59595645 Pulled By: pedroerp fbshipit-source-id: 9f26eee8347a6c650e70163554fa9b2b72f5bfac
| -DCMAKE_INSTALL_PREFIX=/usr/local \ | ||
| -DCMAKE_BUILD_TYPE=Release \ | ||
| -DARROW_BUILD_STATIC=ON \ | ||
| -DThrift_SOURCE=BUNDLED |
There was a problem hiding this comment.
If we remove this we should be able to keep the system thrift and also use that for velox.
There was a problem hiding this comment.
@assignUser, seems thrift is not explicitly installed into system in any other places. Here, with Thrift_SOURCE set to BUNDLED and the following code to install it, velox build can use the thrift after setup script is executed. Right?
There was a problem hiding this comment.
Yes, nice trick installing arrows build thrift :D
…tor#10355)" This reverts commit c3dd274.
With this 0d80228 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.