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

Update to CMake 3.20 features, rapids-cmake and CPM #3844

Merged
merged 45 commits into from
May 25, 2021

Conversation

dantegd
Copy link
Member

@dantegd dantegd commented May 9, 2021

Depends on rapidsai/raft#187

Based on the work started by @mdemoret-nv on https://github.com/mdemoret-nv/cuml/tree/imp-modernize-cmake with updates for CPM, rapids-cmake and consistency with the equivalent PRs in RAFT and cuGraph

Details of changes will be added soon

cc @trxcllnt and @robertmaynard for viz

@dantegd dantegd added 2 - In Progress Currenty a work in progress Build or Dep Issues related to building the code or dependencies improvement Improvement / enhancement to an existing function labels May 9, 2021
@github-actions github-actions bot added CMake CUDA/C++ Cython / Python Cython or Python issue labels May 9, 2021
@dantegd dantegd added the non-breaking Non-breaking change label May 10, 2021
@dantegd dantegd changed the title Update to CMake 3.20 features, rapids-cmake and CPM [skip-ci] Update to CMake 3.20 features, rapids-cmake and CPM May 10, 2021
@github-actions github-actions bot added the conda conda issue label May 10, 2021
@dantegd dantegd marked this pull request as ready for review May 10, 2021 15:13
@dantegd dantegd requested review from a team as code owners May 10, 2021 15:13
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

@trxcllnt
Copy link
Collaborator

trxcllnt commented May 20, 2021

@dantegd Can we make find_package(Doxygen 1.8.12 REQUIRED) not be REQUIRED? I think that's causing configuration to fail if doxygen isn't installed.

@dantegd dantegd requested a review from a team as a code owner May 21, 2021 04:24
@@ -320,6 +321,7 @@ if(BUILD_CUML_CPP_LIBRARY)
target_link_libraries(${CUML_CPP_TARGET}
PUBLIC
rmm::rmm
cuml::Thrust
Copy link
Contributor

Choose a reason for hiding this comment

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

Thrust targets need some some extra logic to be setup by consumers.

You can see how do this here:
https://github.com/rapidsai/cugraph/pull/1585/files#diff-1bba462ab050e89360fd88110a689e85ee037749cea091a1848ab574381d3795R352

Copy link
Contributor

@robertmaynard robertmaynard May 21, 2021

Choose a reason for hiding this comment

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

If that link doesn't work:

set(code_string [=[thrust_create_target(cugraph::Thrust FROM_OPTIONS)]=]) 
rapids_export(INSTALL cugraph    
                       EXPORT_SET cugraph-exports
                       GLOBAL_TARGETS cugraph
                       NAMESPACE cugraph::
                       DOCUMENTATION doc_string
                       FINAL_CODE_BLOCK code_string    )

Copy link
Collaborator

Choose a reason for hiding this comment

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

After this change, I'm this error configuring with an RMM source-build dir:

CMake Error at /home/ptaylor/dev/rapids/rmm/cmake/install/FindThrust.cmake:59 (add_library):
  add_library cannot create target "thrust_internal" because another target
  with the same name already exists.  The existing target is an interface
  library created in source directory "/home/ptaylor/dev/rapids/cuml/cpp".
  See documentation for policy CMP0002 for more details.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like the above error is fixed by rapidsai/rmm#784

@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #3844   +/-   ##
===============================================
  Coverage                ?   85.43%           
===============================================
  Files                   ?      226           
  Lines                   ?    17281           
  Branches                ?        0           
===============================================
  Hits                    ?    14764           
  Misses                  ?     2517           
  Partials                ?        0           
Flag Coverage Δ
dask 48.96% <0.00%> (?)
non-dask 77.41% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 91e1e92...bae8ab0. Read the comment docs.

@dantegd
Copy link
Member Author

dantegd commented May 25, 2021

rerun tests

@@ -308,7 +308,7 @@ if(BUILD_CUML_CPP_LIBRARY)
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src_prims>
$<BUILD_INTERFACE:${RAFT_DIR}/cpp/include>
$<BUILD_INTERFACE:$<$<BOOL:${ENABLE_CUMLPRIMS_MG}>:${cumlprims_mg_INCLUDE_DIRS}>>
$<BUILD_INTERFACE:$<$<BOOL:${ENABLE_CUMLPRIMS_MG}>:${cumlprims_mg_INCLUDE_DIRS}>>
Copy link
Contributor

Choose a reason for hiding this comment

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

We have duplicate lines now adding ${cumlprims_mg_INCLUDE_DIRS}

@dantegd
Copy link
Member Author

dantegd commented May 25, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a8d5a87 into rapidsai:branch-21.06 May 25, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Depends on rapidsai/raft#187

Based on the work started by @mdemoret-nv on https://github.com/mdemoret-nv/cuml/tree/imp-modernize-cmake with updates for CPM, rapids-cmake and consistency with the equivalent PRs in RAFT  and cuGraph

Details of changes will be added soon

cc @trxcllnt and @robertmaynard for viz

Authors:
  - Dante Gama Dessavre (https://github.com/dantegd)
  - Michael Demoret (https://github.com/mdemoret-nv)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - AJ Schmidt (https://github.com/ajschmidt8)
  - John Zedlewski (https://github.com/JohnZed)

URL: rapidsai#3844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team Build or Dep Issues related to building the code or dependencies CMake conda conda issue CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants