correct compliation of cuml c library #3908
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The C api library like the C++ api doesn't need consumers to link to faiss, so remove it from public API
The tests for the cuml C library need to ensure that the C language is enabled, as that isn't done by default since the `cuml` library uses C++ internally with `extern "C"` blocks.
The C api library like the C++ api doesn't need consumers to link to faiss, so remove it from public API, but it needs to be private. In addition correctly specify where to find the FAISS headers
dantegd
approved these changes
May 27, 2021
dantegd
added
5 - Ready to Merge
dantegd
requested changes
May 27, 2021
There was a problem hiding this comment.
Missed a comment in my first review, and after running locally stumbled into the same issue as CI:
[167/223] /usr/local/cuda-11.2/bin/nvcc -forward-unknown-to-host-compiler -DTHRUST_DEVICE_SYSTEM=THRUST_DEVICE_SYSTEM_CUDA -DTHRUST_HOST_SYSTEM=THRUST_HOST_SYSTEM_CPP -I../bench/../src_prims -I_deps/benchmark-src/src/../include -I_deps/raft-src/cpp/include -I_deps/rmm-src/include -I_deps/thrust-src -I_deps/thrust-src/dependencies/cub -isystem=/usr/local/cuda-11.2/include -isystem=/home/galahad/miniconda3/envs/ns0527/include -O3 -DNDEBUG --generate-code=arch=compute_86,code=[sm_86] --expt-extended-lambda --expt-relaxed-constexpr -Xcompiler=-Wno-deprecated-declarations -Xcompiler -pthread -std=c++17 -MD -MT bench/CMakeFiles/prims_benchmark.dir/prims/add.cu.o -MF bench/CMakeFiles/prims_benchmark.dir/prims/add.cu.o.d -x cu -c ../bench/prims/add.cu -o bench/CMakeFiles/prims_benchmark.dir/prims/add.cu.o
FAILED: bench/CMakeFiles/prims_benchmark.dir/prims/add.cu.o
/usr/local/cuda-11.2/bin/nvcc -forward-unknown-to-host-compiler -DTHRUST_DEVICE_SYSTEM=THRUST_DEVICE_SYSTEM_CUDA -DTHRUST_HOST_SYSTEM=THRUST_HOST_SYSTEM_CPP -I../bench/../src_prims -I_deps/benchmark-src/src/../include -I_deps/raft-src/cpp/include -I_deps/rmm-src/include -I_deps/thrust-src -I_deps/thrust-src/dependencies/cub -isystem=/usr/local/cuda-11.2/include -isystem=/home/galahad/miniconda3/envs/ns0527/include -O3 -DNDEBUG --generate-code=arch=compute_86,code=[sm_86] --expt-extended-lambda --expt-relaxed-constexpr -Xcompiler=-Wno-deprecated-declarations -Xcompiler -pthread -std=c++17 -MD -MT bench/CMakeFiles/prims_benchmark.dir/prims/add.cu.o -MF bench/CMakeFiles/prims_benchmark.dir/prims/add.cu.o.d -x cu -c ../bench/prims/add.cu -o bench/CMakeFiles/prims_benchmark.dir/prims/add.cu.o
In file included from ../bench/prims/add.cu:19:
../bench/prims/../common/ml_benchmark.hpp:22:10: fatal error: cuml/common/logger.hpp: No such file or directory
22 | #include <cuml/common/logger.hpp>Not entirely sure of the cause of it yet, since not much changed in the cmakelists on the bench folder
Co-authored-by: Dante Gama Dessavre <[email protected]>
|
rerun tests |
|
rerun tests |
dantegd
approved these changes
Jun 2, 2021
|
@gpucibot merge |
vimarsh6739
pushed a commit
to vimarsh6739/cuml
that referenced
this pull request
Oct 9, 2023
Resolves two issues found when trying to build `cuml` locally. The first was that the `CUML_C_TARGET` had `FAISS` in the public link requirements, which doesn't look needed. This caused CMake configuration errors when trying to generate the `cuml-config.cmake` file. Solution was to move `FAISS` to be a private dependency ( brought in via `CUML_CPP_TARGET` ) The second was that the `CUML_C_TARGET` tests need to make sure the `C` library is enabled. CUML doesn't enable this language by default as the `CUML_C_TARGET` builds using the `C++` compiler and exposes a `C` API via `extern C` blocks. I didn't go with a blanket enable `C` language as each language enabled, as that would increase the default requirements to build CUML when testing is disabled. Authors: - Robert Maynard (https://github.com/robertmaynard) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) URL: rapidsai#3908
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves two issues found when trying to build
cumllocally.The first was that the
CUML_C_TARGEThadFAISSin the public link requirements, which doesn't look needed. This caused CMake configuration errors when trying to generate thecuml-config.cmakefile. Solution was to moveFAISSto be a private dependency ( brought in viaCUML_CPP_TARGET)The second was that the
CUML_C_TARGETtests need to make sure theClibrary is enabled. CUML doesn't enable this language by default as theCUML_C_TARGETbuilds using theC++compiler and exposes aCAPI viaextern Cblocks.I didn't go with a blanket enable
Clanguage as each language enabled, as that would increase the default requirements to build CUML when testing is disabled.