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

Fix cloning, compilation and linking errors for the non-docker version #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -462,3 +462,6 @@ MigrationBackup/

# End of https://www.toptal.com/developers/gitignore/api/clion,visualstudio
/CMakeSettings.json

# Build directory
build/
11 changes: 7 additions & 4 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
[submodule "third_parties/googletest"]
path = third_parties/googletest
url = https://github.com/google/googletest.git
[submodule "third_parties/fmt"]
Copy link
Author

Choose a reason for hiding this comment

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

otherwise impossible to link

path = third_parties/fmt
url = https://github.com/fmtlib/fmt.git
[submodule "third_parties/mp11"]
Copy link
Author

Choose a reason for hiding this comment

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

otherwise impossible to clone

path = third_parties/mp11
url = git@github.com:mis-wut/mp11.git
url = https://github.com/boostorg/mp11.git
[submodule "third_parties/CLI11"]
path = third_parties/CLI11
url = https://github.com/CLIUtils/CLI11.git
[submodule "third_parties/googletest"]
path = third_parties/googletest
url = https://github.com/google/googletest
45 changes: 20 additions & 25 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,22 @@ set(CMAKE_VERBOSE_MAKEFILE ON)

project(meta-json-parser VERSION 0.1 LANGUAGES CXX CUDA)

find_package(CUDAToolkit 10.0 REQUIRED)
# As of now not using LIBCUDF throws
set(USE_LIBCUDF 1)
Copy link
Author

Choose a reason for hiding this comment

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

if not set to 1, the project throws compilation errors (ifdefs are leaky)


IF(NOT LOCAL_LIB)
find_package(Boost 1.75) # version 1.75 is required to use boost::mp11::mp_pairwise_fold_q
ENDIF()
find_package(CUDAToolkit EXACT 11.8 REQUIRED)
Copy link
Author

Choose a reason for hiding this comment

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

currently the newest is 12 (so the previous would find 12, as it only set minimum), but RAPIDS are currently limited to 11.X


IF(NOT Boost_FOUND)
add_subdirectory(third_parties/mp11)
add_library(Boost::boost ALIAS boost_mp11)
ENDIF()

IF(NOT LOCAL_LIB)
find_package(GTest REQUIRED CONFIG)
ENDIF()

IF(NOT GTest_FOUND)
add_subdirectory(third_parties/googletest)
add_library(GTest::gtest ALIAS gtest)
add_library(GTest::gmock ALIAS gmock)
ENDIF()
# Add Boost MP11 library
add_subdirectory(third_parties/mp11)
add_library(Boost::boost ALIAS boost_mp11)

# Add GoogleTest
add_subdirectory(third_parties/googletest)
include(GoogleTest)

#Add FMT library (as CUDA11.8 doesn't support gcc12)
add_subdirectory(third_parties/fmt)
Copy link
Author

Choose a reason for hiding this comment

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

as above


add_library(lib-meta-json-parser INTERFACE)

target_compile_features(lib-meta-json-parser INTERFACE cxx_std_17 cuda_std_17)
Expand Down Expand Up @@ -113,6 +106,7 @@ target_link_libraries(meta-cudf-parser-1 PUBLIC lib-meta-json-parser)

target_include_directories(meta-cudf-parser-1 PUBLIC
"${PROJECT_SOURCE_DIR}/meta_cudf/opt1"
"$ENV{CONDA_PREFIX}/include"
Copy link
Author

Choose a reason for hiding this comment

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

universal instead of hardfixed

)
target_compile_options(meta-cudf-parser-1 PRIVATE $<$<COMPILE_LANGUAGE:CUDA>:--expt-relaxed-constexpr> $<$<COMPILE_LANGUAGE:CUDA>:--extended-lambda>)

Expand Down Expand Up @@ -182,7 +176,7 @@ add_subdirectory(third_parties/CLI11)
#add_executable(meta-json-parser-benchmark benchmark/checkpoint_results.cpp benchmark/checkpoint_results.h)
add_executable(meta-json-parser-benchmark)

target_link_libraries(meta-json-parser-benchmark PRIVATE lib-meta-json-parser CLI11)
target_link_libraries(meta-json-parser-benchmark PRIVATE lib-meta-json-parser fmt::fmt CLI11)
Copy link
Author

Choose a reason for hiding this comment

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

otherwise throws linking errors

Copy link
Contributor

Choose a reason for hiding this comment

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

During which part of build (I wonder which part requires fmt library)?


# libcudf is an optional dependency for meta-json-parser-benchmark
# run cmake with -DUSE_LIBCUDF=1 to enable it
Expand All @@ -192,33 +186,34 @@ if (USE_LIBCUDF)
# instead of hardcoding paths to includes and to libraries, like below
target_include_directories(meta-cudf-parser-1 PUBLIC
/usr/local/include
/usr/local/include/libcudf/libcudacxx
Copy link
Author

Choose a reason for hiding this comment

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

couldn't find libcudacxx in any folder related to current libraries and deleting this doesn't change anything

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember it was necessary for something, perhaps for running via docker image, but might be no longer needed.

$ENV{CONDA_PREFIX}/include
Copy link
Author

Choose a reason for hiding this comment

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

universal instead of hardfixed

)
target_include_directories(meta-cudf-parser-1 AFTER PUBLIC
/opt/conda/envs/rapids/include
$ENV{CONDA_PREFIX}/include
Copy link
Author

Choose a reason for hiding this comment

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

universal instead of hardfixed

Copy link
Author

Choose a reason for hiding this comment

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

also /opt/conda/envs/rapids doesn't exist in a default conda installation of rapids

)
target_link_libraries(meta-cudf-parser-1 PRIVATE
cudf
-L/usr/local/cuda/lib64 -L/usr/local/lib
-L/opt/conda/envs/rapids/lib
-L$ENV{CONDA_PREFIX}/lib
Copy link
Author

Choose a reason for hiding this comment

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

universal instead of hardfixed

Copy link
Author

Choose a reason for hiding this comment

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

also /opt/conda/envs/rapids doesn't exist in a default conda installation of rapids

Copy link
Contributor

Choose a reason for hiding this comment

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

RAPIDS docker images used to have some of necessary includes and libs in /opt/conda/envs/rapids/ - while Dockerfile does not set CONDA_PREFIX environment variable. That's why it is here: for sudo docker build . to work.

Adding the $ENV{CONDA_PREFIX}/lib part is good, but I'd wait with removing /opt/conda/envs/rapids/ (see split commits).

)
target_compile_definitions(meta-cudf-parser-1 PRIVATE
HAVE_LIBCUDF=1
)

target_include_directories(meta-json-parser-benchmark PUBLIC
/usr/local/include
/usr/local/include/libcudf/libcudacxx
Copy link
Author

Choose a reason for hiding this comment

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

as above

$ENV{CONDA_PREFIX}/include
Copy link
Author

Choose a reason for hiding this comment

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

as above

)

# NOTE: needs to be added as the last directory to be checked, because
# it can contain outdated Boost library (which in Ubuntu 20.04 is Boost 1.72)
target_include_directories(meta-json-parser-benchmark AFTER PUBLIC
/opt/conda/envs/rapids/include
$ENV{CONDA_PREFIX}/include
Copy link
Author

Choose a reason for hiding this comment

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

as above

)
target_link_libraries(meta-json-parser-benchmark PRIVATE
cudf
-L/usr/local/cuda/lib64 -L/usr/local/lib
-L/opt/conda/envs/rapids/lib
-L$ENV{CONDA_PREFIX}/lib
Copy link
Author

Choose a reason for hiding this comment

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

universal instead of hardfixed

as above with respect to existence of /envs/rapids

)
target_compile_definitions(meta-json-parser-benchmark PRIVATE
HAVE_LIBCUDF=1
Expand Down
2 changes: 1 addition & 1 deletion include/meta_json_parser/debug_helpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ void describe_table(cudf::io::table_with_metadata& table_with_metadata, bool dum
printf("table (with metadata) has %d columns\n", n_cols);
for (int col_idx = 0; col_idx < n_cols; col_idx++) {
printf("column(%d) name is \"%s\":\n", col_idx,
table_with_metadata.metadata.column_names[col_idx].c_str());
table_with_metadata.metadata.schema_info[col_idx].name.c_str());
Copy link
Author

Choose a reason for hiding this comment

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

cuDF in new (stable) RAPIDS released has changed its API and this is the new approach. schema info is a table of structs containing strings instead of table of strings

more info here

describe_column(table.column(col_idx), dump_data);
}
}
Expand Down
1 change: 0 additions & 1 deletion include/meta_json_parser/parser_output_host.cuh
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#pragma once
#include <cuda_runtime_api.h>
#include <thrust/host_vector.h>
#include <thrust/system/cuda/experimental/pinned_allocator.h>
Copy link
Author

Choose a reason for hiding this comment

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

not used and CUDA 12 toolchain incompatible

#include <boost/mp11/list.hpp>
#include <fstream>
#include <meta_json_parser/config.h>
Expand Down
3 changes: 2 additions & 1 deletion meta_cudf/parser.cu
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ cudf::io::table_with_metadata generate_example_metadata(const char* filename, in
return "Column " + to_string(i++);
});

cudf::io::table_metadata metadata{column_names};
cudf::io::table_metadata metadata;
std::for_each(begin(column_names), end(column_names), [&](auto& elem){metadata.schema_info.push_back({elem});});
Copy link
Author

Choose a reason for hiding this comment

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

as above (cuDF API change)


return cudf::io::table_with_metadata{
make_unique<cudf::table>(cudf_table),
Expand Down
2 changes: 1 addition & 1 deletion third_parties/CLI11
Submodule CLI11 updated 162 files
1 change: 1 addition & 0 deletions third_parties/fmt
Submodule fmt added at 02cae7
2 changes: 1 addition & 1 deletion third_parties/googletest
Submodule googletest updated 245 files
2 changes: 1 addition & 1 deletion third_parties/mp11
Submodule mp11 updated from 19c1a9 to 773ec9