-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: Add Meson support in nanoarrow_device #484
Conversation
|
||
nanoarrow_dep = dependency('nanoarrow') | ||
|
||
apple_dep = dependency('appleframeworks', |
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 am not able to test the apple or cuda deps unfortunately
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 can give this a try tomorrow for both!
ci/scripts/build-with-meson.sh
Outdated
@@ -66,7 +66,7 @@ function main() { | |||
pushd "${SANDBOX_DIR}" | |||
|
|||
show_header "Run test suite" | |||
meson configure -Dtests=true -Db_coverage=true | |||
meson configure -Dtests=true -Db_coverage=true -Dipc=true -Ddevice=true |
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.
Missed this on the IPC change. If we want to expand this job in the future would need to accept these options dynamically, but this should work for now
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.
This looks great!
When I run:
meson configure -Dtests=true -Ddevice=true -Dmetal=true
I get
../src/nanoarrow/nanoarrow_device_metal_test.cc:22:10: fatal error: 'Metal/Metal.hpp' file not found
#include <Metal/Metal.hpp>
^~~~~~~~~~~~~~~~~
../src/nanoarrow/nanoarrow_device_metal_test.cc:22:10: note: did not find header 'Metal.hpp' in framework 'Metal' (loaded from '/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks')
Metal C++ is its own thing and is a sort of header-only wrapper around the Objective C (not included directly in the framework):
arrow-nanoarrow/CMakeLists.txt
Lines 389 to 400 in d6eb52b
if(NANOARROW_DEVICE_WITH_METAL) | |
if(NOT EXISTS "${CMAKE_BINARY_DIR}/metal-cpp") | |
message(STATUS "Fetching metal-cpp") | |
file(DOWNLOAD | |
"https://developer.apple.com/metal/cpp/files/metal-cpp_macOS12_iOS15.zip" | |
"${CMAKE_BINARY_DIR}/metal-cpp.zip") | |
file(ARCHIVE_EXTRACT | |
INPUT | |
${CMAKE_BINARY_DIR}/metal-cpp.zip | |
DESTINATION | |
${CMAKE_BINARY_DIR}) | |
endif() |
I also tried
meson configure -Dtests=true -Ddevice=true -Dcuda=true
...and get
c++ -o src/nanoarrow/nanoarrow-device-cuda-test src/nanoarrow/nanoarrow-device-cuda-test.p/nanoarrow_device_cuda_test.cc.o src/nanoarrow/nanoarrow-device-cuda-test.p/.._.._subprojects_googletest-1.14.0_googletest_src_gtest-all.cc.o src/nanoarrow/nanoarrow-device-cuda-test.p/.._.._subprojects_googletest-1.14.0_googletest_src_gtest_main.cc.o -Wl,--as-needed -Wl,--no-undefined -Wl,-O1 '-Wl,-rpath,$ORIGIN/' -Wl,-rpath-link,/home/dewey/rscratch/arrow-nanoarrow/builddir/src/nanoarrow -Wl,--start-group src/nanoarrow/libnanoarrow_device.so src/nanoarrow/libnanoarrow.so -pthread /usr/local/cuda/lib64/libcudart_static.a -lrt -lpthread -ldl /usr/local/cuda/lib64/libcublas.so -Wl,--end-group
/usr/bin/ld: src/nanoarrow/nanoarrow-device-cuda-test.p/nanoarrow_device_cuda_test.cc.o: in function `NanoarrowDeviceCuda_DeviceCudaBufferCopy_Test::TestBody()':
nanoarrow_device_cuda_test.cc:(.text+0x1ec2): undefined reference to `ArrowDeviceCuda'
/usr/bin/ld: nanoarrow_device_cuda_test.cc:(.text+0x1f34): undefined reference to `cuDeviceGet'
...and this is maybe because cublas
is not the right thing to link to (the thing we're using is called the "driver" API, which is not the "runtime" API). Also possibly because there needs to be a -DNANOARROW_WITH_CUDA
compile define to get the ArrowDeviceCuda
symbol into nanoarrow_device.so
(still workshopping the final version of that).
In the next week or two we'll have a CUDA runner that will help with this 🙂
Sweet thanks for giving this a shot
Possibly, though I would still hope Meson could handle that. I found another project in the WrapDB that uses Metal: Maybe we just need to add
Ah OK...apologies as I know embarassingly little about CUDA. But in the configuration now I have |
Ah ignore my comment above about Metal - I confused I'll ask on the Meson IRC channel tomorrow how to resolve that header; they are usually really responsive to questions like this |
Actually was able to borrow a MacBook to test out the metal stuff. Didn't realize metal-cpp was not installed with the system framework :-) So the metal stuff should be compiling now. There were also some issues with the source files that needed to be corrected. Note that the metal test fails - I haven't looked into why but things at least compile now: 1/1 nanoarrow-device-metal FAIL 0.03s killed by signal 11 SIGSEGV
>>> MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MALLOC_PERTURB_=159 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 /Users/willayd/clones/arrow-nanoarrow/builddir/src/nanoarrow/nanoarrow-device-metal-test
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
Running main() from /tmp/googletest-20230817-5025-15wtgyq/googletest-1.14.0/googletest/src/gtest_main.cc
[==========] Running 10 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 6 tests from NanoarrowDeviceMetal
[ RUN ] NanoarrowDeviceMetal.DefaultDevice
../src/nanoarrow/nanoarrow_device_metal_test.cc:30: Failure
Expected equality of these values:
ArrowDeviceMetalInitDefaultDevice(device.get(), nullptr)
Which is: 22
0
[ FAILED ] NanoarrowDeviceMetal.DefaultDevice (15 ms)
[ RUN ] NanoarrowDeviceMetal.DeviceGpuBufferInit |
Thanks! I can also help here since I have the setup. I wonder if you have an Intel MacBook (which wouldn't have a metal default device). (Metal's default device is also available on the MacOS CI runner if you use macOS-latest, since those are all M1s). |
src/nanoarrow/meson.build
Outdated
endif | ||
|
||
if get_option('cuda') | ||
cuda_dep = dependency('cuda', modules: ['cuda_driver']) |
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.
Just guessing on the module name. Hoping it matches what CMake has (and what exists in our current CMake config)
https://cmake.org/cmake/help/latest/module/FindCUDAToolkit.html
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 tried a number of things here, but ultimately was unable to make the linking work properly. We need libcuda.so
, which I couldn't find in the Meson CUDA sources. It's the "driver" library/API, which the cudart
would also need to link; however, I think the Meson CUDA stuff is almost 100% designed for nvcc
, and we're using regular gcc
(i.e., we're not actually using NVIDIA's compiler, we're using a regular compiler and linking to the library).
It's fine to error here for now until somebody with both Meson experience and a graphics card feels like debugging this!
Yea its an old Intel device - so is the metal-cpp header only required for that? |
It will build without error because the framework is installed; however, it won't run because there is no GPU on an intel mac (i.e., when you call GetDefaultDevice from Metal it will give you a nullptr). I'll give both of these a try shortly! |
link_with: nanoarrow_device_lib, | ||
dependencies: [nanoarrow_dep, gtest_dep, metal_cpp_dep], | ||
) | ||
test('nanoarrow-device-metal', exc) |
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.
There also needs to be a compile define here -DNANOARROW_DEVICE_WITH_METAL
(I think the error you were seeing might have happened even with this enabled if you don't have a Mac with a GPU; however, I'm seeing it also with the Meson build.
Also see #527 for a few updates to the metal build (+ adding it to CI).
src/nanoarrow/meson.build
Outdated
endif | ||
|
||
if get_option('cuda') | ||
cuda_dep = dependency('cuda', modules: ['cuda_driver']) |
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 tried a number of things here, but ultimately was unable to make the linking work properly. We need libcuda.so
, which I couldn't find in the Meson CUDA sources. It's the "driver" library/API, which the cudart
would also need to link; however, I think the Meson CUDA stuff is almost 100% designed for nvcc
, and we're using regular gcc
(i.e., we're not actually using NVIDIA's compiler, we're using a regular compiler and linking to the library).
It's fine to error here for now until somebody with both Meson experience and a graphics card feels like debugging this!
src/nanoarrow/meson.build
Outdated
if get_option('cuda') | ||
cuda_dep = dependency('cuda', modules: ['cuda_driver']) | ||
device_deps += cuda_dep | ||
device_srcs += 'nanoarrow_device_cuda.cc' |
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.
device_srcs += 'nanoarrow_device_cuda.cc' | |
device_srcs += 'nanoarrow_device_cuda.c' |
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.
Thank you!
No description provided.