Fix RAFT_NVTX option not set

[achirkin]

Pass NVTX option to raft in a more similar way to the other arguments and make sure RAFT_NVTX option in the installed raft-config.cmake.

[achirkin]

Another alternative to this would be to just rename NVTX option in CMakeLists.txt to RAFT_NVTX.

[cjnolet]

rerun tests

[cjnolet]

@robertmaynard, hoping you might have an idea of why this doesn't seem to be turning on the option in RAFT. @achirkin mentioned that the explicit set(RAFT_NVTX ON) is needed in order for the RAFT to pick up the option.

[robertmaynard]

We should not pass it as an argument to the find, but only set it before hand. I recommend this approach:

@@ -45,11 +45,11 @@ function(find_and_configure_raft)
         set(RAFT_COMPILE_LIBRARIES OFF)
     endif()
 
-    message(VERBOSE "CUML: raft FIND_PACKAGE_ARGUMENTS COMPONENTS ${RAFT_COMPONENTS}")
+    # We need to set this each time so that on subsequent calls to cmake
+    # the raft-config.cmake re-evaluates the RAFT_NVTX value
+    set(RAFT_NVTX ${PKG_NVTX})
 
-    if(PKG_NVTX)
-      set(RAFT_NVTX ON)
-    endif()
+    message(VERBOSE "CUML: raft FIND_PACKAGE_ARGUMENTS COMPONENTS ${RAFT_COMPONENTS}")
 
     rapids_cpm_find(raft ${PKG_VERSION}
             GLOBAL_TARGETS      raft::raft
@@ -66,7 +66,6 @@ function(find_and_configure_raft)
               "RAFT_COMPILE_NN_LIBRARY ${PKG_USE_RAFT_NN}"
               "RAFT_COMPILE_DIST_LIBRARY ${PKG_USE_RAFT_DIST}"
               "RAFT_USE_FAISS_STATIC ${PKG_USE_FAISS_STATIC}"
-              "RAFT_NVTX ${PKG_NVTX}"
     )
 
     if(raft_ADDED)

The issue is around using an existing version of raft ( from conda ) and changing the value of RAFT_NVTX for multiple invocations of cmake. After the first invocation of CMake, rapids has been found and the arguments to rapids_cpm_find are not used anymore.

So to consistently support NVTX values changing we want to set it as a local variable so that the following raft-config.cmake logic always sees it: https://github.com/rapidsai/raft/blob/branch-22.08/cpp/CMakeLists.txt#L206

[cjnolet]

Ah ha, that totally makes sense. Thanks for explaining!

[cjnolet]

I think it would be better, while we are already making changes to get_raft.cmake, to remove the RAFT_NVTX argument from rapids_cpm_find altogether (per Robert Maynard's suggesiton).

[codecov-commenter]

Codecov Report

Merging #4825 (b2e51ae) into branch-22.08 (97941e3) will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           branch-22.08    #4825   +/-   ##
=============================================
  Coverage         77.62%   77.62%           
=============================================
  Files               180      180           
  Lines             11384    11384           
=============================================
  Hits               8837     8837           
  Misses             2547     2547           

Flag	Coverage Δ
dask	45.52% <ø> (ø)
non-dask	67.26% <ø> (ø)

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 97941e3...b2e51ae. Read the comment docs.

[cjnolet]

@gpucibot merge