[FEA] support of prefiltered brute force by rhdong · Pull Request #146 · NVIDIA/cuvs

rhdong · 2024-05-22T20:45:40Z

The PR is one part of prefiltered brute force and should work with the PR of raft: [FEA] support of prefiltered brute force raft#2294

rhdong · 2024-05-22T20:55:22Z

+#include <cuvs/core/bitmap.hpp>
 #include <cuvs/neighbors/brute_force.hpp>

+#include <raft/core/bitset.cuh>


Hi @cjnolet @benfred @lowener , I can't remove this inclusion, because it will cause a compilation issue of n_elements of bitset reference but redefinition. I appreciate if you have any idea better to process. Thank you guys!

The include of the bitmap.cuh should be where n_elements is called, in cpp/src/neighbors/detail/knn_brute_force.cuh.

This reverts commit 9fbca1a.

lowener · 2024-05-23T00:15:14Z

+#include <cuvs/core/bitmap.hpp>
 #include <cuvs/neighbors/brute_force.hpp>

+#include <raft/core/bitset.cuh>


The include of the bitmap.cuh should be where n_elements is called, in cpp/src/neighbors/detail/knn_brute_force.cuh.

…tered-bf

cjnolet · 2024-05-24T21:21:06Z

 #include <raft/util/cudart_utils.hpp>   // get_device_for_address
 #include <raft/util/integer_utils.hpp>  // rounding up

+#include <cuvs/core/bitmap.hpp>


The bitset header is included here because the bitset struct prototype is being defined here. I don't see anything being defined for the bitmap here, though.

Mainly for reducing the number of the include files in the cuvs/neighbors/brute_force.hpp and other files need cuvs::core::bitmap_view

We should be including what we use and not relying on transitivity here. Ultimately, this creates a lot of confusion for future developers. If bitmap is being explicitly required in parts of the public APIs, it should be getting included there and not here. Also, why aren't we including the bitmap here, defining it here, and compiling it in src/ like bitset is doing?

cjnolet · 2024-05-24T21:25:58Z

+    if (!sample_filter.has_value()) {                                                             \
+      detail::brute_force_search<T, int64_t>(res, idx, queries, neighbors, distances);            \
+    } else {                                                                                      \
+      detail::brute_force_search<T, int64_t>(                                                     \


I was a little confused on the reasoning for this overload at first. Could we maybe name the second one brute_force_search_filtered?

cjnolet · 2024-05-24T21:26:40Z

@@ -0,0 +1,96 @@
+/*
+ * Copyright (c) 2021-2024, NVIDIA CORPORATION.


Since this is a new file, please only use the 2024 year. I've missed this a couple of times in my PR, but best to fix it where it's noticed.

cjnolet · 2024-05-24T21:27:02Z

@@ -0,0 +1,95 @@
+/*
+ * Copyright (c) 2023-2024, NVIDIA CORPORATION.


Since this is a new file, please use only 2024 year.

cjnolet · 2024-05-24T21:28:22Z

+  auto filter_view =
+    raft::make_device_vector_view<const BitmapT, IdxT>(filter.data(), filter.n_elements());
+
+  raft::detail::popc(res, filter_view, n_queries * n_dataset, nnz_view);


We should never be calling into raft detail APIs in cuvs.

Totally agree! I've noticed this, but as you know, just for so tight schedule to split the raft internal calling to cuVS(I suppose public API needs more regular test/benchmark code, at that time, I had to make it in first). I will fix it ASAP: #158

Thanks for creating an issue for this. Can you please reference a link to the github issue in a comment on this line of code so we don't lose sight of it? Then we can merge this in.

cjnolet · 2024-05-24T21:31:07Z

+                                  raft::make_host_scalar_view<T>(&alpha),
+                                  raft::make_host_scalar_view<T>(&beta));
+    } else {
+      raft::sparse::distance::detail::faster_dot_on_csr(res,


We should never be calling into RAFT detail APIs in cuvs. If you think about the reason why we separate public from details APIs, it's so that each library can put forth a contract that they make guarantees not to break across versions while still hosting internal implementation-specific code that makes no such guarantees. If we're invoking detail APIs from RAFT within cuVS then we're going to end up either 1) making breakig changes those detail APIs and not realizing we just broke cuVS downstream, or 2) never being able to make changes to detail APIs because they need to maintain the same guarantees as public APIs.

This needs to be fixed, but RAFT is in code freeze. What I think you should do to fix this is to create a GIthub issue in cuVS to describe the issue, then add comments in all the places in this PR that you are calling into RAFT detail APIs and reference the Github issue there.

After you do that and address the other PR review feedback, we can merge this PR into 24.06 but you'll need to open up a RAFT PR to immediately expose all of these APIs publicly in RAFT and then open up a PR to invoke the public APIs in cuVS (target both to 24.08 since it's too late to make raft changes for 24.06).

Of course, if you've already exposed these APIs publicly in RAFT and calling into detail was just an oversight, then they could simply be fixed in this PR. The general rule of thumb for detail APIs even within a library is that you should expose any functions publicly if you are going to invoke them across namespace separate namespace boundaries. Obvously that's more simple to do for a header-only library like RAFT than it is for a library where all public APIs are strictly compiled like cuVS, but we should be striving to reduce the amount of inter-namespace calls into detail APIs as possible so we can maintain our public API contracts.

cjnolet · 2024-05-24T21:37:00Z

+
+  void random_array(value_t* array, size_t size)
+  {
+    std::random_device rd;


please generate a raft::random::rectangular_rmat for the bitmap. This is the only way we can test variable degree distributions, which is what we're likely to encounter in practice.

Will be fixed in the next commit

cjnolet · 2024-05-24T21:38:34Z

+
+    brute_force::search(handle, dataset, queries, out_idx, out_val, std::make_optional(filter));
+
+    ASSERT_TRUE(cuvs::spatial::knn::devArrMatchKnnPair(out_idx_expected_d.data(),


raft::spatial::knn is deprecated and I'd prefer to not to transfer that over to cuVS. Please rename this namespace to cuvs::neighbors so it's consistent with all the other nearest neighbors stuff.

cjnolet · 2024-05-24T21:39:13Z

+
+#include <memory>
+
+namespace cuvs::spatial::knn {


Please rename this to cuvs::neighbors

cjnolet · 2024-05-24T22:13:19Z

@rhdong this also needs to be exposed through the Python API.

…tered-bf-dev

cjnolet · 2024-05-29T17:56:52Z

+                                  raft::make_host_scalar_view<T>(&alpha),
+                                  raft::make_host_scalar_view<T>(&beta));
+    } else {
+      raft::sparse::distance::detail::faster_dot_on_csr(res,


Thanks for creating an issue to fix this. Let's get this C++ side merged in. Can you please reference the github issue in a comment just abov ethis line of code so that we don't lose sight of it? Then we can get this merged in.

cjnolet · 2024-05-29T21:28:28Z

/merge

[FEA] support of prefiltered brute force

7357b31

rhdong requested review from a team as code owners May 22, 2024 20:45

github-actions Bot added cpp CMake labels May 22, 2024

rhdong requested review from benfred and cjnolet May 22, 2024 20:45

rhdong added feature request New feature or request non-breaking Introduces a non-breaking change C++ labels May 22, 2024

rhdong commented May 22, 2024

View reviewed changes

rhdong added 2 commits May 22, 2024 15:50

TBR

9fbca1a

Revert "TBR"

c3e14d7

This reverts commit 9fbca1a.

lowener reviewed May 23, 2024

View reviewed changes

rhdong added 8 commits May 22, 2024 19:04

Optimized by the review comments

dd85921

Merge remote-tracking branch 'origin/branch-24.06' into rhdong/prefil…

bdce2a3

…tered-bf

TBR

9ab817a

trigger CI re-run

4aeb3a4

TBR+

1d3adb8

Revert customized raft dep

d505b6b

trigger CI re-run

b69a27b

Revert "trigger CI re-run"

7c5030b

rhdong removed the C++ label May 24, 2024

cjnolet requested changes May 24, 2024

View reviewed changes

cjnolet assigned rhdong May 24, 2024

rhdong added 4 commits May 26, 2024 12:07

Merge remote-tracking branch 'origin/branch-24.06' into rhdong/prefil…

d62fe79

…tered-bf-dev

Optimize by review comments

8788763

Initialize the bitmap with rmat_rectangular_gen

a290192

Merge remote-tracking branch 'origin/branch-24.06' into rhdong/prefil…

5addf70

…tered-bf-dev

cjnolet reviewed May 29, 2024

View reviewed changes

add TODO for detail::popc

8873fde

cjnolet approved these changes May 29, 2024

View reviewed changes

rapids-bot Bot merged commit c533fe3 into NVIDIA:branch-24.06 May 29, 2024

		@@ -0,0 +1,96 @@
		/*
		* Copyright (c) 2021-2024, NVIDIA CORPORATION.

		@@ -0,0 +1,95 @@
		/*
		* Copyright (c) 2023-2024, NVIDIA CORPORATION.


		brute_force::search(handle, dataset, queries, out_idx, out_val, std::make_optional(filter));

		ASSERT_TRUE(cuvs::spatial::knn::devArrMatchKnnPair(out_idx_expected_d.data(),

Uh oh!

Conversation

rhdong commented May 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhdong May 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjnolet commented May 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjnolet commented May 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rhdong May 26, 2024 •

edited

Loading