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

Revert recent fused l2 nn instantiations #899

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Oct 6, 2022

These instantiations have caused some issues because they expose the cub::KeyValuePair symbol compiled into libraft-distances downstream. Unfortunately, since rapids-cmake transforms the name of the cub symbols to include the list of cuda architectures upon which they are built, any users who link against libraft-distance and don't build their code downstream with that exact list of architectures will get an undefined symbol error.

I had initially removed the symbol from the instantiation by copying the cub::KeyValuePair into RAFT and renaming it to raft::KeyValuePair but it seems this is causing problems for HDBSCAN in the centos7 builds within cuml. cc @Nyrio. I'm reverting these changes for 22.10 to give us an opportunity to fix them in 22.12. While this will have a negative impact on compile times, it will at least allow our tests to pass in cuml.

@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 6, 2022
@cjnolet cjnolet requested review from a team as code owners October 6, 2022 10:40
@Nyrio
Copy link
Contributor

Nyrio commented Oct 6, 2022

I'm curious what was causing the said problems with HDBSCAN in cuML? Using a custom pair type instead of cub::KeyValuePair seems to be the best solution here.

@cjnolet
Copy link
Member Author

cjnolet commented Oct 6, 2022

@Nyrio I agree completely, but suddenly an hdbscan test started failing in the centos build after this change (seeming intermittently but still happening 3/4 if the time) and we are in a crunch for code freeze, which technically started already. I hope backing this out is enough to fix the failing test and then we can investigate further in 22.12.

@cjnolet
Copy link
Member Author

cjnolet commented Oct 6, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 11e00f7 into rapidsai:branch-22.10 Oct 6, 2022
cjnolet added a commit to cjnolet/raft that referenced this pull request Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants