Fix arrow::cuda static link error (#460)

* fix missing nccl target, remove imported global hack, ensure arrow_libraries var is exported to the outer scope so rapidsai_cudf links them * update yarn.lock * remove outdated scatter test, update readText tests * update zfill test * catch errors in column.getValue * fix DataFrame.flattenIndices test failure * fix finding thrift on rebuilds
rapidsai · Jun 2, 2023 · bfdd7e3 · bfdd7e3
1 parent 5b10bc2
commit bfdd7e3
Show file tree

Hide file tree

Showing 18 changed files with 351 additions and 377 deletions.
diff --git a/modules/core/cmake/Modules/ConfigureArrow.cmake b/modules/core/cmake/Modules/ConfigureArrow.cmake
@@ -324,6 +324,14 @@ function(find_and_configure_arrow VERSION BUILD_STATIC ENABLE_S3 ENABLE_ORC ENAB
 
       set(parquet_code_string
           [=[
+              if("${THRIFT_CMAKE_DIR}" STREQUAL "")
+                set(THRIFT_CMAKE_DIR "${CMAKE_CURRENT_LIST_DIR}/thrift_ep-install/lib/cmake/thrift")
+              endif()
+
+              if(EXISTS ${THRIFT_CMAKE_DIR}/thriftTargets.cmake AND (NOT TARGET thrift::thrift))
+                include("${THRIFT_CMAKE_DIR}/thriftTargets.cmake")
+              endif()
+
               if (TARGET cudf::parquet_shared AND (NOT TARGET parquet_shared))
                   add_library(parquet_shared ALIAS cudf::parquet_shared)
               endif()
@@ -351,47 +359,51 @@ function(find_and_configure_arrow VERSION BUILD_STATIC ENABLE_S3 ENABLE_ORC ENAB
     endif()
 
     set(PROJECT_BINARY_DIR "${PROJECT_BINARY_DIR_prev}")
-  endif()
-  # We generate the arrow-config and arrowcuda-config files when we built arrow locally, so always
-  # do `find_dependency`
-  rapids_export_package(BUILD Arrow ${PROJECT_NAME}-exports)
-  rapids_export_package(INSTALL Arrow ${PROJECT_NAME}-exports)
-
-  # We have to generate the find_dependency(ArrowCUDA) ourselves since we need to specify
-  # ArrowCUDA_DIR to be where Arrow was found, since Arrow packages ArrowCUDA.config in a
-  # non-standard location
-  rapids_export_package(BUILD ArrowCUDA ${PROJECT_NAME}-exports)
-  if(ENABLE_PARQUET)
-    rapids_export_package(BUILD Parquet ${PROJECT_NAME}-exports)
-    rapids_export_package(BUILD ArrowDataset ${PROJECT_NAME}-exports)
-  endif()
 
-  include("${rapids-cmake-dir}/export/find_package_root.cmake")
-  rapids_export_find_package_root(BUILD Arrow "${Arrow_BINARY_DIR}" ${PROJECT_NAME}-exports)
-  rapids_export_find_package_root(BUILD ArrowCUDA "${Arrow_BINARY_DIR}" ${PROJECT_NAME}-exports)
-  if(ENABLE_PARQUET)
-    rapids_export_find_package_root(BUILD Parquet "${Arrow_BINARY_DIR}" ${PROJECT_NAME}-exports)
-    rapids_export_find_package_root(BUILD ArrowDataset "${Arrow_BINARY_DIR}" ${PROJECT_NAME}-exports)
+    # We generate the arrow-config and arrowcuda-config files when we built arrow locally, so always
+    # do `find_dependency`
+    rapids_export_package(BUILD Arrow ${PROJECT_NAME}-exports)
+    rapids_export_package(INSTALL Arrow ${PROJECT_NAME}-exports)
+
+    # We have to generate the find_dependency(ArrowCUDA) ourselves since we need to specify
+    # ArrowCUDA_DIR to be where Arrow was found, since Arrow packages ArrowCUDA.config in a
+    # non-standard location
+    rapids_export_package(BUILD ArrowCUDA ${PROJECT_NAME}-exports)
+    if(ENABLE_PARQUET)
+      rapids_export_package(BUILD Parquet ${PROJECT_NAME}-exports)
+      rapids_export_package(BUILD ArrowDataset ${PROJECT_NAME}-exports)
+    endif()
+
+    include("${rapids-cmake-dir}/export/find_package_root.cmake")
+    rapids_export_find_package_root(BUILD Arrow "${Arrow_BINARY_DIR}" ${PROJECT_NAME}-exports)
+    rapids_export_find_package_root(BUILD ArrowCUDA "${Arrow_BINARY_DIR}" ${PROJECT_NAME}-exports)
+    if(ENABLE_PARQUET)
+      rapids_export_find_package_root(BUILD Parquet "${Arrow_BINARY_DIR}" ${PROJECT_NAME}-exports)
+      rapids_export_find_package_root(BUILD ArrowDataset "${Arrow_BINARY_DIR}" ${PROJECT_NAME}-exports)
+    endif()
   endif()
 
   set(ARROW_FOUND
-      "${ARROW_FOUND}"
+      ${ARROW_FOUND}
       PARENT_SCOPE
   )
   set(ARROW_LIBRARIES
-      "${ARROW_LIBRARIES}"
+      ${ARROW_LIBRARIES}
       PARENT_SCOPE
   )
 
-  # Make sure consumers of our libs can see arrow libs
-  _fix_cmake_global_defaults(arrow_shared)
-  _fix_cmake_global_defaults(arrow_static)
-  _fix_cmake_global_defaults(parquet_shared)
-  _fix_cmake_global_defaults(parquet_static)
-  _fix_cmake_global_defaults(arrow_cuda_shared)
-  _fix_cmake_global_defaults(arrow_cuda_static)
-  _fix_cmake_global_defaults(arrow_dataset_shared)
-  _fix_cmake_global_defaults(arrow_dataset_static)
+  if(NOT ("${arrow_code_string}" STREQUAL ""))
+    cmake_language(EVAL CODE "${arrow_code_string}")
+  endif()
+  if(NOT ("${arrow_cuda_code_string}" STREQUAL ""))
+    cmake_language(EVAL CODE "${arrow_cuda_code_string}")
+  endif()
+  if(NOT ("${arrow_dataset_code_string}" STREQUAL ""))
+    cmake_language(EVAL CODE "${arrow_dataset_code_string}")
+  endif()
+  if(NOT ("${parquet_code_string}" STREQUAL ""))
+    cmake_language(EVAL CODE "${parquet_code_string}")
+  endif()
 
 endfunction()
 

diff --git a/modules/core/cmake/Modules/ConfigureBlazingSQL.cmake b/modules/core/cmake/Modules/ConfigureBlazingSQL.cmake
@@ -61,9 +61,6 @@ function(find_and_configure_blazingsql)
         )
     endif()
 
-    # Make sure consumers of our libs can see blazingdb::blazingsql-io
-    _fix_cmake_global_defaults(blazingdb::blazingsql-io)
-
     set(blazingsql-io_VERSION "${blazingsql-io_VERSION}" PARENT_SCOPE)
 
     _get_rapidsai_module_version(blazingsql-engine VERSION)
@@ -102,9 +99,6 @@ function(find_and_configure_blazingsql)
         )
     endif()
 
-    # Make sure consumers of our libs can see blazingdb::blazingsql-engine
-    _fix_cmake_global_defaults(blazingdb::blazingsql-engine)
-
     set(blazingsql-engine_VERSION "${blazingsql-engine_VERSION}" PARENT_SCOPE)
 
     if (NOT TARGET thrift::thrift)

diff --git a/modules/core/cmake/Modules/ConfigureCUDF.cmake b/modules/core/cmake/Modules/ConfigureCUDF.cmake
@@ -1,5 +1,5 @@
 #=============================================================================
-# Copyright (c) 2020-2022, NVIDIA CORPORATION.
+# Copyright (c) 2020-2023, NVIDIA CORPORATION.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -68,14 +68,8 @@ function(find_and_configure_cudf)
                                 "CUDF_USE_PER_THREAD_DEFAULT_STREAM ON")
     endif()
 
-    # Make sure consumers of our libs can see cudf::cudf
-    _fix_cmake_global_defaults(cudf::cudf)
-    # Make sure consumers of our libs can see nvcomp::nvcomp
-    _fix_cmake_global_defaults(nvcomp::nvcomp)
-    # Make sure consumers of our libs can see cudf::cudftestutil
-    _fix_cmake_global_defaults(cudf::cudftestutil)
-
     set(cudf_VERSION "${cudf_VERSION}" PARENT_SCOPE)
+    set(ARROW_LIBRARIES ${ARROW_LIBRARIES} PARENT_SCOPE)
 
     _set_package_dir_if_exists(nvcomp nvcomp)
     find_package(nvcomp)

diff --git a/modules/core/cmake/Modules/ConfigureCUGRAPH.cmake b/modules/core/cmake/Modules/ConfigureCUGRAPH.cmake
@@ -52,8 +52,6 @@ function(find_and_configure_cugraph)
             PATCH_COMMAND          patch --reject-file=- -p1 -N < ${CMAKE_CURRENT_LIST_DIR}/../patches/cugraph.patch || true
         )
     endif()
-    # Make sure consumers of our libs can see cugraph::cugraph
-    _fix_cmake_global_defaults(cugraph::cugraph)
 
     if(NOT TARGET cugraph::cuHornet AND
       (NOT DEFINED ENV{NODE_RAPIDS_USE_LOCAL_DEPS_BUILD_DIRS}))

diff --git a/modules/core/cmake/Modules/ConfigureCUGRAPHOPS.cmake b/modules/core/cmake/Modules/ConfigureCUGRAPHOPS.cmake
@@ -45,10 +45,6 @@ function(find_and_configure_cugraph_ops)
                                    "CUDA_STATIC_RUNTIME ON"
                                    "BUILD_CUGRAPH_OPS_CPP_TESTS OFF")
     endif()
-    # Make sure consumers of our libs can see cugraph-ops::Thrust
-    _fix_cmake_global_defaults(cugraph-ops::Thrust)
-    # Make sure consumers of our libs can see cugraph-ops::cugraph-ops++
-    _fix_cmake_global_defaults(cugraph-ops::cugraph-ops++)
 
     include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/link_utils.cmake)
     _statically_link_cuda_toolkit_libs(cugraph-ops++)

diff --git a/modules/core/cmake/Modules/ConfigureCUML.cmake b/modules/core/cmake/Modules/ConfigureCUML.cmake
@@ -1,5 +1,5 @@
 #=============================================================================
-# Copyright (c) 2021-2022, NVIDIA CORPORATION.
+# Copyright (c) 2021-2023, NVIDIA CORPORATION.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -18,6 +18,7 @@ include_guard(GLOBAL)
 function(find_and_configure_cuml)
 
     include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/get_cpm.cmake)
+    include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/get_nccl.cmake)
     include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/get_version.cmake)
     include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/ConfigureCUMLPRIMS.cmake)
 
@@ -73,9 +74,6 @@ function(find_and_configure_cuml)
         )
     endif()
 
-    # Make sure consumers of our libs can see cuml::cuml++
-    _fix_cmake_global_defaults(cuml::cuml++)
-
     include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/link_utils.cmake)
     _statically_link_cuda_toolkit_libs(cuml::cuml++)
 

diff --git a/modules/core/cmake/Modules/ConfigureCUMLPRIMS.cmake b/modules/core/cmake/Modules/ConfigureCUMLPRIMS.cmake
@@ -45,8 +45,6 @@ function(find_and_configure_cumlprims_mg)
                                    "DETECT_CONDA_ENV OFF"
                                    "BUILD_SHARED_LIBS OFF")
     endif()
-    # Make sure consumers of our libs can see cumlprims_mg::cumlprims_mg
-    _fix_cmake_global_defaults(cumlprims_mg::cumlprims_mg)
 
     include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/link_utils.cmake)
     _statically_link_cuda_toolkit_libs(cumlprims_mg::cumlprims_mg)

diff --git a/modules/core/cmake/Modules/ConfigureCUSPATIAL.cmake b/modules/core/cmake/Modules/ConfigureCUSPATIAL.cmake
@@ -51,8 +51,6 @@ function(find_and_configure_cuspatial)
                                    "PER_THREAD_DEFAULT_STREAM ON"
                                    "DISABLE_DEPRECATION_WARNING ON")
     endif()
-    # Make sure consumers of our libs can see cuspatial::cuspatial
-    _fix_cmake_global_defaults(cuspatial::cuspatial)
 
     include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/link_utils.cmake)
     _statically_link_cuda_toolkit_libs(cuspatial::cuspatial)

diff --git a/modules/core/cmake/Modules/ConfigureCXX.cmake b/modules/core/cmake/Modules/ConfigureCXX.cmake
@@ -1,5 +1,5 @@
 #=============================================================================
-# Copyright (c) 2020-2022, NVIDIA CORPORATION.
+# Copyright (c) 2020-2023, NVIDIA CORPORATION.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -95,3 +95,5 @@ endif(DISABLE_DEPRECATION_WARNINGS)
 
 # Enable -fPIC for all libs
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)
+# https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_PACKAGE_TARGETS_GLOBAL.html#variable:CMAKE_FIND_PACKAGE_TARGETS_GLOBAL
+set(CMAKE_FIND_PACKAGE_TARGETS_GLOBAL ON)
diff --git a/modules/core/cmake/Modules/ConfigureRAFT.cmake b/modules/core/cmake/Modules/ConfigureRAFT.cmake
@@ -52,8 +52,6 @@ function(find_and_configure_raft)
                                    "RAFT_USE_FAISS_STATIC ON"
                                    "RAFT_COMPILE_LIBRARIES ON")
     endif()
-    # Make sure consumers of our libs can see raft::raft
-    _fix_cmake_global_defaults(raft::raft)
     # Make these -isystem so -Werror doesn't fail their builds
     _set_interface_include_dirs_as_system(faiss::faiss)
 

diff --git a/modules/core/cmake/Modules/ConfigureRMM.cmake b/modules/core/cmake/Modules/ConfigureRMM.cmake
@@ -40,10 +40,6 @@ function(find_and_configure_rmm)
                                   "BUILD_BENCHMARKS OFF"
                                   "DISABLE_DEPRECATION_WARNING ${DISABLE_DEPRECATION_WARNINGS}")
     endif()
-    # Make sure consumers of our libs can see rmm::rmm
-    _fix_cmake_global_defaults(rmm::rmm)
-    _fix_cmake_global_defaults(rmm::Thrust)
-    _fix_cmake_global_defaults(rmm::spdlog_header_only)
 
     set(rmm_VERSION "${rmm_VERSION}" PARENT_SCOPE)
 endfunction()

diff --git a/modules/core/cmake/Modules/get_cpm.cmake b/modules/core/cmake/Modules/get_cpm.cmake
@@ -104,22 +104,6 @@ function(_clean_build_dirs_if_not_fully_built dir libname)
     endif()
 endfunction()
 
-# If a target is installed, found by the `find_package` step of CPMFindPackage,
-# and marked as IMPORTED, make it globally accessible to consumers of our libs.
-function(_fix_cmake_global_defaults target)
-    if(TARGET ${target})
-        get_target_property(_is_imported ${target} IMPORTED)
-        get_target_property(_already_global ${target} IMPORTED_GLOBAL)
-        if(_is_imported AND NOT _already_global)
-            set_target_properties(${target} PROPERTIES IMPORTED_GLOBAL TRUE)
-        endif()
-        get_target_property(_aliased_target ${target} ALIASED_TARGET)
-        if (_aliased_target)
-            _fix_cmake_global_defaults(${_aliased_target})
-        endif()
-    endif()
-endfunction()
-
 function(_set_interface_include_dirs_as_system target)
     get_target_property(_real ${target} ALIASED_TARGET)
     if (NOT TARGET ${_real})

diff --git a/modules/core/cmake/Modules/get_nccl.cmake b/modules/core/cmake/Modules/get_nccl.cmake
@@ -1,5 +1,5 @@
 #=============================================================================
-# Copyright (c) 2022, NVIDIA CORPORATION.
+# Copyright (c) 2022-2023, NVIDIA CORPORATION.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -17,19 +17,22 @@ include_guard(GLOBAL)
 
 function(find_and_configure_nccl)
 
-    if(TARGET NCCL::NCCL)
-        return()
+    if((NOT TARGET NCCL::NCCL) AND (NOT TARGET nccl::nccl))
+        rapids_find_generate_module(NCCL
+            HEADER_NAMES  nccl.h
+            LIBRARY_NAMES nccl
+        )
+        # Currently NCCL has no CMake build-system so we require
+        # it built and installed on the machine already
+        rapids_find_package(NCCL REQUIRED)
     endif()
 
-    rapids_find_generate_module(NCCL
-        HEADER_NAMES  nccl.h
-        LIBRARY_NAMES nccl
-    )
-
-    # Currently NCCL has no CMake build-system so we require
-    # it built and installed on the machine already
-    rapids_find_package(NCCL REQUIRED)
-
+    if (TARGET nccl::nccl AND (NOT TARGET NCCL::NCCL))
+        add_library(NCCL::NCCL ALIAS nccl::nccl)
+    endif()
+    if (TARGET NCCL::NCCL AND (NOT TARGET nccl::nccl))
+        add_library(nccl::nccl ALIAS NCCL::NCCL)
+    endif()
 endfunction()
 
 find_and_configure_nccl()
diff --git a/modules/cudf/src/column.cpp b/modules/cudf/src/column.cpp
@@ -530,7 +530,9 @@ Napi::Value Column::get_child(Napi::CallbackInfo const& info) {
 }
 
 Napi::Value Column::get_value(Napi::CallbackInfo const& info) {
-  return Napi::Value::From(info.Env(), cudf::get_element(*this, info[0].ToNumber()));
+  try {
+    return Napi::Value::From(info.Env(), cudf::get_element(*this, info[0].ToNumber()));
+  } catch (std::exception const& e) { throw Napi::Error::New(Env(), e.what()); }
 }
 
 }  // namespace nv
diff --git a/modules/cudf/src/data_frame.ts b/modules/cudf/src/data_frame.ts
@@ -719,12 +719,11 @@ export class DataFrame<T extends TypeMap = any> {
                  const table = includeNulls ? df.asTable().explodeOuterPosition(i, mr)  //
                                             : df.asTable().explodePosition(i, mr);
                  return new DataFrame(df.names.reduce((series_map, name, index) => {
-                   if (index <= i) {
-                     series_map[name] =
-                       (this.get(name) as any).elements.__construct(table.getColumnByIndex(index));
+                   if (index === i) {
+                     series_map[name] = Series.new(table.getColumnByIndex(index));
                    } else {
                      series_map[name] =
-                       df.__constructChild(name, table.getColumnByIndex(index + 1));
+                       df.__constructChild(name, table.getColumnByIndex(+(index >= i) + index));
                    }
                    return series_map;
                  }, {} as SeriesMap<U>));

diff --git a/modules/cudf/test/cudf-series-test.ts b/modules/cudf/test/cudf-series-test.ts
@@ -428,21 +428,6 @@ test('Series.scatter (scalar with array indicies)', () => {
   expect([...result]).toEqual([0, 1, 999, 3, 999, 999, 6, 7, 999, 9]);
 });
 
-test('Series.scatter (check_bounds)', () => {
-  const col          = Series.new({type: new Int32, data: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]});
-  const values       = Series.new({type: new Int32, data: [200, 400, 500, 800]});
-  const good_indices = [2, 4, 5, 8];
-  const bad_indices  = [2, 4, 5, 18];
-
-  const result = col.scatter(values, good_indices, true)
-                   .scatter(999, good_indices, true)
-                   .scatter(values, bad_indices)
-                   .scatter(999, bad_indices);
-
-  expect(() => result.scatter(values, bad_indices, true)).toThrowError();
-  expect(() => result.scatter(999, bad_indices, true)).toThrowError();
-});
-
 test('Series.scatter (scalar)', () => {
   const col     = Series.new({type: new Int32, data: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]});
   const indices = Series.new({type: new Int32, data: [2, 4, 5, 8]});
@@ -875,7 +860,7 @@ describe('Series.readText', () => {
     const path         = Path.join(readTextTmpDir, 'simple.txt');
     await promises.writeFile(path, outputString);
     const text = Series.readText(path, '');
-    expect(text.getValue(0)).toEqual(outputString);
+    expect(text.toArray()).toEqual(outputString.split(''));
     await new Promise<void>((resolve, reject) =>
                               rimraf(path, (err?: Error|null) => err ? reject(err) : resolve()));
   });
@@ -884,7 +869,7 @@ describe('Series.readText', () => {
     const path         = Path.join(readTextTmpDir, 'simple.txt');
     await promises.writeFile(path, outputString);
     const text = Series.readText(path, '');
-    expect(text.getValue(0)).toEqual(outputString);
+    expect(text.toArray()).toEqual(outputString.split(''));
     await new Promise<void>((resolve, reject) =>
                               rimraf(path, (err?: Error|null) => err ? reject(err) : resolve()));
   });
@@ -893,7 +878,7 @@ describe('Series.readText', () => {
     const path         = Path.join(readTextTmpDir, 'simple.txt');
     await promises.writeFile(path, outputString);
     const text = Series.readText(path, '');
-    expect(text.getValue(0)).toEqual(outputString);
+    expect(text.toArray()).toEqual(outputString.split(''));
     await new Promise<void>((resolve, reject) =>
                               rimraf(path, (err?: Error|null) => err ? reject(err) : resolve()));
   });

diff --git a/modules/cudf/test/series/string-tests.ts b/modules/cudf/test/series/string-tests.ts
@@ -219,7 +219,7 @@ test('Series.pad (right, fill)', () => {
 
 test('Series.zfill', () => {
   const a = Series.new(['1234', '-9876', '+0.34', '-342567', null]);
-  expect([...a.zfill(6)]).toStrictEqual(['001234', '0-9876', '0+0.34', '-342567', null]);
+  expect([...a.zfill(6)]).toStrictEqual(['001234', '-09876', '+00.34', '-342567', null]);
 });
 
 test('Series.isHex', () => {