Expose raft::handle_t in the public header #1192
Conversation
vyasr
added
bug
|
Both cuML and cuGraph CI is blocked on this change, as can be seen in some of the failures on rapidsai/cuml#5165 (see this build log) and rapidsai/cugraph#3189 (see this build log). |
|
OK, it looks like both the cuML and cuGraph failures are actually due to slightly different problems. cuML was relying on transitive includes in raft that were no longer applicable after the change, while cuGraph has an embedded copy of some raft Cython code that needed updating for the changes to raft. The change in this PR still seems helpful and necessary for other users of raft, but it no longer appears to be a blocker for the other RAPIDS libraries. |
|
@vyasr I do agree that this is a super useful change and I certainly don't mind merging it. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## branch-23.04 #1192 +/- ##
===============================================
Coverage ? 87.99%
===============================================
Files ? 21
Lines ? 483
Branches ? 0
===============================================
Hits ? 425
Misses ? 58
Partials ? 0 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. |
|
/merge |
#1140 migrated raft to using
device_resourcesinstead ofhandle_teverywhere internally, but it was not intended to be a breaking change sincehandle_tinherits fromdevice_resources. However,include/handle.hppwas changed to includedevice_resources.hppinstead ofcore/handle.hpp, which is a breaking change since it means that downstream projects now need to includecore/handle.hppdirectly if they want access tohandle_t. While that change may be desired eventually, I do not think that enforcing that requirement was an intended consequence of the change right now.