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.

Sign up for GitHub

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

Jump to bottom

ivf_flat::index: hide implementation details #747

Merged
merged 147 commits into from
Aug 24, 2022
Merged

ivf_flat::index: hide implementation details #747

merged 147 commits into from
Aug 24, 2022

Conversation

achirkin
Copy link
Contributor

Hide the mutable mdarray members of ivf_flat::index behind immutable mdspan views.

achirkin and others added 30 commits May 13, 2022 10:51
@achirkin
inital commit and formatting cleanup
35ab60d
@achirkin
update save/load index function to work with cuann benchmark suite, s…
24e8c4d 
…eparate out cuivfl handle init in separate func
@achirkin
Added benchmarks.
cb8bcd2
@achirkin
Merge branch 'branch-22.06' into fea-knn-ivf-flat
884723c
@achirkin
Add a missing parameter docs
8c4a0a0
@achirkin
Adapt to the changes in the warpsort api
070fd05
@achirkin
cleanup: use WarpSize constant
83b6630
@achirkin
cleanup: remove unnecessary helpers
3a2703c
@achirkin
Use a more efficient warp_sort_filtered
31bbaec
@achirkin
Recover files that have only non-relevant changes to reduce the size …
4b40181 
…of the PR
@achirkin
wip: replacing explicit allocations with rmm buffers
7e3041c
@achirkin
Merge branch 'branch-22.06' into fea-knn-ivf-flat
f6556b7
@achirkin
Merge branch 'branch-22.06' into fea-knn-ivf-flat
f75761f
@achirkin @tfeher
Update cpp/include/raft/spatial/knn/detail/ann_quantized_faiss.cuh
dd558b4 
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
@achirkin @tfeher
Update cpp/include/raft/spatial/knn/detail/ann_quantized_faiss.cuh
94b3cbe 
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
@achirkin
wip: replace cudaMemcpy with raft::copy
2be45a9
@achirkin
Simplified some cudaMemcpy invocations
30c32a9
@achirkin
Merge branch 'branch-22.06' into fea-knn-ivf-flat
c8e7b4d
@achirkin
Refactoring with helper functions
150a438
@achirkin
Make the scratch buf 3x L2 cache size
ddfb8cc
@achirkin
Remove serialization code for now
b788e2e
@achirkin
remove obsolete comment
3e1c14d
@achirkin
Add a missing sync
a001999
@achirkin
Rename ann_quantized_faiss
2d08271
@achirkin
wip from manual allocations to rmm: updated some parts with pointer r…
0f88aaa 
…equirements CPU/GPU
@achirkin
wip from manual allocations to rmm
363dfc9
@achirkin
fix style
e5399f8
@achirkin
Set minimum memory pool size in radix_topk to 256 bytes
306f5bf
@achirkin
wip malloc-to-rmm: removed most of the manual allocations
fd7d2ba
@achirkin
misc cleanup
403667a
@github-actions github-actions bot added the cpp label Jul 22, 2022
tfeher
tfeher requested changes Jul 25, 2022
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @achirkin for these updates, I have a few small comments, otherwise in general it looks good to me.

cpp/include/raft/spatial/knn/ivf_flat.cuh Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_utils.cuh Outdated Show resolved Hide resolved
cpp/include/raft/spatial/knn/detail/ann_kmeans_balanced.cuh Outdated Show resolved Hide resolved
@tfeher tfeher mentioned this pull request Jul 25, 2022
Closed
cjnolet
cjnolet reviewed Jul 25, 2022
View reviewed changes
cpp/include/raft/spatial/knn/ivf_flat_types.hpp
}

/** Inverted list indices: ids of items in the source data [size] */
[[nodiscard]] inline auto indices() const noexcept
Copy link
Member

Choose a reason for hiding this comment

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

This looks really nice and clean and it designates the ivf_flat::index as the owning object without having to deal w/ the complexities and maintenance of transferring that ownership. I like this a lot.

@achirkin achirkin self-assigned this Jul 28, 2022
@github-actions github-actions bot removed the CMake label Jul 28, 2022
@achirkin achirkin marked this pull request as ready for review July 28, 2022 08:05
@achirkin
Copy link
Contributor Author

achirkin commented Jul 28, 2022 •
edited

This PR is ready for review, but should only be merged after #764 due to a bug in mdarrays (which causes cuml tests to segfault).

tfeher
tfeher approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks Artem for addressing the issues! The PR looks good to me.

@achirkin achirkin changed the base branch from branch-22.08 to branch-22.10 August 9, 2022 07:30
@cjnolet
Copy link
Member

cjnolet commented Aug 24, 2022

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Aug 24, 2022

@achirkin I notice this PR is breaking but from the looks of the changes, it's only breaking for the new IVF-flat index API, right? If that's the case, are there any active consumers of this API that you know of which would break if we merged this?

@achirkin
Copy link
Contributor Author

Yes, I marked it breaking because of the changes in the bew ivf-flat api, and I am not aware of anybody using it yet.

@cjnolet
Copy link
Member

cjnolet commented Aug 24, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit ab9a695 into rapidsai:branch-22.10 Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet approved these changes

@tfeher tfeher tfeher approved these changes

Assignees

@achirkin achirkin

Labels
3 - Ready for Review breaking Breaking change cpp improvement Improvement / enhancement to an existing function
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@achirkin @cjnolet @tfeher