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

Add a static library for cuvs #382

Merged
merged 7 commits into from
Oct 3, 2024
Merged

Conversation

benfred
Copy link
Member

@benfred benfred commented Oct 3, 2024

For the cuml integration, we need to be able to statically link to cuvs in order build the python wheels. This change adds a static target that lets us do that

For the cuml integration, we need to be able to statically link to cuvs in order
build the python wheels. This change adds a static target that lets us do that
@benfred benfred added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Oct 3, 2024
target_link_libraries(
cuvs_static
PUBLIC rmm::rmm raft::raft ${CUVS_CTK_MATH_DEPENDENCIES}
PRIVATE nvidia::cutlass::cutlass $<TARGET_NAME_IF_EXISTS:OpenMP::OpenMP_CXX>
Copy link
Contributor

@achirkin achirkin Oct 3, 2024

Choose a reason for hiding this comment

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

Shouldn't this target also link cuvs-cagra-search?
(just for recap: recently I added cuvs-cagra-search static target that holds only internally-used symbols for a subset of files, so that I could set specific CMake target flags only for these symbols)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah - I agree this should link cuvs-cagra-search (and tried in the previous commit) . The error I was hitting was :

  CMake Error in CMakeLists.txt:
    export called with target "cuvs_static" which requires target
    "cuvs-cagra-search" that is not in any export set.

https://github.com/rapidsai/cuvs/actions/runs/11155746120/job/31007185860

Will try your suggestion - thanks for this!

Copy link
Member Author

Choose a reason for hiding this comment

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

works great - thanks! applied in latest commit

Copy link
Contributor

@achirkin achirkin Oct 3, 2024

Choose a reason for hiding this comment

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

Do you think we still need the cuvs-cagra-search in the install export set? Theoretically, we shouldn't be exposing that as a library and nobody outside cuVS should need to link against it. I suspect the error you saw previously was indirectly caused by the linker error and thus failure of CMake to register cuvs-cagra-search.

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@benfred benfred marked this pull request as ready for review October 3, 2024 07:15
@benfred benfred requested a review from a team as a code owner October 3, 2024 07:15
@cjnolet
Copy link
Member

cjnolet commented Oct 3, 2024

/merge

@rapids-bot rapids-bot bot merged commit 5629977 into rapidsai:branch-24.10 Oct 3, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants