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

Add sample filtering for ivf_flat. Filtering code refactoring and cleanup #1541

Merged
merged 10 commits into from
Jun 5, 2023
Merged

Add sample filtering for ivf_flat. Filtering code refactoring and cleanup #1541

merged 10 commits into from
Jun 5, 2023

Conversation

alexanderguzhva
Copy link
Contributor

@alexanderguzhva alexanderguzhva commented May 20, 2023 •
edited
Loading

The PR does the following:

  • Introduces ivf_flat::search_with_filtering() call in the same way the filtering was introduced to ivf_pq in Introduce sample filtering to IVFPQ index search #1513
  • Moves sample_filter.cuh from raft/neighbor/detail to raft/neighbor
  • Moves NoneSampleFilter from raft::neighbor::ivf_pq::detail namespace to raft::neighbor::filtering namespace
  • Renames NoneSampleFilter to NoneIvfSampleFilter and template argument SampleFilterT to IvfSampleFilterT
  • Adds a missing resource::get_workspace_resource(handle) in ivf_flat-inl.cuh in a search_with_filtering() call (which was copied from search() call with the same problem)
  • Adds more comments in ivf_pq-inl.h
  • Some code cleanup in ivf_pq-inl.h

@alexanderguzhva alexanderguzhva requested a review from a team as a code owner May 20, 2023 22:16
@rapids-bot
Copy link

rapids-bot bot commented May 20, 2023

Pull requests from external contributors require approval from a rapidsai organization member with write or admin permissions before CI can begin.

@github-actions github-actions bot added the cpp label May 20, 2023
@cjnolet cjnolet mentioned this pull request May 23, 2023
Open
5 tasks
@alexanderguzhva
Copy link
Contributor Author

@achirkin @cjnolet Would it be possible for you folks to run some tests, if you don't mind? :) The reason is that I'm leaving Meta in 1 week and then go to a vacation for several weeks, and we're having a holiday on Monday, so it would be nice if this PR could be merged this week. Please let me know on how I can speed up the process. Thanks.

@cjnolet
Copy link
Member

cjnolet commented May 25, 2023

/ok to test

@cjnolet cjnolet changed the base branch from branch-23.06 to branch-23.08 May 25, 2023 21:02
@cjnolet
Copy link
Member

cjnolet commented May 25, 2023

@alexanderguzhva done. Do you mind merging your changes forward into branch-23.08? We just entered code freeze for release 23.06.

@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 25, 2023
cjnolet
cjnolet reviewed May 25, 2023
View reviewed changes
cpp/include/raft/neighbors/sample_filter.cuh Outdated
@@ -65,7 +65,7 @@ struct NoneSampleFilter {
* };
*
* Initialize it as:
* using filter_type = IndexSampleFilter<idx_t>;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of raft/neighbors/sample_filter.cuh, we use the suffix _types to denote header files that contain important vocabulary bits for interacting w/ any public APIs not in raft/core. Can you rename this file to neighbors/sample_filter_types.hpp? It's also important that any functions in these files be accessible from host (and conditionally accessible from device). Can you change all __host__ __device__ to _RAFT_HOST_DEVICE (from core/detail/macros.hpp)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjnolet sure, 23.08.

@alexanderguzhva alexanderguzhva force-pushed the ivf_flat_search_with_filtering branch from 2cf2624 to 61040c5 Compare May 25, 2023 22:39
cjnolet
cjnolet reviewed May 26, 2023
View reviewed changes
cpp/include/raft/neighbors/detail/ivf_pq_compute_similarity-ext.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/detail/ivf_pq_compute_similarity-inl.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/detail/ivf_pq_search.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/detail/refine.cuh Outdated Show resolved Hide resolved
cpp/src/neighbors/detail/ivf_flat_interleaved_scan_float_float_int64_t.cu Outdated Show resolved Hide resolved
cpp/src/neighbors/detail/ivf_flat_interleaved_scan_int8_t_int32_t_int64_t.cu Outdated Show resolved Hide resolved
cpp/src/neighbors/detail/ivf_flat_interleaved_scan_uint8_t_uint32_t_int64_t.cu Outdated
@@ -15,22 +15,28 @@
*/

#include <raft/neighbors/detail/ivf_flat_interleaved_scan-inl.cuh>
#include <raft/neighbors/sample_filter.cuh>
Copy link
Member

Choose a reason for hiding this comment

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

Rather than marking all of them, I'll just point out here that I think a search and replace might be in order.

Copy link
Contributor Author

@alexanderguzhva alexanderguzhva May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, it seems that my grep did not work as I expected somewhy O_o, I'll fix that.
Also, what about the problem in check-style, bcz it seems to be complaining on /python/raft-dask/pyproject.toml and /python/pylibraft/pyproject.toml?

@cjnolet
Copy link
Member

cjnolet commented May 26, 2023

/ok to test

achirkin
achirkin approved these changes May 30, 2023
View reviewed changes
Copy link
Contributor

@achirkin achirkin left a comment

Choose a reason for hiding this comment

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

Sorry for the late response, LGTM!

cpp/include/raft/neighbors/detail/refine.cuh Outdated
refinement_index.metric(),
1,
k,
raft::distance::is_min_close(metric),
// TODO: add the filtering support
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, we can remove this todo because refine does not need filtering support (it takes already filtered results).

Am I right, @cjnolet?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds right: if the neighbor candidates are already filtered, then we do not need any extra filtering in refine.

@cjnolet
Copy link
Member

cjnolet commented Jun 2, 2023

/ok to test

cjnolet
cjnolet approved these changes Jun 2, 2023
View reviewed changes
Copy link
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.

LGTM. Thanks @alexanderguzhva for this feature!

@cjnolet
Copy link
Member

cjnolet commented Jun 2, 2023

/ok to test

@cjnolet
Copy link
Member

cjnolet commented Jun 5, 2023

/merge

@rapids-bot rapids-bot bot merged commit 9bf7b4b into rapidsai:branch-23.08 Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfeher tfeher tfeher left review comments

@achirkin achirkin achirkin approved these changes

@cjnolet cjnolet cjnolet approved these changes

Assignees

@alexanderguzhva alexanderguzhva

Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
VS/ML/DM Primitives Release Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexanderguzhva @cjnolet @tfeher @achirkin