Use NVTX from GitHub. #15178
Use NVTX from GitHub. #15178
Conversation
github-actions
bot
added
libcudf
bdice
added
feature request
cpp/cmake/thirdparty/get_nvtx.cmake
Outdated
| GIT_SHALLOW TRUE | ||
| DOWNLOAD_ONLY TRUE | ||
| ) | ||
| set(NVTX_INCLUDE_DIR |
There was a problem hiding this comment.
I get the feeling that this is not the right way to do this. I think I need to add the nvtx targets, rather than specifying the include directory directly.
The targets are defined by c/CMakeLists.txt in the nvtx repo. I googled around a bit and tried to find how to add this. Do I need something like SOURCE_SUBDIR c for it to find the targets? I'm not sure how to use this.
cc: @robertmaynard @vyasr
There was a problem hiding this comment.
Do I need something like SOURCE_SUBDIR c for it to find the targets?
Yes 🙂
https://github.com/NVIDIA/NVTX/tree/dev/c#use-cmake-package-manager-cpm
There was a problem hiding this comment.
Hmm. I tried this but wasn’t having success. I think that there is something I don’t understand about how rapids-cmake calls CPM.
There was a problem hiding this comment.
You need to use CPM _ARGS SOURCE_SUBDIR c to tell rapids-cmake to pass the option down to cpm.
There was a problem hiding this comment.
@robertmaynard I think I did that in 226101b. In my understanding, that should make the nvtx3-cpp target available, and I should be able to use target_link_libraries(cudf PRIVATE nvtx3-cpp) to get it to recognize the include directories from nvtx. However, this isn't working as I expected. Can you help me debug it?
Co-authored-by: Robert Maynard <robertjmaynard@gmail.com>
| @@ -777,7 +779,7 @@ add_dependencies(cudf jitify_preprocess_run) | |||
| target_link_libraries( | |||
| cudf | |||
| PUBLIC ${ARROW_LIBRARIES} CCCL::CCCL rmm::rmm | |||
| PRIVATE cuco::cuco ZLIB::ZLIB nvcomp::nvcomp kvikio::kvikio | |||
| PRIVATE nvtx3-cpp cuco::cuco ZLIB::ZLIB nvcomp::nvcomp kvikio::kvikio | |||
There was a problem hiding this comment.
Will need to also have the cudf tests use this target, since some tests are including internal cudf headers.
There was a problem hiding this comment.
@robertmaynard I had to do this for benchmarks, too. On second thought, should cudf be exporting this dependency somehow? Or do we expect every library using cudf to add nvtx3-cpp to their target_link_libraries?
(To be clear, I don't know the "right" way to solve this.)
There was a problem hiding this comment.
The reason that tests and benchmarks need to add the nvtx3-cpp target is that they include non 'publicheaders fromcudf/detail`.
Actual consumers of libcudf will not do so, and therefore have no need to have nvtx3 installed when building against libcudf.
There was a problem hiding this comment.
Ah, that makes sense. Thanks, I hadn’t considered that.
There was a problem hiding this comment.
For posterity, the JNI code was also consuming cudf detail headers and needed to link to NVTX.
internal detail headers shouldn't consumed outside libcudf. :) If they are needed outside libcudf, a discussion about moving things out of detail should be started. |
|
/merge |
Description
This PR removes the vendored copy of NVTX and instead fetches it from GitHub.
Note: Consumers of libcudf internal
detailheaders will need to provide their own NVTX. This can be done by using the CMake code in this PR (or the sample CMake code in the NVTX README), and callingtarget_link_libraries(your_target PRIVATE nvtx3-cpp).Closes #6476.
Checklist