Add innerproduct to the pairwise distance api #1226

benfred · 2023-02-02T04:45:13Z

No description provided.

mdoijade

I believe it would be ideal to use cuBLAS for inner product instead of pairwiseDistanceMatKernel perf wise.

This adds InnerProduct distance to the pairwise distances api, using cublass gemm to compute the distance. Since this requires a cublas handle, this also changes the distance api to take a raft::device_resources instead of just a cuda stream

benfred · 2023-02-07T00:08:45Z

@mdoijade I've updated to use cublas here - I had to change the distance api to use a raft::device_resources instead of a just a cuda stream in order to pass in the cublas handle though, which makes this somewhat of a bigger change.

I think in general though we should be passing device_resources instead of cuda streams - so I am hoping that this is ok. @cjnolet what do you think?

cjnolet · 2023-02-07T00:36:25Z

cpp/include/raft/distance/detail/kernels/gram_matrix.cuh

@@ -212,7 +212,7 @@ class GramMatrixBase {
                        int ld_out)
  {
    raft::distance::distance<raft::distance::DistanceType::L2Unexpanded, math_t, math_t, math_t>(
-      x1, x2, out, n1, n2, n_cols, stream, is_row_major);
+      raft::device_resources(stream), x1, x2, out, n1, n2, n_cols, is_row_major);


I think from the public API standpoint, we want to strive to accept device_resources everywhere (and eventually just raft::resources) but things inside of detail are okay to accept just the individual resources they need- so long as the public API is pulling resources like streams from the raft::resources speified from the user and not creating their own.

For example, I'm okay with this function accepting the stream directly since it's in the detail namespace. The kernel gramm API itself is in kind of weird state right now. We moved it over from cuml mostly because it's 1) primitives built directly from the pairwise distances API, and 2) it required just enough customization that it was hard to use the existing specializtions and thus cuml's compile time was increased.

At some point, we do need to refactor the kernel gramm API into a set of flattened stateless public API functions but for now it's kind of been left in this state just so cuml doesn't break.

yeah - I didn't want to break this here by changing the kernel gramm API (for the other changes, cuml is using the pairwise_distances api's which already take a raft handle instead of a stream afaict).

This is a little weird in that its upgrading a stream -> device_resources, but since the L2Unexpanded distance here only uses the stream, this won't do anything funky like allocate a cublas handle.

cpp/include/raft/distance/detail/distance.cuh

mdoijade · 2023-02-07T07:21:31Z

@mdoijade I've updated to use cublas here - I had to change the distance api to use a raft::device_resources instead of a just a cuda stream in order to pass in the cublas handle though, which makes this somewhat of a bigger change.

I think in general though we should be passing device_resources instead of cuda streams - so I am hoping that this is ok. @cjnolet what do you think?

Thanks @benfred this looks good to me. Do you think it is worth adding test case for inner product on the c++ distance tests layer as well ?

cjnolet

The changes look great overall! Mostly very minor things.

cpp/src/distance/distance/specializations/detail/inner_product.cu

cjnolet · 2023-02-09T01:31:53Z

python/pylibraft/pylibraft/test/test_distance.py

@@ -57,7 +58,10 @@ def test_distance(n_rows, n_cols, inplace, metric, order, dtype):

    output = np.zeros((n_rows, n_rows), dtype=dtype)

-    expected = cdist(input1, input1, metric)
+    if metric == "inner_product":


We mentioned offline, we should test this through both the C++ and python APIs here. If you think it'd be helpful, I don't mind helping to add a quick google test case here.

thanks for the pointers! I can't believe I missed the c++ tests for the other metrics (didn't see them when I first looked for some , though its super obvious they are there). Added in the last commit -

Haha no problem. I just know a lot of things have been filling up people's plates lately so I didn't mind jumping in to help if needed.

cjnolet · 2023-02-09T01:34:28Z

cpp/include/raft/distance/distance.cuh

@@ -109,20 +108,20 @@ template <raft::distance::DistanceType distanceType,
          typename AccType,
          typename OutType,
          typename Index_ = int>
-void distance(const InType* x,
+void distance(raft::resources const& handle,


This might be a breaking change, though it might not? (e.g. comment here). We probably want to build cuml w/ this change at least to check.

testing out w/ cuml here rapidsai/cuml#5230

needs rapidsai/cuml@6f0d7fc in cuml looks like

cjnolet · 2023-02-09T01:34:52Z

cpp/include/raft/distance/distance.cuh

@@ -202,21 +200,22 @@ template <raft::distance::DistanceType distanceType,
          typename AccType,
          typename OutType,
          typename Index_ = int>
-void distance(const InType* x,
+void distance(raft::resources const& handle,


Same above (checking / updating cuml as needed).

cjnolet · 2023-02-09T01:35:10Z

cpp/include/raft/distance/distance.cuh

@@ -238,7 +237,7 @@ void distance(const InType* x,
 * @param metric_arg metric argument (used for Minkowski distance)
 */
 template <typename Type, typename Index_ = int>
-void pairwise_distance(raft::device_resources const& handle,
+void pairwise_distance(raft::resources const& handle,


I don't think this one is breaking

… pairwise_inner_product

Test out the changes in rapidsai/raft#1226, to make sure they don't break cuml

cjnolet · 2023-02-11T02:20:30Z

cpp/src/distance/distance/specializations/detail/canberra_double_double_double_int.cu

@@ -19,20 +19,8 @@
 namespace raft {
 namespace distance {
 namespace detail {
-template void distance<raft::distance::DistanceType::Canberra, float, float, float, int>(
-  const float* x,


Oh wow, so we were building the same canberra specialization multiple times? It makes sense now...

cjnolet

LGTM!

benfred · 2023-02-13T16:38:45Z

/merge

Test out the changes in rapidsai/raft#1226, to make sure they don't break cuml

Fixes for supporting InnerProduct distance in the pairwise_distance api - required to handle the changes in rapidsai/raft#1226

Fixes for supporting InnerProduct distance in the pairwise_distance api - required to handle the changes in rapidsai/raft#1226. Also resolves #4078. That fix was necessary to tack on to this PR due to upstream RAPIDS updates to the spdlog version (in rmm via rapids-cmake). Authors: - Ben Frederickson (https://github.com/benfred) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #5230

benfred added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 2, 2023

benfred requested review from a team as code owners February 2, 2023 04:45

benfred marked this pull request as draft February 2, 2023 04:45

github-actions bot added CMake cpp python labels Feb 2, 2023

mdoijade suggested changes Feb 2, 2023

View reviewed changes

cjnolet assigned benfred Feb 3, 2023

Add InnerProduct to pairwise distances api

c18564f

This adds InnerProduct distance to the pairwise distances api, using cublass gemm to compute the distance. Since this requires a cublas handle, this also changes the distance api to take a raft::device_resources instead of just a cuda stream

benfred force-pushed the pairwise_inner_product branch from a94ceb6 to c18564f Compare February 7, 2023 00:03

Merge branch 'branch-23.04' into pairwise_inner_product

d01e53a

benfred marked this pull request as ready for review February 7, 2023 00:08

cjnolet reviewed Feb 7, 2023

View reviewed changes

cpp/include/raft/distance/detail/distance.cuh Outdated Show resolved Hide resolved

cjnolet reviewed Feb 7, 2023

View reviewed changes

cpp/include/raft/distance/detail/distance.cuh Show resolved Hide resolved

benfred and others added 5 commits February 7, 2023 11:06

benchmark build fix

f400477

Use raft::resources instead of raft::device_resources

713b9d3

style

35b7398

Merge branch 'branch-23.04' into pairwise_inner_product

3492ca4

Merge branch 'branch-23.04' into pairwise_inner_product

c184175

cjnolet requested changes Feb 9, 2023

View reviewed changes

benfred added 3 commits February 10, 2023 14:18

Add C++ test for IP distance

47f5fdb

Split IP specializations into separate files

1ff9fbb

Merge branch 'pairwise_inner_product' of github.com:benfred/raft into…

e61d863

… pairwise_inner_product

benfred added a commit to benfred/cuml that referenced this pull request Feb 10, 2023

test out innerproduct pairwise_distances changes

80f16a9

Test out the changes in rapidsai/raft#1226, to make sure they don't break cuml

benfred mentioned this pull request Feb 10, 2023

Support innerproduct distance in the pairwise_distance API rapidsai/cuml#5230

Merged

Merge branch 'branch-23.04' into pairwise_inner_product

27da08d

cjnolet reviewed Feb 11, 2023

View reviewed changes

cjnolet approved these changes Feb 11, 2023

View reviewed changes

benfred added 2 commits February 10, 2023 22:42

fix

b589d5d

Merge branch 'branch-23.04' into pairwise_inner_product

4dbbcca

rapids-bot bot merged commit cca81c4 into rapidsai:branch-23.04 Feb 13, 2023

benfred deleted the pairwise_inner_product branch February 13, 2023 16:45

ajschmidt8 pushed a commit to rapidsai/cuml that referenced this pull request Feb 13, 2023

test out innerproduct pairwise_distances changes

250e8be

Test out the changes in rapidsai/raft#1226, to make sure they don't break cuml

rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Feb 13, 2023

Support innerproduct distance in the pairwise_distance API

4eb6ba2

Fixes for supporting InnerProduct distance in the pairwise_distance api - required to handle the changes in rapidsai/raft#1226

benfred mentioned this pull request Feb 15, 2023

[FEA] New brute-force API needs to support at least the metrics supported by FAISS' bfknn #1159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add innerproduct to the pairwise distance api #1226

Add innerproduct to the pairwise distance api #1226

benfred commented Feb 2, 2023

mdoijade left a comment

benfred commented Feb 7, 2023

cjnolet Feb 7, 2023

cjnolet Feb 7, 2023

benfred Feb 8, 2023 •

edited

Loading

mdoijade commented Feb 7, 2023

cjnolet left a comment

cjnolet Feb 9, 2023

benfred Feb 10, 2023

cjnolet Feb 11, 2023

cjnolet Feb 9, 2023

benfred Feb 10, 2023

benfred Feb 11, 2023

cjnolet Feb 9, 2023

cjnolet Feb 9, 2023

cjnolet Feb 11, 2023

cjnolet left a comment

benfred commented Feb 13, 2023

Add innerproduct to the pairwise distance api #1226

Add innerproduct to the pairwise distance api #1226

Conversation

benfred commented Feb 2, 2023

mdoijade left a comment

Choose a reason for hiding this comment

benfred commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benfred Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

mdoijade commented Feb 7, 2023

cjnolet 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

cjnolet left a comment

Choose a reason for hiding this comment

benfred commented Feb 13, 2023

benfred Feb 8, 2023 •

edited

Loading