[FEA] Add bitset for ANN pre-filtering and deletion #1803

lowener · 2023-09-04T13:19:36Z

Related to #1600

wphicks

Love to see this functionality go in! This is something that we currently use in FIL, and I'd love to replace it with a RAFT version.

My general pieces of feedback would be:

I'd like to make sure that we have a really good understanding of performance and how close to SoL this is. At minimum, adding benchmarks seems like it would be very worthwhile to allow for rapid perf iteration even after the initial PR.
To the extent possible (and acknowledging the slight differences in purpose), matching the std::bitset API would make for a really nice developer experience. Little things like adding a .flip method would save downstream users a lot of time.
Generalizing this as much as possible to different memory locations would also help with a lot of use cases that I can envision.

If we are to use this in FIL, we would also need to be able to compile with or without nvcc, but it is fine if we want to leave that off the table for now.

cpp/include/raft/util/bitset.cuh

wphicks · 2023-09-06T20:06:28Z

cpp/include/raft/util/bitset.cuh

+   *
+   * @param bitset_span Device vector view of the bitset
+   */
+  _RAFT_HOST_DEVICE bitset_view(raft::device_vector_view<BitsetT, IdxT> bitset_span)


It seems like most of the logic is host-device compatible. Any reason why we restrict ourselves to an on-device bitset? I ask in particular because I can imagine several common workflows where we construct the bitset on host and then apply it on device by copying the underlying memory. As a matter of fact, this is exactly what we do in FIL.

Separately, it seems like a span, rather than an mdspan, is sufficient for what we're using it for. If we're going to use an mdspan anyway, is there a specific reason to restrict ourselves to 1D? If not, is there some other reason not to just accept a span?

cpp/include/raft/util/bitset.cuh

wphicks · 2023-09-06T20:34:48Z

cpp/include/raft/util/bitset.cuh

+        res, raft::ceildiv(bitset_len, bitset_element_size))}
+  {
+    RAFT_EXPECTS(mask_index.extent(0) <= bitset_len, "Mask index cannot be larger than bitset len");
+    cudaMemsetAsync(bitset_.data_handle(),


Would it be more efficient to construct the data on host and then just transfer it all at once, rather than setting and unsetting the data like this? It would come at the cost of host memory footprint, but I suspect that it would be worth it for large bitsets.

cpp/include/raft/util/bitset.cuh

cpp/test/util/bitset.cu

wphicks · 2023-09-19T14:46:35Z

Just to follow up on my previous review, I think all of the code details are in a good place, and we can follow up on any perf questions efficiently using the new benchmarks. My only remaining concern before merging an initial version of this is that it would be nice to make sure that the API does not change significantly in later follow-ups. To that end, it would be really great if we could match the std::bitset API as much as possible, with any deviations justified by the slight difference in purpose or the need for device-side support. Having e.g. flip, set, reset, etc. methods available on the object itself would make for a very easy transition for folks who are already familiar with std::bitset.

lowener · 2023-09-19T17:51:25Z

The reasoning behind this was to have a stateless API as indicated in the dev guide.
Should I do an exception to match std::bitset API?

cjnolet · 2023-09-20T09:45:02Z

The reasoning behind this was to have a stateless API as indicated in the dev guide.
Should I do an exception to match std::bitset API?

@lowener sorry to do this last minute, but I think stateful is okay for these types of reusable objects. We want statelessness across the APIs for compute functions but raft::bitset is becoming a pretty core API (like mdspan or mdarray, for example) and I think maintaining state in the bitset is okay. I also think we should move the bitset into raft/core. That was my fault- I thought initially it would be a couple utility functions but now it's evolved into a first-class construct, which is great.

I agree with @wphicks that we should match existing standards as much as possible. The stl are generally very well thought out by large teams of c++ veterans and by following their interfaces, at the very least, we give our users compatbility and familiarity.

cjnolet

I think this looks good for a first pass and given the circumstances, I'm going to approve it for now. We can continue to revisit and iterate on it as it gets integrated more into the ANN filtering features.

cjnolet · 2023-09-26T16:14:18Z

docs/source/cpp_api/core.rst

+   core_math.rst
+   core_bitset.rst


As we continue to build out these filtering primitives, I think we need to consider consolidating them under a single page in the documentation, kind of like what we have done for the math primitives.

I don't think we need to do that right now, though. But something to think about- maybe something like "fast lookup primitives" or something.

divyegala

Are device functions for test/set/flip going to be follow-up work?

divyegala · 2023-09-26T16:15:27Z

cpp/include/raft/core/bitset.cuh

+ */
+template <typename bitset_t = uint32_t, typename index_t = uint32_t>
+struct bitset_view {
+  index_t static constexpr const bitset_element_size = sizeof(bitset_t) * 8;


const is redundant

Suggested change

index_t static constexpr const bitset_element_size = sizeof(bitset_t) * 8;

index_t static constexpr bitset_element_size = sizeof(bitset_t) * 8;

divyegala · 2023-09-26T17:09:55Z

cpp/include/raft/core/bitset.cuh

+  inline _RAFT_HOST_DEVICE auto data_handle() -> bitset_t* { return bitset_ptr_; }
+  inline _RAFT_HOST_DEVICE auto data_handle() const -> const bitset_t* { return bitset_ptr_; }


Suggested change

inline _RAFT_HOST_DEVICE auto data_handle() -> bitset_t* { return bitset_ptr_; }

inline _RAFT_HOST_DEVICE auto data_handle() const -> const bitset_t* { return bitset_ptr_; }

inline _RAFT_HOST_DEVICE auto data() -> bitset_t* { return bitset_ptr_; }

inline _RAFT_HOST_DEVICE auto data() const -> const bitset_t* { return bitset_ptr_; }

divyegala · 2023-09-26T17:11:10Z

cpp/include/raft/core/bitset.cuh

+  inline auto to_mdspan() -> raft::device_vector_view<bitset_t, index_t>
+  {
+    return raft::make_device_vector_view<bitset_t, index_t>(bitset_ptr_, n_elements());
+  }
+  inline auto to_mdspan() const -> raft::device_vector_view<const bitset_t, index_t>
+  {
+    return raft::make_device_vector_view<const bitset_t, index_t>(bitset_ptr_, n_elements());
+  }


Can we also add two operators? inline auto operator raft::device_vector_view..., this will make the conversion automatic

divyegala · 2023-09-26T17:12:13Z

cpp/bench/prims/core/bitset.cu

+RAFT_BENCH_REGISTER(Uint8_32, "", bitset_input_vecs);
+RAFT_BENCH_REGISTER(Uint16_64, "", bitset_input_vecs);
+RAFT_BENCH_REGISTER(Uint32_32, "", bitset_input_vecs);
+RAFT_BENCH_REGISTER(Uint32_64, "", bitset_input_vecs);


Can you please post the benchmark results as a comment in your PR?

divyegala · 2023-09-26T17:12:44Z

cpp/include/raft/core/bitset.cuh

+ */
+template <typename bitset_t = uint32_t, typename index_t = uint32_t>
+struct bitset {
+  index_t static constexpr const bitset_element_size = sizeof(bitset_t) * 8;


Suggested change

index_t static constexpr const bitset_element_size = sizeof(bitset_t) * 8;

index_t static constexpr bitset_element_size = sizeof(bitset_t) * 8;

divyegala · 2023-09-26T17:17:13Z

cpp/include/raft/core/bitset.cuh

+      bitset_len_{bitset_len},
+      default_value_{default_value}
+  {
+    cudaMemsetAsync(bitset_.data(),


Please don't use cuda rt API directly

divyegala · 2023-09-26T17:18:46Z

cpp/include/raft/core/bitset.cuh

+  bitset(const raft::resources& res,
+         raft::device_vector_view<const index_t, index_t> mask_index,
+         index_t bitset_len,
+         bool default_value = true)


Can we use an enum here instead of boolean? Something like default_value::ALL_ONES and default_value::ALL_ZEROS

divyegala · 2023-09-26T17:20:29Z

cpp/include/raft/core/bitset.cuh

+   */
+  inline auto n_elements() const -> index_t
+  {
+    return raft::ceildiv(bitset_len_, bitset_element_size);


Suggested change

return raft::ceildiv(bitset_len_, bitset_element_size);

return bitset_.size();

divyegala · 2023-09-26T17:23:19Z

cpp/include/raft/core/bitset.cuh

+   * @param bitset_len Length of the bitset
+   * @param default_value Default value to set the bits to. Default is true.
+   */
+  bitset(const raft::resources& res,


Why not store this as a member so that none of the other functions need to accept raft::resources on their API?

divyegala · 2023-09-26T17:23:32Z

cpp/include/raft/core/bitset.cuh

+    bitset_len_ = new_bitset_len;
+    if (old_size < new_size) {
+      // If the new size is larger, set the new bits to the default value
+      cudaMemsetAsync(bitset_.data() + old_size,


Use RAFT API

divyegala · 2023-09-26T20:19:18Z

/merge

Closes #1600. To be merged after #1803 and #1811 This PR adds `bitset_filter` to filter an index with a bitset. Authors: - Micka (https://github.com/lowener) - tsuki (https://github.com/enp1s0) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet)

PR based on the new Bitset feature (#1803) to support vector deletion in ANN. Closes #1177. Closes #1620. This PR adds `ivf_to_sample_filter` that acts as an intermediate filter to use an IVF index with a bitset filter. Authors: - Micka (https://github.com/lowener) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - William Hicks (https://github.com/wphicks) URL: #1831

PR based on the new Bitset feature (rapidsai#1803) to support vector deletion in ANN. Closes rapidsai#1177. Closes rapidsai#1620. This PR adds `ivf_to_sample_filter` that acts as an intermediate filter to use an IVF index with a bitset filter. Authors: - Micka (https://github.com/lowener) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - William Hicks (https://github.com/wphicks) URL: rapidsai#1831

Add bitset

25a85e0

github-actions bot added cpp CMake labels Sep 4, 2023

lowener added 3 commits September 5, 2023 01:22

Update naming and use raft::linalg::map

dbabbd2

Fix shared mem allocation

4906c3a

Fix copyright

463d7f5

lowener added 3 - Ready for Review non-breaking Non-breaking change feature request New feature or request labels Sep 5, 2023

lowener marked this pull request as ready for review September 5, 2023 00:49

lowener requested review from a team as code owners September 5, 2023 00:49

Add documentation

5227a95

cjnolet assigned lowener Sep 5, 2023

lowener added 2 commits September 5, 2023 23:56

Merge branch 'branch-23.10' into 23.10-bitset-fea

07671aa

Simplify code and remove shared mem kernel

75b0f82

wphicks reviewed Sep 6, 2023

View reviewed changes

lowener added 7 commits September 11, 2023 15:59

Enable bitset type template and add flip function

f834b18

Add bench for bitset

d74bbb2

Merge branch 'branch-23.10' into 23.10-bitset-fea

f9c544e

Fix namespace typo

d075184

Use raft device_uvector in bitset

1f54720

Fix build.sh

e0c1d24

Add n_elements fix size

8d30c9f

lowener mentioned this pull request Sep 18, 2023

[FEA] Support vector deletion in ANN IVF #1831

Merged

lowener added 2 commits September 18, 2023 22:21

Fix naming

480d20e

Fix naming of inputs

240af3e

Fix bitset_len in bitset_view

08c43d0

lowener added 2 commits September 20, 2023 16:31

Move bitset to core and match std::bitset

9115bbb

Merge branch 'branch-23.10' into 23.10-bitset-fea

121ef46

lowener mentioned this pull request Sep 22, 2023

[FEA] Add bitset_filter for CAGRA indices removal #1837

Merged

Merge branch 'branch-23.10' into 23.10-bitset-fea

541e239

cjnolet approved these changes Sep 26, 2023

View reviewed changes

Merge branch 'branch-23.10' into 23.10-bitset-fea

b9de2e3

divyegala reviewed Sep 26, 2023

View reviewed changes

rapids-bot bot merged commit b9b5f44 into rapidsai:branch-23.10 Sep 26, 2023
54 checks passed

lowener mentioned this pull request Oct 2, 2023

[FEA] Follow-up work on Bitset #1866

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEA] Add bitset for ANN pre-filtering and deletion #1803

[FEA] Add bitset for ANN pre-filtering and deletion #1803

lowener commented Sep 4, 2023

wphicks left a comment

wphicks Sep 6, 2023

wphicks Sep 6, 2023

wphicks commented Sep 19, 2023

lowener commented Sep 19, 2023

cjnolet commented Sep 20, 2023

cjnolet left a comment

cjnolet Sep 26, 2023

divyegala left a comment

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala Sep 26, 2023

divyegala commented Sep 26, 2023

	index_t static constexpr const bitset_element_size = sizeof(bitset_t) * 8;
	index_t static constexpr bitset_element_size = sizeof(bitset_t) * 8;

		inline _RAFT_HOST_DEVICE auto data_handle() -> bitset_t* { return bitset_ptr_; }
		inline _RAFT_HOST_DEVICE auto data_handle() const -> const bitset_t* { return bitset_ptr_; }

	return raft::ceildiv(bitset_len_, bitset_element_size);
	return bitset_.size();

[FEA] Add bitset for ANN pre-filtering and deletion #1803

[FEA] Add bitset for ANN pre-filtering and deletion #1803

Conversation

lowener commented Sep 4, 2023

wphicks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wphicks commented Sep 19, 2023

lowener commented Sep 19, 2023

cjnolet commented Sep 20, 2023

cjnolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyegala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyegala commented Sep 26, 2023