Fix FindNVTX.cmake

[maxhgerlach]

This fixes a bug that I've introduced with #3261.

NVTX would not be found when building with HOROVOD_CUDA_HOME to locate CUDA.

[nvcastet]

do you need to put ${CUDAToolkit_INCLUDE_DIRS} on line 19 as an extra hint to search include?
CMAKE_PREFIX_PATH takes the root dir and append bin, lib, or include for the appropriate search: https://cmake.org/cmake/help/latest/variable/CMAKE_PREFIX_PATH.html

[maxhgerlach]

On my system, setting NVTX_ROOT properly seems to have already done the trick.

Would you say that adding this extra hint would help even more (on other systems)?

[maxhgerlach]

For CMake 3.12+ (we require 3.13 now), setting <package>_ROOT correctly should be sufficient for find_path: https://cmake.org/cmake/help/latest/command/find_path.html

[nvcastet]

It potentially ends up to be the same but CMAKE_PREFIX_PATH is usually used for the parent (root) directory instead of the subdir include. <PackageName>_ROOT is used in the same way. Specific subdir (bin/lib/include) hints are usually added to the corresponding find_* HINTS argument.
On my container CUDAToolkit_LIBRARY_ROOT is empty, which is weird, so relying on CUDAToolkit_INCLUDE_DIRS instead seems to be a good idea, but maybe it is set in some cases?
I would just have added CUDAToolkit_INCLUDE_DIRS to the find_path:

find_path(NVTX_INCLUDE_DIR
          NAMES nvtx3/nvToolsExt.h
          HINTS ${HOROVOD_NVTX_INCLUDE} ${CUDAToolkit_INCLUDE_DIRS})

[nvcastet]

At the end as you mentioned it works in both cases and here we seem to only need to search for just the include of nvtx. Out of curiosity, do you know why we don't need to find the nvtx lib too libnvToolsExt.so?

[maxhgerlach]

Ah, understood. It would probably be clearer not to plug CUDAToolkit_INCLUDE_DIRS into NVTX_ROOT (although it works for us as we don't need the .so).

Out of curiosity, do you know why we don't need to find the nvtx lib too libnvToolsExt.so?

The Nsight Systems User Guide only speaks of the header file: https://docs.nvidia.com/nsight-systems/UserGuide/index.html#nvtx-trace Might it be necessary to explicitly link that library for use cases where one does not attach that profiler?

[nvcastet]

In fact we are using the nvtx header-only library, so the shared lib is never needed:
https://gitlab.kitware.com/cmake/cmake/-/issues/21377

[maxhgerlach]

I went with your suggestion, @nvcastet, there's no need to deal with the NVTX_ROOT variable at all here.

[EnricoMi]

Have you checked all the other places where CUDA_TOOLKIT_ROOT_DIR has been replaced with CUDAToolkit_LIBRARY_ROOT in #3261 while CUDAToolkit_INCLUDE_DIRS would have been the right one?

[maxhgerlach]

@EnricoMi, very good point. I will give the other replacements a look. Most likely they were false, too.

[maxhgerlach]

Improved two other places where I had thought CUDAToolkit_LIBRARY_ROOT would be a viable replacement for CUDA_TOOLKIT_ROOT_DIR.

[EnricoMi]

Improved two other places where I had thought CUDAToolkit_LIBRARY_ROOT would be a viable replacement for CUDA_TOOLKIT_ROOT_DIR.

Then I am happy with this, I leave this to @nvcastet to approve

[nvcastet]

Assuming CI passes, LGTM.

[github-actions]

Unit Test Results

     773 files  ±0       773 suites  ±0   8h 47m 36s ⏱️ - 19m 23s
     722 tests ±0       661 ✔️  - 14       47 💤 ±0  14 ❌ +14 
16 693 runs  ±0  11 761 ✔️  - 15  4 917 💤 ±0  15 ❌ +15 

For more details on these failures, see this check.

Results for commit fc3ebe0. ± Comparison against base commit 046c071.

[github-actions]

Unit Test Results (with flaky tests)

     951 files  +     76       951 suites  +76   11h 2m 7s ⏱️ + 58m 22s
     722 tests ±       0       659 ✔️  -      14       47 💤 ±    0  16 ❌ +14 
20 962 runs  +1 898  14 564 ✔️ +1 240  6 347 💤 +610  51 ❌ +48 

For more details on these failures, see this check.

Results for commit fc3ebe0. ± Comparison against base commit 046c071.

[maxhgerlach]

CI passes besides known failures with head framework versions (Spark Lightning, TF batchnorm, TF hvd.join on GPU)