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

Replace raft_export with rapids_export #1079

Closed
wants to merge 3 commits into from

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Dec 8, 2022

The primary use of raft_export was to support COMPONENTS, which has now been added to rapids_export.

@vyasr vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change 2 - In Progress Currenty a work in progress CMake labels Dec 8, 2022
@vyasr vyasr self-assigned this Dec 8, 2022
@vyasr
Copy link
Contributor Author

vyasr commented Dec 8, 2022

Currently libraft builds with this change aren't 100% compatible with the old pylibraft builds. The issue is that raft_export exports the compiled dependency targets while rapids_export does not. AFAICT the difference comes from the fact that the config file used by raft_export has embedded in it the extra logic for including the corresponding libs i.e. including raft-distance-(targets|dependencies).cmake also includes the corresponding raft-distance-lib file. Is there a way that we can emulate that behavior with rapids_export, or perhaps with additional downstream modifications?

This change causes problems because pylibraft is currently configured to fail to build if it is not being compiled against a C++ raft build that built libraft_distance.so (it will not use raft in header-only mode). The trick is that I don't think we want to export raft_distance_lib or raft_nn_lib as COMPONENTS, but we do want them to be available as GLOBAL_TARGETS for the purpose of installation (or other possible uses).

@vyasr
Copy link
Contributor Author

vyasr commented Dec 8, 2022

CC @robertmaynard

@robertmaynard
Copy link
Contributor

In general we can't switch to rapids_export since it doesn't export COMPONENTS or geneate a <project>-config.cmake that handles components.

We could improve rapids_export to support COMPONENTS but that behavior would match what we wrote in raft_export I expect.

This change causes problems because pylibraft is currently configured to fail to build if it is not being compiled against a C++ raft build that built libraft_distance.so (it will not use raft in header-only mode). The trick is that I don't think we want to export raft_distance_lib or raft_nn_lib as COMPONENTS, but we do want them to be available as GLOBAL_TARGETS for the purpose of installation (or other possible uses).

I don't understand the problem you are running into. We can't construct targets without ensuring the required dependencies they have are fullfilled. So either we have distance and nn as COMPONENTS and they are optionally brought in, or we don't have components and we always look for the dependencies of distance and nn which in short makes them required COMPONENTS and breaks other RAFT consumers which don't want to have the superset of dependencies caused by requiring both distance and nn.

The initial implementation of raft_distance_lib and raft_nn_lib was that they are an optimization technique and seamless to the caller of find_package(RAFT COMPONENTS ...). It wasn't expected that a consumer would error out if the pre-compiled template library didn't exist.

If we want to fail to compile against a header only COMPONENT version of raft we can do so at compile time by requiring the RAFT_DISTANCE_COMPILED or RAFT_NN_COMPILED compiler defines that are already provided. If needed we could add some extra target property to raft::distance and raft::nn to make this kind of query easier on the CMake side ( you currently can check INTERFACE_COMPILE_DEFINITIONS)

@vyasr
Copy link
Contributor Author

vyasr commented Jan 6, 2023

Somehow I had thought we already merged rapidsai/rapids-cmake#154. OK I'm going close this PR for now, can reopen if and when that feature is merged and sufficient for raft's needs here.

Regarding the compiled libs, yes, I realize that the behavior of the Python lib is not really what the C++ CMake was designed for. Perhaps we don't really need to make it that strict and it would be fine to allow the headers, but just ensure that the libraries are built in practice. I can consider removing that check (regardless of what we do with the other changes proposed in this PR).

@vyasr vyasr closed this Jan 6, 2023
@vyasr vyasr deleted the refactor/rapids_export branch March 30, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currenty a work in progress CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
Development

Successfully merging this pull request may close these issues.

2 participants