Skip to content

[FEA] expose python & C API for prefiltered brute force#174

Merged
rapids-bot[bot] merged 25 commits into
rapidsai:branch-24.08from
rhdong:rhdong/pbf-python
Jul 29, 2024
Merged

[FEA] expose python & C API for prefiltered brute force#174
rapids-bot[bot] merged 25 commits into
rapidsai:branch-24.08from
rhdong:rhdong/pbf-python

Conversation

@rhdong
Copy link
Copy Markdown
Member

@rhdong rhdong commented Jun 5, 2024

No description provided.

@rhdong rhdong requested review from benfred and cjnolet June 5, 2024 00:05
@rhdong rhdong requested review from a team as code owners June 5, 2024 00:05
@rhdong rhdong added feature request New feature or request non-breaking Introduces a non-breaking change C and removed cpp CMake labels Jun 5, 2024
Comment thread python/cuvs/cuvs/neighbors/brute_force/brute_force.pyx Outdated
Copy link
Copy Markdown
Contributor

@benfred benfred left a comment

Choose a reason for hiding this comment

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

this looks great! thanks for getting this in.

some minor comments / feedback below:

Comment thread python/cuvs/cuvs/neighbors/brute_force/brute_force.pyx Outdated
Comment thread python/cuvs/cuvs/neighbors/brute_force/brute_force.pxd Outdated
Comment thread python/cuvs/cuvs/neighbors/prefilters/prefilters.pyx Outdated
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Jun 6, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@rhdong rhdong requested a review from benfred June 6, 2024 01:49
@rhdong
Copy link
Copy Markdown
Member Author

rhdong commented Jun 6, 2024

/ok to test

@rhdong rhdong force-pushed the rhdong/pbf-python branch from eb49a91 to 5ce3b8e Compare June 6, 2024 06:35
@rhdong rhdong force-pushed the rhdong/pbf-python branch from 5ce3b8e to 3a50b90 Compare June 6, 2024 06:36
@benfred benfred requested a review from a team as a code owner June 6, 2024 19:18
@rhdong rhdong requested a review from benfred June 20, 2024 05:37
Copy link
Copy Markdown
Contributor

@benfred benfred left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for this,

@benfred
Copy link
Copy Markdown
Contributor

benfred commented Jun 25, 2024

/ok to test

@rhdong
Copy link
Copy Markdown
Member Author

rhdong commented Jun 26, 2024

/ok to test

@rhdong
Copy link
Copy Markdown
Member Author

rhdong commented Jun 26, 2024

/ok to test

@rhdong rhdong requested a review from benfred June 26, 2024 22:32
Copy link
Copy Markdown
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

I think this API is coming along great! Moslty minor things at this point, but I think this should be ready very soon.

Comment thread cpp/include/cuvs/neighbors/prefilters.h
*/
enum cuvsPrefilterType {
/* No filter */
NO_FILTER,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We will need a way to specify a custom filter function (maybe to accept a function pointer?) also. While bfknn currently only supports bitmap / bitset, we also need to expose the filtering APIs for the other ANN indexes (most ideally through the same API).

I'm okay pushing this to a future change, since it's going to be needed ASAP so that we can expose the remaining filtering functions for the other algorithm. Can you create an issue for this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, I will!

Comment thread cpp/test/neighbors/brute_force_c.cu Outdated
Comment thread python/cuvs/cuvs/neighbors/prefilters/__init__.py Outdated
Comment thread python/cuvs/cuvs/neighbors/prefilters/prefilters.pxd Outdated
Comment thread python/cuvs/cuvs/neighbors/prefilters/prefilters.pyx
@rhdong
Copy link
Copy Markdown
Member Author

rhdong commented Jul 2, 2024

/ok to test

@rhdong rhdong requested a review from cjnolet July 2, 2024 02:27
};

template <typename T, typename IdxT, typename bitmap_t = uint32_t>
void recall_eval_with_filter(T* query_data,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please make sure these are testing all possible combinations of the filter struct/type? Eg NULL, NO_FILTER, BITMAP, and BITSET?

We should set a good precedent for testing these early, so that all follow-on filtering implementations can do the same thing.

Copy link
Copy Markdown
Member Author

@rhdong rhdong Jul 2, 2024

Choose a reason for hiding this comment

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

Yeah, that makes sense. I just checked the code, the NO_FILTER, BITMAP have been covered(pls refer to https://github.com/rapidsai/cuvs/pull/174/files#diff-1b162130023c13d62c06d91f3aab663d943091dddd8a38813c27f65e7d6c61dfR93), and the BITSET is reserved for other algorithms to compatible which is not be implemented in brute-force. Would you like me to implement BITSET in brute-force and test it?

@rhdong rhdong requested a review from cjnolet July 4, 2024 21:36
Copy link
Copy Markdown
Contributor

@benfred benfred left a comment

Choose a reason for hiding this comment

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

lgtm!

@benfred
Copy link
Copy Markdown
Contributor

benfred commented Jul 17, 2024

/ok to test

@benfred
Copy link
Copy Markdown
Contributor

benfred commented Jul 22, 2024

/ok to test

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Jul 29, 2024

/merge

@rapids-bot rapids-bot Bot merged commit 2517826 into rapidsai:branch-24.08 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C CMake cpp feature request New feature or request non-breaking Introduces a non-breaking change Python

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants