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

Moving random ball cover #218

Open
wants to merge 12 commits into
base: branch-24.10
Choose a base branch
from
Open

Moving random ball cover #218

wants to merge 12 commits into from

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Jul 11, 2024

No description provided.

@cjnolet cjnolet added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jul 11, 2024
@cjnolet cjnolet self-assigned this Jul 11, 2024
@cjnolet cjnolet requested review from a team as code owners July 11, 2024 19:54
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.37%. Comparing base (49e45bb) to head (30aedf6).
Report is 1 commits behind head on branch-24.10.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-24.10     #218   +/-   ##
=============================================
  Coverage         70.37%   70.37%           
=============================================
  Files                12       12           
  Lines                54       54           
=============================================
  Hits                 38       38           
  Misses               16       16           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

notebooks/rmm_log.txt Outdated Show resolved Hide resolved
cpp/src/neighbors/faiss_select/Comparators.cuh Outdated Show resolved Hide resolved
cpp/src/neighbors/faiss_select/DistanceUtils.h Outdated Show resolved Hide resolved
@@ -0,0 +1,205 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: since we're no longer header only - I'd prefer to get rid of having -ext.cuh and -inl.cuh header files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean just consolidate them into one file or do you mean remove the intantiations / ignored extern templates?

cpp/include/cuvs/neighbors/ball_cover.hpp Outdated Show resolved Hide resolved
cpp/include/cuvs/neighbors/ball_cover.hpp Outdated Show resolved Hide resolved
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@cjnolet
Copy link
Member Author

cjnolet commented Jul 30, 2024

Linking #110

@cjnolet cjnolet changed the base branch from branch-24.08 to branch-24.10 August 26, 2024 21:53
@cjnolet
Copy link
Member Author

cjnolet commented Aug 27, 2024

/ok to test

@github-actions github-actions bot removed the ci label Aug 27, 2024
@rapidsai rapidsai deleted a comment from copy-pr-bot bot Aug 27, 2024
@cjnolet
Copy link
Member Author

cjnolet commented Aug 28, 2024

/ok to test

@cjnolet
Copy link
Member Author

cjnolet commented Aug 29, 2024

Metrics are showing this change adds ~78mb to the overall binary size. This is not acceptable.

Screenshot from 2024-08-29 12-45-14

rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Oct 4, 2024
This PR updates to use cuVS instead of RAFT for vector search, pairwise distances and clustering. This is required for us to deprecate the vector search functionality in RAFT, in favour of the code in cuVS.

Because some code hasn't been migrated over to cuvs yet, we will continue to use the version in RAFT - but with RAFT in header only mode. In particular this functionality will be used in RAFT header only mode:

* Random Ball Cover (see rapidsai/cuvs#218)
* Sparse KNN
* nn-descent rapidsai/cuvs#364
* [MetricProcessor](c7d1b0e)
* knn_merge_parts
* build_dendrogram_host
* build_sorted_mst
*  raft DistanceType

Because sparse KNN in RAFT uses the DistanceType in RAFT, we can't fully move over to use the DistanceType code in cuVS with this PR. (Also the DistanceType code in RAFT has a `Precomputed` option that isn't available in cuvs - but is needed by cuml for dbscan.)  This means that we have both the raft and cuvs DistanceType enum's in use with this change, with conversions between them.

Authors:
  - Ben Frederickson (https://github.com/benfred)
  - Bradley Dice (https://github.com/bdice)
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)
  - Corey J. Nolet (https://github.com/cjnolet)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Dante Gama Dessavre (https://github.com/dantegd)
  - Bradley Dice (https://github.com/bdice)

URL: #6085
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 Python
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants