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

Example to build custom application and link to libcudf #7671

Merged
merged 33 commits into from
Jun 18, 2021

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Mar 22, 2021

closes #4824, also closes #2547

This PR introduces a basic libcudf example to demonstrate building custom application and link to libcudf using CMake. Libcudf examples now lives under cpp/examples folder. There's a examples/build.sh that manages building and linking examples. Besides, this PR adds examples into gpuCI tests to make sure examples still builds as the main package evolves.

@isVoid isVoid added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue container 5 - DO NOT MERGE Hold off on merging; see PR for details labels Mar 22, 2021
@isVoid isVoid requested review from a team as code owners March 22, 2021 20:22
@harrism
Copy link
Member

harrism commented Mar 22, 2021

I don't think we should skip CI. I think we should have simple examples like this inside of the libcudf repo and we should CI them so that we catch breakages.

@isVoid isVoid changed the title [skip-ci] Example to build custom application and link to libcudf Example to build custom application and link to libcudf Mar 22, 2021
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Thanks for creating this. Mostly just minor style / simplification suggestions.

cpp/example/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/example/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/example/Dockerfile Outdated Show resolved Hide resolved
cpp/example/Dockerfile Outdated Show resolved Hide resolved
cpp/example/README.md Outdated Show resolved Hide resolved
cpp/example/src/process_csv.cpp Outdated Show resolved Hide resolved
cpp/example/src/process_csv.cpp Outdated Show resolved Hide resolved
cpp/example/src/process_csv.cpp Outdated Show resolved Hide resolved
cpp/example/src/process_csv.cpp Outdated Show resolved Hide resolved
cpp/example/src/process_csv.cpp Outdated Show resolved Hide resolved
@harrism
Copy link
Member

harrism commented Mar 22, 2021

One other thing: I think you should prepare for future libcudf examples, this is not likely to be the only one. So instead of putting it in /cpp/example, call the top level directory examples, and make a subdirectory for this example, e.g. /cpp/examples/basic.

Co-authored-by: Mark Harris <mharris@nvidia.com>
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@d1deca8). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #7671   +/-   ##
===============================================
  Coverage                ?   82.97%           
===============================================
  Files                   ?      109           
  Lines                   ?    18214           
  Branches                ?        0           
===============================================
  Hits                    ?    15113           
  Misses                  ?     3101           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1deca8...41a6e3f. Read the comment docs.

@harrism
Copy link
Member

harrism commented Mar 24, 2021

@ajschmidt8 already approved for ops, but IMO we should get CI working on this example before it is merged...

@harrism harrism added feature request New feature or request non-breaking Non-breaking change labels Mar 24, 2021
@isVoid
Copy link
Contributor Author

isVoid commented Mar 24, 2021

A few updates:

  • Code is more concise, removed helper function get_columns_from_table
  • Removed memory pool init
  • Removed ccache

@isVoid isVoid removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Mar 24, 2021
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Nice example. Thanks.

@harrism harrism requested a review from ttnghia May 26, 2021 22:36
# Add libcudf examples build scripts down below

# Parallelism control
PARALLEL_LEVEL=${PARALLEL_LEVEL:-4}
Copy link
Contributor

@ttnghia ttnghia May 26, 2021

Choose a reason for hiding this comment

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

I'm not an expert in cmake/shell thus I'm not sure if this will result in -j 4 or -j -4?

Copy link
Member

Choose a reason for hiding this comment

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

Seems to be the same as here:

export PARALLEL_LEVEL=${PARALLEL_LEVEL:-4}

Copy link
Contributor

@ttnghia ttnghia May 26, 2021

Choose a reason for hiding this comment

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

I got it! We will have 4, not -4 so everything will be fine. The shell syntax is a bit confusing:

${parameter:-word}
    If parameter is unset or null, the expansion of word is substituted. 
    Otherwise, the value of parameter is substituted.

Ref: https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

@harrism harrism added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels May 26, 2021
@harrism
Copy link
Member

harrism commented May 26, 2021

@gpucibot merge

@harrism
Copy link
Member

harrism commented May 26, 2021

rerun tests

@isVoid
Copy link
Contributor Author

isVoid commented May 26, 2021

@harrism should this rebase to 21.08? (rebasing since I see it on 21.08 board)

@isVoid isVoid changed the base branch from branch-21.06 to branch-21.08 May 26, 2021 23:54
@harrism
Copy link
Member

harrism commented May 27, 2021

Oh, OK. I thought it was fine to go in 21.06.

@galipremsagar
Copy link
Contributor

rerun tests

@davidwendt
Copy link
Contributor

Would this also close issue #2547 ?

@isVoid
Copy link
Contributor Author

isVoid commented Jun 7, 2021

rerun tests

@isVoid
Copy link
Contributor Author

isVoid commented Jun 8, 2021

Looks like the failed CI builds are due to mis-configuration of paths in build.sh. Investigating.

@isVoid isVoid added 0 - Waiting on Author Waiting for author to respond to review and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels Jun 8, 2021
cpp/examples/basic/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/examples/basic/CMakeLists.txt Outdated Show resolved Hide resolved
@isVoid
Copy link
Contributor Author

isVoid commented Jun 17, 2021

Thanks to @robertmaynard pointing out, that the examples should not build from main, because older examples may break with latest releases. Instead, they should explicitly link to the latest version, this way to which version the examples should work with is clear.

This requires auto bumping the version from update-version.sh, which is updated accordingly.

Requires re-review from @rapidsai/ops-codeowners

@rapids-bot rapids-bot bot merged commit adaca18 into rapidsai:branch-21.08 Jun 18, 2021
@isVoid
Copy link
Contributor Author

isVoid commented Jun 18, 2021

Oops. I forgot to block merging. @robertmaynard can you still look at the CMake file and raise an issue for it?

@robertmaynard
Copy link
Contributor

robertmaynard commented Jun 18, 2021

In my mind examples should have as little 'noise' so that readers can easily find
the important bits. To this end we would want to do the following:

Refactor the add_executable and target_link_libaries lines to look like:

add_executable(basic_example src/process_csv.cpp)
target_link_libraries(basic_example PRIVATE cudf::cudf)

This has multiple improvements:

  • We use real names instead of variables, making it easier to read
  • We don't quote relative paths, since that is uneeded
  • We use the Modern CMake pattern of marking target_link_libraries libraries
    as public/private/interface.

Drop the set(CMAKE_CUDA_ARCHITECTURES "") line. The can be specified in the
build.sh. If we are wanting a nicer API for specifying which cuda archs we are
building for we could use rapids_cmake

Drop the set(CMAKE_EXPORT_COMPILE_COMMANDS ON) entirely. This is only needed for
some autocomplete tools.

Drop the set(CMAKE_CXX_STANDARD 17) line and instead at the end of the file do:

target_compile_features(basic_example PRIVATE cxx_std_17)

The project(basic_example is enabling the CUDA and C languages which aren't
needed to use cudf so we should drop them. If cudf is subsequently built
from source via CPM it will enable the CUDA language when needed

I think we can clean up the logic around downloading and using CPM, it currently has some logic that really only makes sense for 'production' projects. Plus it is using an old CPM version fetched from the old project URL.

I think we can simplify the cpm logic to be:

set(CPM_DOWNLOAD_VERSION v0.32.2)
file(DOWNLOAD https://github.com/cpm-cmake/CPM.cmake/releases/download/${CPM_DOWNLOAD_VERSION}/get_cpm.cmake ${CMAKE_BINARY_DIR}/cmake/get_cpm.cmake)
include(${CMAKE_BINARY_DIR}/cmake/get_cpm.cmake)

set(CUDF_VERSION 21.08)
CPMFindPackage(NAME  cudf
    GIT_REPOSITORY  https://github.com/rapidsai/cudf
    GIT_TAG         branch-${VERSION}
    GIT_SHALLOW     TRUE
    SOURCE_SUBDIR   cpp
)

Total end result would be the CMakeLists.txt looking like:

cmake_minimum_required(VERSION 3.18)

project(basic_example VERSION 0.0.1 LANGUAGES CXX)

set(CPM_DOWNLOAD_VERSION v0.32.2)
file(DOWNLOAD https://github.com/cpm-cmake/CPM.cmake/releases/download/${CPM_DOWNLOAD_VERSION}/get_cpm.cmake ${CMAKE_BINARY_DIR}/cmake/get_cpm.cmake)
include(${CMAKE_BINARY_DIR}/cmake/get_cpm.cmake)

set(CUDF_TAG branch-21.08)
CPMFindPackage(NAME  cudf
    GIT_REPOSITORY  https://github.com/rapidsai/cudf
    GIT_TAG         ${CUDF_TAG}
    GIT_SHALLOW     TRUE
    SOURCE_SUBDIR   cpp
)

# Configure your project here
add_executable(basic_example src/process_csv.cpp)
target_link_libraries(basic_example PRIVATE cudf::cudf)
target_compile_features(basic_example PRIVATE cxx_std_17)

rapids-bot bot pushed a commit that referenced this pull request Jun 22, 2021
)

Follow up of #7671 

This PR addresses review comments from #7671 (comment), modernizing the CMakeFile in libcudf basic example.

This PR also updates build tests for examples to make sure they are tested after both regular/project flash code path.

Authors:
  - Michael Wang (https://github.com/isVoid)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Ray Douglass (https://github.com/raydouglass)
  - Mark Harris (https://github.com/harrism)

URL: #8568
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Waiting on Author Waiting for author to respond to review CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Create example C++ application using cudf APIs [FEA][DOC] Need examples folder for cudf cpp
9 participants