Add Linux build on GPU on Github Actions #9335
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
cc @assignUser @Yuhta @pedroerp @kgpai It's building on all pull requests. The testing would happen on a subset of PR or on nightly as discussed previously, and would be done on a different job. I've tested locally with fmt version 9.1.0 as well, and it compiles with both CUDA 11.8 and 12.4 Could you please also let me know what are the CUDA versions used internally, to make sure we add them to the test matrix here so we don't break the build? |
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Looks good but I think we should just integrate it into the adapters build as the only difference in both is the additional installation of cuda + -DVELOX_ENABLE_GPU=ON.
That way we only add one additional build instead of two. Another option would be to only build the gpu parts with anything not needed (e.g. the connectors I would assume?) .
If I understand correctly the tests will be skipped automatically when no cuda device is detected anyway right?
|
@assignUser ok, sounds good, I'll fold it into the adapters build. |
|
@luhenry Thinking on this a bit more, what are the minimal flags you need? If the build is much smaller (+ potentially skip building test) it might be faster to build 2 standalone builds Vs one additional adapters build. |
|
@assignUser the build isn't much smaller as Wave depends (directly or indirectly) on most of the other velox libraries. Also, it doesn't add much to the existing build in terms of time to compile or produced binaries. If you're concerned about resource/machine usage, we can build a limited set of cuda versions (what's the one(s) you'd like to focus on?) and build the larger set on the nightlies. |
Yep that was my concern. Sounds like the most efficent way would be adding the wave build to the existing adapters build with one cuda version (someone else should choose which one ^^) + a nightly matrix build with multiple versions. cc @kgpai |
I've changed it to a |
|
@luhenry looks good but could you change the test skipping code so it doesn't cause gtest to pick up a failed test? Otherwise this will always fail the adapters job which is a bit unfortunate ^^ I was about to suggest installing cuda in the docker image but it seems that gha has great mirrors and the install only takes 27s so that's probably not much slower than the additional ~1GB to download if we add it to the container :D |
Fixed with 5c3780e
It should be adding ~217MB as that's what's getting installed by |
I did local testing (with both versions though) and that added ~ 800mb to the final image but in any case it's fine as it is for now. :) |
|
@assignUser I've just merged the |
|
@assignUser all tests are now passing. |
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@luhenry Can you rebase to the newest? |
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@Yuhta fixed. |
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: Pull Request resolved: facebookincubator#9335 Reviewed By: kagamiori Differential Revision: D55646566 Pulled By: Yuhta fbshipit-source-id: 7c1cc2ef8db3da4e9f9889b9c7bcde52283e6bb2
No description provided.