ARROW-1424: [Python] Add CUDA support to pyarrow #2536
Conversation
…for copy_to_host.
…uffer and CudaHostBuffer.
… unittests for CudaBufferWriter/Reader
|
Looks like you need to rebase, let me know if you need help |
Change-Id: Id79eb87983b3e1eb449fd8ecf05def823d655ef2
wesm
force-pushed
the
pearu-cuda-pyarrow
branch
from
e35b2a0
to
d237b34
Compare
|
I went ahead and fixed the branch (ugly merge the crossed the parquet-cpp repo merge). Will review when I can |
|
Thanks, Wes! |
|
I get a core dump when running the test suite: Here is the truncated gdb backtrace: |
|
This PR is still WIP primarily due to the test_IPC issue, see the description above. |
There was a problem hiding this comment.
Thanks @pearu for working through this! The code looks clean and reasonable.
I left a number of stylistic comments and questions. I think we should probably also change our terminology around "GPU" to be "CUDA" instead. Might want to change this in the C++ library, too, in case we have demand for supporting OpenCL at some point
python/CMakeLists.txt
Outdated
| if (ARROW_GPU_FOUND) | ||
| ADD_THIRDPARTY_LIB(arrow_gpu | ||
| SHARED_LIB ${ARROW_GPU_SHARED_LIB}) | ||
| endif() |
|
@pitrou I cannot reproduce core dump here on Ubuntu 16.04 with Python versions 3.6.6 nor 3.7.0. However, I suspect that the core dump could be related to the usage |
Codecov Report
@@ Coverage Diff @@
## master #2536 +/- ##
==========================================
+ Coverage 87.67% 88.23% +0.55%
==========================================
Files 372 311 -61
Lines 57914 54578 -3336
==========================================
- Hits 50777 48155 -2622
+ Misses 7063 6423 -640
+ Partials 74 0 -74
Continue to review full report at Codecov.
|
|
If I comment out the |
… Context.get_num_devices and Context.device_number.
python/setup.py
Outdated
| if self.with_cuda: | ||
| cmake_options.append('-DPYARROW_BUILD_CUDA=on') | ||
| else: | ||
| cmake_options.append('-DPYARROW_BUILD_CUDA=off') |
There was a problem hiding this comment.
Not sure this is desired. IMHO we should probably let the extension build by default, where possible.
There was a problem hiding this comment.
Removed -DPYARROW_BUILD_CUDA=off as redundant.
Detecting Arrow CUDA is implemented in FindArrowCuda.cmake. Moving the detection algorithm to setup.py is involved and against the current logic in cmake/setup files, imho.
An option is to make --with-cuda default, that is, replace it with --without-cuda.
There was a problem hiding this comment.
I guess it would be possible to analyze the output of cmake to detect if libarrow_gpu library is available and then define with_cuda flag. It sounds hackish though.
There was a problem hiding this comment.
Why not try to build and fail silently, as for Parquet and Orc?
There was a problem hiding this comment.
setup.py-wise, _cuda is configured and build exactly like _parquet or _orc. So I don't understand your analogue, especially because parquet and orc are disabled by default.
I suggest the following behavior:
Case 1: libarrow_gpu can be detected by cmake, then
python setup.py build_ext --with-cudawill build_cudaextensionPYARROW_BUILD_CUDA=1 python setup.py build_extwill build_cudaextensionpython setup.py build_extwill succeed, no_cudaextension is builtPYARROW_BUILD_CUDA=0 python setup.py build_extwill succeed, no_cudaextension is built
Case 2: libarrow_gpu is not detected by cmake, then
python setup.py build_ext --with-cudawill failPYARROW_BUILD_CUDA=1 python setup.py build_extwill failpython setup.py build_extwill succeed, no_cudaextension is built
|
What about the licensing of CUDA? I guess we cannot include it into our official Arrow wheels? |
|
CUDA toolkit is huge, I don't think you want to include it in any case. |
|
@xhochy - According to Attachment A of teh cudatoolkit EULA, there are actually a fairly large number of files you are allowed to redistribute. These are mostly shared objects that let you run (but not compile) CUDA code. Of course, you'd still need the NVIDIA drivers installed... |
I maybe missed out the important point: Could we build the cuda support in such a fashion that we link against CUDA but don't depend on things that are not allowed in the ASF policy. I'm not really informed about the ABI stability of CUDA but it seems that Torch for example has wheels with CUDA support. Looking at them it rather seems like you need to build for every major version which is quite hard to package. |
|
yeah, I don't know the details of ASF policy here |
pearu
changed the title
|
@pearu Thanks a lot for doing this! I will review again in a few days, unless someone beats to me to it. |
wesm
changed the title
| return self.host_buffer.get().size() | ||
|
|
||
|
|
||
| cdef class BufferReader(NativeFile): |
There was a problem hiding this comment.
It could be clearer to call this CudaBufferReader but this is fine for now
| import numpy as np | ||
| import sysconfig | ||
|
|
||
| cuda = pytest.importorskip("pyarrow.cuda") |
There was a problem hiding this comment.
This should not fail silently (skipping all tests) like it will now. Suggest we improve this aspect when we get CI set up for this code (somehow)
This PR implements CUDA support to pyarrow. In order to use it, the Arrow C++ library must be built with the following cmake options:
To enable CUDA support in pyarrow, it must be built with
--with-cudaoption, e.g.or the environment must define
This CUDA support implementation is rather complete: all Arrow C++ GPU related functions and classes are exposed to Python, the new methods and functions are documented, the test coverage is close to 100%.
However, there are some issues that need to be tackled and questions to be answered (hence the WIP attribute):
test_IPCinpython/pyarrow/tests/test_gpu.py) fails when callingopen_ipc_buffer:cuIpcOpenMemHandle fails with code 201. Currently, I don't know why. Any hint or help on this is much appreciated. [FIXED: usingmultiprocessing.Processinspawnmode]