Use rapids-cmake new COMPONENT exporting feature #1154
Conversation
robertmaynard
added
feature request
robertmaynard
force-pushed
the
fea/use_rapids_export_components
branch
from
a013790 to
2a9862e
Compare
robertmaynard
force-pushed
the
fea/use_rapids_export_components
branch
from
613d086 to
36fa38a
Compare
Codecov ReportBase: 87.99% // Head: 87.99% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## branch-23.02 #1154 +/- ##
=============================================
Coverage 87.99% 87.99%
=============================================
Files 21 21
Lines 483 483
=============================================
Hits 425 425
Misses 58 58 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
robertmaynard
force-pushed
the
fea/use_rapids_export_components
branch
from
36fa38a to
f87323c
Compare
Still a draft as the rapids-cmake feature hasn't been merged yet. I expect we are going to have to slip this to 23.04 given that rapids-cmake is in burndown |
|
@robertmaynard just pinging to see how you feel about this PR. Do you think this is going to make it for 23.04 or should we push? |
Aiming to have the rapids-cmake changes merged tomorrow |
robertmaynard
force-pushed
the
fea/use_rapids_export_components
branch
2 times, most recently
from
257e910 to
84a8935
Compare
robertmaynard
force-pushed
the
fea/use_rapids_export_components
branch
from
84a8935 to
d1115e1
Compare
| set(raft_export_sets raft-compiled-exports raft-distributed-exports) | ||
| if(TARGET raft_lib) | ||
| list(APPEND raft_components compiled) | ||
| list(APPEND raft_export_sets raft-compiled-lib-exports) |
There was a problem hiding this comment.
At this point I'm a bit confused by the different targets and export sets that we have in raft. Why do we need both the raft_compiled and raft_lib targets? It used to be necessary for the nn/dist targets so that calling code could use either component independently as header-only or precompiled, but now why can't the raft target just be the header-only target and raft_compiled always be precompiled? And that stems from my other question, why do we need both raft-compiled-exports and raft-compiled-lib-exports?
There was a problem hiding this comment.
@vyasr from my understanding at this point, the answer is "we don't need both raft_compiled and raft_lib anymore, but at the current time they aren't really causing harm, so we aren't in an immediate rush to remove them". @robertmaynard please correct me if that assertion is wrong.
There was a problem hiding this comment.
The raft_compiled target is created even when RAFT_COMPILE_LIBRARY is turned off, so one solution for proper invariant behavior when generating the libraft-headers and libraft was to have the current organization.
If we are okay with making the raft::compiled target conditional all this logic can be removed and we could have a single target raft_compiled and export set ( raft-compiled-exports ). If we are good with this, I would be happy to open a PR that does these changes.
|
/merge |
Switches raft over to the upcoming raft_export(
COMPONET) support.This allows raft to not require to support custom exporting logic