Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for iterating over batches in bfknn #1947
Add support for iterating over batches in bfknn #1947
Changes from 8 commits
aebe8a4
a947970
ad27879
d4394c1
d8ec324
e401a6d
0f4b2a7
5a859bc
eef2c4a
a40edc6
7ab2e16
126a033
13afb44
62a438f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please define the interface in a file called
raft/neighbors/neighbors_types.hpp
(for generalized / non-brute-force types) and/orraft/neighbors/brute_force_types.hpp
, define the implementation inraft::neighbors::detail
namespace, and then create a stateless factory function likeraft::neighbors::brute_force::search_batch_k()
to create an instance of it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need copy/paste usage examples for all public API functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've structured RAFT's computational APIs as stateless functions that operate on objects which hold trivial state. I'm not totally against having an iterator that knows how to load the next batch, but to be consistent with other APIs, we should have a stateless public API function that creates an iterator instance and then arm the iterator implementation with whatever state it's needed to iterate from beginning to end.
This way, the user can call something like:
Then the batched iterator itself is a mere implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in the last commit - the
batch_k_query
is now an abstract class containing no cuda code, and there is adetail::gpu_batch_k_query
class with the cuda definitions that is created via amake_batch_k_query
factory functionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this conditional in a few places in the code- perhaps we should consolidate them into a function in
detail
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
batch
class could be re-used for batching IVF and CAGRA searches. Can it be implemented in a separate file?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the batch size changes as we iterate through k?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The batch sizes can change as we iterate through K - if we want to support the redis
VecSimBatchIterator
interface, we will need this to handle thegetNextResults
method https://github.com/RedisAI/VectorSimilarity/blob/22954489d9184c9eba55f477463439a3532ca04e/src/VecSim/batch_iterator.h#L40-L42