Skip to content
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

GH-37523: [C++][CI][CUDA] Don't use newer API and add missing CUDA dependencies #37497

Merged
merged 10 commits into from
Sep 5, 2023

Conversation

zeroshade
Copy link
Member

@zeroshade zeroshade commented Aug 31, 2023

Rationale for this change

#37365 was built locally using a newer version of the CUDA driver API than the crossbow builds run with causing the crossbow build to fail due to a change in macro names. This puts the macro name back to allow it to build with the older version of the cuda driver api.

@zeroshade zeroshade requested a review from pitrou August 31, 2023 15:07
@zeroshade
Copy link
Member Author

@github-actions crossbow submit ubuntu-cuda-cpp

@github-actions
Copy link

Unable to match any tasks for `ubuntu-cuda-cpp`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/6039155877

@zeroshade
Copy link
Member Author

zeroshade commented Aug 31, 2023

@github-actions crossbow submit *cuda*

@github-actions
Copy link

Revision: 6626ee6

Submitted crossbow builds: ursacomputing/crossbow @ actions-b539647434

Task Status
conda-linux-aarch64-cuda-py3 Azure
conda-linux-ppc64le-cuda-py3 Azure
conda-linux-x64-cuda-py3 Azure
conda-win-x64-cuda-py3 Azure
test-cuda-cpp Github Actions
test-cuda-python Github Actions

@github-actions
Copy link

Revision: 6626ee6

Submitted crossbow builds: ursacomputing/crossbow @ actions-ccf9bbe515

Task Status
conda-linux-aarch64-cuda-py3 Azure
conda-linux-ppc64le-cuda-py3 Azure
conda-linux-x64-cuda-py3 Azure
conda-win-x64-cuda-py3 Azure
test-cuda-cpp Github Actions
test-cuda-python Github Actions

@zeroshade
Copy link
Member Author

@github-actions crossbow submit *cuda*

@github-actions
Copy link

Revision: 65d8df8

Submitted crossbow builds: ursacomputing/crossbow @ actions-f74ba67795

Task Status
conda-linux-aarch64-cuda-py3 Azure
conda-linux-ppc64le-cuda-py3 Azure
conda-linux-x64-cuda-py3 Azure
conda-win-x64-cuda-py3 Azure
test-cuda-cpp Github Actions
test-cuda-python Github Actions

@zeroshade
Copy link
Member Author

@github-actions crossbow submit test-cuda-python

@kou
Copy link
Member

kou commented Aug 31, 2023

It seems that this has many changes to say "MINOR".
https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes

Could you open an issue for this?

@github-actions
Copy link

Revision: 4e6835b

Submitted crossbow builds: ursacomputing/crossbow @ actions-b39bc0bebc

Task Status
test-cuda-python Github Actions

@zeroshade
Copy link
Member Author

@pitrou @AlenkaF @jorisvandenbossche I haven't been able to work out why the include path for cuda.h isn't getting included properly for the pyarrow python build here. The change I made caused the cuda.h include to now be exposed to the cython build (previously it was only in a .cc file on the C++ side so the cython build never needed to include it through cuda_api.h). Unfortunately it seems i'm not familiar enough with the cython/pyarrow build to work this out on my own.

Running tests myself locally I don't have any problems building it with a mamba environment, so I'm not sure what I'm missing. Any of you have any ideas?

@AlenkaF
Copy link
Member

AlenkaF commented Sep 1, 2023

First thing I would try is adding the header file in the install of the cmake

arrow_install_all_headers("arrow/gpu")

as it is not being installed currently (https://github.com/ursacomputing/crossbow/actions/runs/6042448301/job/16397574982#step:5:6699).
I think this has to be updated here

set(ARROW_CUDA_SRCS cuda_arrow_ipc.cc cuda_context.cc cuda_internal.cc cuda_memory.cc)
or here:
install(FILES "${CMAKE_CURRENT_BINARY_DIR}/cuda_version.h"
DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/arrow/gpu")
arrow_install_all_headers("arrow/gpu")

@kou
Copy link
Member

kou commented Sep 1, 2023

How about this?

diff --git a/cpp/src/arrow/gpu/ArrowCUDAConfig.cmake.in b/cpp/src/arrow/gpu/ArrowCUDAConfig.cmake.in
index bb36abf24..626c536a0 100644
--- a/cpp/src/arrow/gpu/ArrowCUDAConfig.cmake.in
+++ b/cpp/src/arrow/gpu/ArrowCUDAConfig.cmake.in
@@ -30,6 +30,10 @@ include(CMakeFindDependencyMacro)
 find_dependency(Arrow)
 if(CMAKE_VERSION VERSION_LESS 3.17)
   find_package(CUDA REQUIRED)
+  add_library(ArrowCUDA::cuda_driver SHARED IMPORTED)
+  set_target_properties(ArrowCUDA::cuda_driver
+                        PROPERTIES IMPORTED_LOCATION "${CUDA_CUDA_LIBRARY}"
+                                   INTERFACE_INCLUDE_DIRECTORIES "${CUDA_INCLUDE_DIRS}")
 else()
   find_package(CUDAToolkit REQUIRED)
 endif()
diff --git a/cpp/src/arrow/gpu/CMakeLists.txt b/cpp/src/arrow/gpu/CMakeLists.txt
index 2556f30f0..a5b176793 100644
--- a/cpp/src/arrow/gpu/CMakeLists.txt
+++ b/cpp/src/arrow/gpu/CMakeLists.txt
@@ -32,8 +32,11 @@ endif()
 set(ARROW_CUDA_LINK_LIBS arrow::flatbuffers)
 if(CMAKE_VERSION VERSION_LESS 3.17)
   find_package(CUDA REQUIRED)
-  set(ARROW_CUDA_SHARED_LINK_LIBS ${CUDA_CUDA_LIBRARY})
-  include_directories(SYSTEM ${CUDA_INCLUDE_DIRS})
+  add_library(ArrowCUDA::cuda_driver SHARED IMPORTED)
+  set_target_properties(ArrowCUDA::cuda_driver
+                        PROPERTIES IMPORTED_LOCATION "${CUDA_CUDA_LIBRARY}"
+                                   INTERFACE_INCLUDE_DIRECTORIES "${CUDA_INCLUDE_DIRS}")
+  set(ARROW_CUDA_SHARED_LINK_LIBS ArrowCUDA::cuda_driver)
 else()
   # find_package(CUDA) is deprecated, and for newer CUDA, it doesn't
   # recognize that the CUDA driver library is in the "stubs" dir, but
@@ -61,6 +64,7 @@ add_arrow_lib(arrow_cuda
               ${ARROW_CUDA_SHARED_LINK_LIBS}
               SHARED_INSTALL_INTERFACE_LIBS
               Arrow::arrow_shared
+              ${ARROW_CUDA_SHARED_LINK_LIBS}
               # Static arrow_cuda must also link against CUDA shared libs
               STATIC_LINK_LIBS
               ${ARROW_CUDA_LINK_LIBS}
diff --git a/cpp/src/arrow/gpu/arrow-cuda.pc.in b/cpp/src/arrow/gpu/arrow-cuda.pc.in
index 84c8c5889..173d7d91e 100644
--- a/cpp/src/arrow/gpu/arrow-cuda.pc.in
+++ b/cpp/src/arrow/gpu/arrow-cuda.pc.in
@@ -22,6 +22,6 @@ libdir=@ARROW_PKG_CONFIG_LIBDIR@
 Name: Apache Arrow CUDA
 Description: CUDA integration library for Apache Arrow
 Version: @ARROW_VERSION@
-Requires: arrow
+Requires: arrow cuda
 Libs: -L${libdir} -larrow_cuda
 Cflags: -I${includedir}

@zeroshade
Copy link
Member Author

@github-actions crossbow submit test-cuda-python

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Revision: 78ccfb9

Submitted crossbow builds: ursacomputing/crossbow @ actions-0ca7a3d49d

Task Status
test-cuda-python Github Actions

@zeroshade
Copy link
Member Author

@kou thanks! Your cmake changes fixed it perfectly! also I agree completely that this has expanded beyond the scope of a MINOR change. I'll create an issue for it.

@zeroshade zeroshade changed the title MINOR: fix cuda crossbow build GH-37523: [C++][CI][CUDA] fix cuda crossbow build Sep 1, 2023
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

⚠️ GitHub issue #37523 has been automatically assigned in GitHub to PR creator.

@kou kou changed the title GH-37523: [C++][CI][CUDA] fix cuda crossbow build GH-37523: [C++][CI][CUDA] Don't use newer API and add missing CUDA dependencies Sep 1, 2023
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@@ -70,6 +70,7 @@ export PYARROW_WITH_SUBSTRAIT=${ARROW_SUBSTRAIT:-OFF}
export PYARROW_PARALLEL=${n_jobs}

export LD_LIBRARY_PATH=${ARROW_HOME}/lib:${LD_LIBRARY_PATH}
export CMAKE_PREFIX_PATH=${ARROW_HOME}:${CMAKE_PREFIX_PATH}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?
If we don't need this, I want to revert this before we merge this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that fixed one of the build errors, but I haven't tried without it since adding your fix. I'll try removing it and re-run the crossbow build, if all works fine then we're good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good with it reverted! all is passing. Thanks for the help!

@github-actions github-actions bot added awaiting merge Awaiting merge awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review awaiting merge Awaiting merge labels Sep 1, 2023
@zeroshade
Copy link
Member Author

@github-actions crossbow submit test-cuda-python

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 5, 2023
@github-actions
Copy link

github-actions bot commented Sep 5, 2023

Revision: f204bab

Submitted crossbow builds: ursacomputing/crossbow @ actions-00d8e7b991

Task Status
test-cuda-python Github Actions

@zeroshade zeroshade merged commit 13f0cd8 into apache:main Sep 5, 2023
34 checks passed
@zeroshade zeroshade removed the awaiting change review Awaiting change review label Sep 5, 2023
@zeroshade zeroshade deleted the cuda-macro-build-fix branch September 5, 2023 15:33
@github-actions github-actions bot added the awaiting changes Awaiting changes label Sep 5, 2023
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 13f0cd8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…UDA dependencies (apache#37497)

### Rationale for this change
apache#37365 was built locally using a newer version of the CUDA driver API than the crossbow builds run with causing the crossbow build to fail due to a change in macro names. This puts the macro name back to allow it to build with the older version of the cuda driver api.

* Closes: apache#37523

Authored-by: Matt Topol <[email protected]>
Signed-off-by: Matt Topol <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…UDA dependencies (apache#37497)

### Rationale for this change
apache#37365 was built locally using a newer version of the CUDA driver API than the crossbow builds run with causing the crossbow build to fail due to a change in macro names. This puts the macro name back to allow it to build with the older version of the cuda driver api.

* Closes: apache#37523

Authored-by: Matt Topol <[email protected]>
Signed-off-by: Matt Topol <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C++][CI][CUDA] CUDA Crossbow build failure
4 participants