-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix building for GPU with CUDA 12.3.1 #8853
Conversation
✅ Deploy Preview for meta-velox canceled.
|
This change adds just enough support to build Velox with VELOX_ENABLE_GPU=ON with the CUDA 12.3.1 toolchain
ecad2e2
to
c1a288c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small nits, but overall looks good to me. Thank you for helping with this.
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
--- a/folly/Conv.h | ||
+++ b/folly/Conv.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw you submitted a PR to upstream these changes to folly. What's the status of it? Is it likely to get merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have very little visibility whether it will get merged and when. I'll make sure to follow up in the coming days as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(CMake) LGTM!
72d5972
to
5a5ce2a
Compare
@luhenry There is one failure in CCI: https://circleci.com/gh/facebookincubator/velox/315682 |
@Yuhta merging latest |
The change to googletest is triggering the build failure on macOS, I'll rework that tomorrow my time |
@luhenry You just need to rebase onto master |
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@luhenry Got some compilation error in internal build:
|
@Yuhta thanks! It didn't catch with |
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@luhenry Some more error between nvcc and fmt. If it is too hard, we can delay the support of fmt from GPU code for now:
|
@Yuhta is that using a different version of fmt than on the circleci or ubuntu image? I can't reproduce when running |
Let me try compiling with CUDA 11.5 locally see how that goes. |
@Yuhta Its fmt 9 with some custom patches, but 10 is backwards compatible to 9. |
@Yuhta @kgpai I can reproduce with fmt 9.1 and CUDA 11.5.2 locally:
I'll investigate and figure out what we can do to fix that |
I can also confirm that I don't get the same error with CUDA 12.4 and fmt 9.1. Looking further into the versions, it got fixed in CUDA 12.2. |
It's failing to compile fmt with older versions
@luhenry It would be nice to keep it compatible with CUDA 11 for now, because we don't have control over the CUDA version used in build infra |
This change adds just enough support to build Velox with VELOX_ENABLE_GPU=ON with the CUDA 12.3.1 toolchain