-
Notifications
You must be signed in to change notification settings - Fork 215
API for CPU side output indices and distances of all_neighbors
#1905
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.
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
Changes from all commits
1e4efbe
2760282
9b0b5d5
bb6691c
985029f
f76cfcf
a6eacef
ddbaca9
2fa76de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -28,7 +28,8 @@ extern "C" { | |||||||||||
| * provide the dataset on host. | ||||||||||||
| * | ||||||||||||
| * Notes: | ||||||||||||
| * - Outputs (indices, distances, core_distances) are expected to be on device memory. | ||||||||||||
| * - Outputs (indices, distances) can be on host memory (numpy arrays) | ||||||||||||
| * or device memory (CUDA arrays). core_distances can only be on device memory. | ||||||||||||
|
Comment on lines
+31
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documentation inconsistency: The documentation states that Update the documentation to clarify that 📝 Proposed documentation fix- * - Outputs (indices, distances) can be on host memory (numpy arrays)
- * or device memory (CUDA arrays). core_distances can only be on device memory.
+ * - Outputs (indices, distances, core_distances) can be on host memory (numpy arrays)
+ * or device memory (CUDA arrays). When core_distances is provided, it must be on the
+ * same memory location as indices and distances.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| * - Host variant accepts host-resident dataset; device variant accepts device-resident dataset. | ||||||||||||
| * - For batching, `overlap_factor < n_clusters` must hold. | ||||||||||||
| * - When `core_distances` is provided, mutual-reachability distances are produced (see alpha). | ||||||||||||
|
|
@@ -94,16 +95,16 @@ CUVS_EXPORT cuvsError_t cuvsAllNeighborsIndexParamsDestroy(cuvsAllNeighborsIndex | |||||||||||
| * resources | ||||||||||||
| * @param[in] params Build parameters (see cuvsAllNeighborsIndexParams) | ||||||||||||
| * @param[in] dataset 2D tensor [num_rows x dim] on host or device (auto-detected) | ||||||||||||
| * @param[out] indices 2D tensor [num_rows x k] on device (int64) | ||||||||||||
| * @param[out] distances Optional 2D tensor [num_rows x k] on device (float32); can be NULL | ||||||||||||
| * @param[out] indices 2D tensor [num_rows x k] on host or device (int64) | ||||||||||||
| * @param[out] distances Optional 2D tensor [num_rows x k] on host or device (float32); can be NULL | ||||||||||||
| * @param[out] core_distances Optional 1D tensor [num_rows] on device (float32); can be NULL | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update The parameter description still states "on device" only, but the implementation allows both host and device memory (matching the location of 📝 Proposed fix- * `@param`[out] core_distances Optional 1D tensor [num_rows] on device (float32); can be NULL
+ * `@param`[out] core_distances Optional 1D tensor [num_rows] on host or device (float32); can be NULL. Must be on the same memory location as indices and distances.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| * @param[in] alpha Mutual-reachability scaling; used only when core_distances is provided | ||||||||||||
| * | ||||||||||||
| * The function automatically detects whether the dataset is host-resident or device-resident | ||||||||||||
| * and calls the appropriate implementation. For host datasets, it partitions data into | ||||||||||||
| * `n_clusters` clusters and assigns each row to `overlap_factor` nearest clusters. For device | ||||||||||||
| * datasets, `n_clusters` must be 1 (no batching); `overlap_factor` is ignored. | ||||||||||||
| * Outputs always reside in device memory. | ||||||||||||
| * Outputs can be on host memory (numpy arrays) or device memory (CUDA arrays). | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing, it sticks out to me. To be honest I didn't check if we used it in other places, but also the other thing is that numpy arrays are just one potential origin for host memory, it could just be C arrays, or say other host python arrays from the python layer, and this seems to imply that there's something that would make these functions need numpy arrays, so I'm leaning towards removing numpy from C docs. |
||||||||||||
| */ | ||||||||||||
| CUVS_EXPORT cuvsError_t cuvsAllNeighborsBuild(cuvsResources_t res, | ||||||||||||
| cuvsAllNeighborsIndexParams_t params, | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,33 +80,36 @@ static cuvs::neighbors::all_neighbors::all_neighbors_params convert_params( | |
| return out; | ||
| } | ||
|
|
||
| static void ensure_indices_dtype_and_device_compatibility(DLManagedTensor* indices) | ||
| static void ensure_indices_dtype_compatibility(DLManagedTensor* indices) | ||
| { | ||
| auto dtype = indices->dl_tensor.dtype; | ||
| RAFT_EXPECTS(dtype.code == kDLInt && dtype.bits == 64, "indices must be int64 output tensor"); | ||
| RAFT_EXPECTS(cuvs::core::is_dlpack_device_compatible(indices->dl_tensor), | ||
| "indices tensor must be device-compatible"); | ||
| RAFT_EXPECTS(cuvs::core::is_dlpack_device_compatible(indices->dl_tensor) || | ||
| cuvs::core::is_dlpack_host_compatible(indices->dl_tensor), | ||
| "indices tensor must be either device-compatible or host-compatible"); | ||
| } | ||
|
|
||
| static void ensure_optional_distance_dtype_and_device_compatibility(DLManagedTensor* distances) | ||
| static void ensure_optional_distance_dtype_compatibility(DLManagedTensor* distances) | ||
| { | ||
| if (distances == nullptr) { return; } | ||
| auto dtype = distances->dl_tensor.dtype; | ||
| RAFT_EXPECTS(dtype.code == kDLFloat && dtype.bits == 32, | ||
| "distances must be float32 output tensor"); | ||
| RAFT_EXPECTS(cuvs::core::is_dlpack_device_compatible(distances->dl_tensor), | ||
| "distances tensor must be device-compatible"); | ||
| RAFT_EXPECTS(cuvs::core::is_dlpack_device_compatible(distances->dl_tensor) || | ||
| cuvs::core::is_dlpack_host_compatible(distances->dl_tensor), | ||
| "distances tensor must be either device-compatible or host-compatible"); | ||
| } | ||
|
|
||
| static void ensure_optional_core_distance_dtype_and_device_compatibility( | ||
| DLManagedTensor* core_distances) | ||
| static void ensure_optional_core_distance_dtype_compatibility(DLManagedTensor* core_distances) | ||
| { | ||
| if (core_distances == nullptr) { return; } | ||
| auto dtype = core_distances->dl_tensor.dtype; | ||
| RAFT_EXPECTS(dtype.code == kDLFloat && dtype.bits == 32, | ||
| "core_distances must be float32 output tensor"); | ||
| RAFT_EXPECTS(cuvs::core::is_dlpack_device_compatible(core_distances->dl_tensor), | ||
| "core_distances tensor must be device-compatible"); | ||
| RAFT_EXPECTS( | ||
| cuvs::core::is_dlpack_device_compatible(core_distances->dl_tensor) || | ||
| cuvs::core::is_dlpack_host_compatible(core_distances->dl_tensor), | ||
| "core_distances tensor must be either device-compatible or host-compatible"); | ||
| } | ||
|
|
||
| template <typename T> | ||
|
|
@@ -124,9 +127,9 @@ void _build_host(cuvsResources_t res, | |
| RAFT_EXPECTS(cuvs::core::is_dlpack_host_compatible(dlt), | ||
| "Host build expects host-compatible dataset tensor"); | ||
|
|
||
| ensure_indices_dtype_and_device_compatibility(indices_tensor); | ||
| ensure_optional_distance_dtype_and_device_compatibility(distances_tensor); | ||
| ensure_optional_core_distance_dtype_and_device_compatibility(core_distances_tensor); | ||
| ensure_indices_dtype_compatibility(indices_tensor); | ||
| ensure_optional_distance_dtype_compatibility(distances_tensor); | ||
| ensure_optional_core_distance_dtype_compatibility(core_distances_tensor); | ||
|
|
||
| // Check dependencies between parameters | ||
| if (core_distances_tensor != nullptr && distances_tensor == nullptr) { | ||
|
|
@@ -138,26 +141,63 @@ void _build_host(cuvsResources_t res, | |
|
|
||
| auto cpp_params = convert_params(params, n_rows, n_cols); | ||
|
|
||
| using dataset_mdspan_t = raft::host_matrix_view<const T, int64_t, raft::row_major>; | ||
| using indices_mdspan_t = raft::device_matrix_view<int64_t, int64_t, raft::row_major>; | ||
| using distances_mdspan_t = raft::device_matrix_view<float, int64_t, raft::row_major>; | ||
| using core_mdspan_t = raft::device_vector_view<float, int64_t>; | ||
| using dataset_mdspan_t = raft::host_matrix_view<const T, int64_t, raft::row_major>; | ||
|
|
||
| auto dataset = cuvs::core::from_dlpack<dataset_mdspan_t>(dataset_tensor); | ||
| auto indices = cuvs::core::from_dlpack<indices_mdspan_t>(indices_tensor); | ||
| bool indices_is_host = cuvs::core::is_dlpack_host_compatible(indices_tensor->dl_tensor); | ||
| bool distances_is_host = distances_tensor ? cuvs::core::is_dlpack_host_compatible(distances_tensor->dl_tensor) : indices_is_host; | ||
|
|
||
| std::optional<distances_mdspan_t> distances = std::nullopt; | ||
| if (distances_tensor) { | ||
| distances = cuvs::core::from_dlpack<distances_mdspan_t>(distances_tensor); | ||
| if (distances_tensor && distances_is_host != indices_is_host) { | ||
| RAFT_FAIL("distances and indices must be on the same memory location (both host or both device)"); | ||
| } | ||
|
|
||
| std::optional<core_mdspan_t> core_distances = std::nullopt; | ||
| if (core_distances_tensor) { | ||
| core_distances = cuvs::core::from_dlpack<core_mdspan_t>(core_distances_tensor); | ||
| bool core_distances_is_host = | ||
| cuvs::core::is_dlpack_host_compatible(core_distances_tensor->dl_tensor); | ||
| RAFT_EXPECTS(core_distances_is_host == indices_is_host, | ||
| "core_distances must be on the same memory location as indices and distances"); | ||
| } | ||
|
|
||
| cuvs::neighbors::all_neighbors::build( | ||
| cpp_res, cpp_params, dataset, indices, distances, core_distances, alpha); | ||
| auto dataset = cuvs::core::from_dlpack<dataset_mdspan_t>(dataset_tensor); | ||
|
|
||
| if (indices_is_host) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The if (indices_is_host) { … } else { … } blocks here are essentially the same 30-ish lines duplicated four times across _build_host and _build_device (host branch and device branch in each). Would you be open to extracting a small templated helper that takes the dataset mdspan type + a host/device tag and does the optional unpacking + dispatch? Should cut this down a lot, or is this cod that'll significantly change in teh follow up and not worth the change? |
||
| using indices_mdspan_t = raft::host_matrix_view<int64_t, int64_t, raft::row_major>; | ||
| using distances_mdspan_t = raft::host_matrix_view<float, int64_t, raft::row_major>; | ||
| using core_mdspan_t = raft::host_vector_view<float, int64_t>; | ||
|
|
||
| auto indices = cuvs::core::from_dlpack<indices_mdspan_t>(indices_tensor); | ||
|
|
||
| std::optional<distances_mdspan_t> distances = std::nullopt; | ||
| if (distances_tensor) { | ||
| distances = cuvs::core::from_dlpack<distances_mdspan_t>(distances_tensor); | ||
| } | ||
|
|
||
| std::optional<core_mdspan_t> core_distances = std::nullopt; | ||
| if (core_distances_tensor) { | ||
| core_distances = cuvs::core::from_dlpack<core_mdspan_t>(core_distances_tensor); | ||
| } | ||
|
|
||
| cuvs::neighbors::all_neighbors::build( | ||
| cpp_res, cpp_params, dataset, indices, distances, core_distances, alpha); | ||
| } else { | ||
| using indices_mdspan_t = raft::device_matrix_view<int64_t, int64_t, raft::row_major>; | ||
| using distances_mdspan_t = raft::device_matrix_view<float, int64_t, raft::row_major>; | ||
| using core_mdspan_t = raft::device_vector_view<float, int64_t>; | ||
|
|
||
| auto indices = cuvs::core::from_dlpack<indices_mdspan_t>(indices_tensor); | ||
|
|
||
| std::optional<distances_mdspan_t> distances = std::nullopt; | ||
| if (distances_tensor) { | ||
| distances = cuvs::core::from_dlpack<distances_mdspan_t>(distances_tensor); | ||
| } | ||
|
|
||
| std::optional<core_mdspan_t> core_distances = std::nullopt; | ||
| if (core_distances_tensor) { | ||
| core_distances = cuvs::core::from_dlpack<core_mdspan_t>(core_distances_tensor); | ||
| } | ||
|
|
||
| cuvs::neighbors::all_neighbors::build( | ||
| cpp_res, cpp_params, dataset, indices, distances, core_distances, alpha); | ||
| } | ||
| } | ||
|
|
||
| template <typename T> | ||
|
|
@@ -175,9 +215,9 @@ void _build_device(cuvsResources_t device_res, | |
| RAFT_EXPECTS(cuvs::core::is_dlpack_device_compatible(dlt), | ||
| "Device build expects device-compatible dataset tensor"); | ||
|
|
||
| ensure_indices_dtype_and_device_compatibility(indices_tensor); | ||
| ensure_optional_distance_dtype_and_device_compatibility(distances_tensor); | ||
| ensure_optional_core_distance_dtype_and_device_compatibility(core_distances_tensor); | ||
| ensure_indices_dtype_compatibility(indices_tensor); | ||
| ensure_optional_distance_dtype_compatibility(distances_tensor); | ||
| ensure_optional_core_distance_dtype_compatibility(core_distances_tensor); | ||
|
|
||
| // Check dependencies between parameters | ||
| if (core_distances_tensor != nullptr && distances_tensor == nullptr) { | ||
|
|
@@ -189,26 +229,62 @@ void _build_device(cuvsResources_t device_res, | |
|
|
||
| auto cpp_params = convert_params(params, n_rows, n_cols); | ||
|
|
||
| using dataset_mdspan_t = raft::device_matrix_view<const T, int64_t, raft::row_major>; | ||
| using indices_mdspan_t = raft::device_matrix_view<int64_t, int64_t, raft::row_major>; | ||
| using distances_mdspan_t = raft::device_matrix_view<float, int64_t, raft::row_major>; | ||
| using core_mdspan_t = raft::device_vector_view<float, int64_t>; | ||
| using dataset_mdspan_t = raft::device_matrix_view<const T, int64_t, raft::row_major>; | ||
| auto dataset = cuvs::core::from_dlpack<dataset_mdspan_t>(dataset_tensor); | ||
|
|
||
| auto dataset = cuvs::core::from_dlpack<dataset_mdspan_t>(dataset_tensor); | ||
| auto indices = cuvs::core::from_dlpack<indices_mdspan_t>(indices_tensor); | ||
| bool indices_is_host = cuvs::core::is_dlpack_host_compatible(indices_tensor->dl_tensor); | ||
| bool distances_is_host = distances_tensor ? cuvs::core::is_dlpack_host_compatible(distances_tensor->dl_tensor) : indices_is_host; | ||
|
|
||
| std::optional<distances_mdspan_t> distances = std::nullopt; | ||
| if (distances_tensor) { | ||
| distances = cuvs::core::from_dlpack<distances_mdspan_t>(distances_tensor); | ||
| if (distances_tensor && distances_is_host != indices_is_host) { | ||
| RAFT_FAIL("distances and indices must be on the same memory location (both host or both device)"); | ||
| } | ||
|
|
||
| std::optional<core_mdspan_t> core_distances = std::nullopt; | ||
| if (core_distances_tensor) { | ||
| core_distances = cuvs::core::from_dlpack<core_mdspan_t>(core_distances_tensor); | ||
| bool core_distances_is_host = | ||
| cuvs::core::is_dlpack_host_compatible(core_distances_tensor->dl_tensor); | ||
| RAFT_EXPECTS(core_distances_is_host == indices_is_host, | ||
| "core_distances must be on the same memory location as indices and distances"); | ||
| } | ||
|
|
||
| cuvs::neighbors::all_neighbors::build( | ||
| cpp_res, cpp_params, dataset, indices, distances, core_distances, alpha); | ||
| if (indices_is_host) { | ||
| using indices_mdspan_t = raft::host_matrix_view<int64_t, int64_t, raft::row_major>; | ||
| using distances_mdspan_t = raft::host_matrix_view<float, int64_t, raft::row_major>; | ||
| using core_mdspan_t = raft::host_vector_view<float, int64_t>; | ||
|
|
||
| auto indices = cuvs::core::from_dlpack<indices_mdspan_t>(indices_tensor); | ||
|
|
||
| std::optional<distances_mdspan_t> distances = std::nullopt; | ||
| if (distances_tensor) { | ||
| distances = cuvs::core::from_dlpack<distances_mdspan_t>(distances_tensor); | ||
| } | ||
|
|
||
| std::optional<core_mdspan_t> core_distances = std::nullopt; | ||
| if (core_distances_tensor) { | ||
| core_distances = cuvs::core::from_dlpack<core_mdspan_t>(core_distances_tensor); | ||
| } | ||
|
|
||
| cuvs::neighbors::all_neighbors::build( | ||
| cpp_res, cpp_params, dataset, indices, distances, core_distances, alpha); | ||
| } else { | ||
| using indices_mdspan_t = raft::device_matrix_view<int64_t, int64_t, raft::row_major>; | ||
| using distances_mdspan_t = raft::device_matrix_view<float, int64_t, raft::row_major>; | ||
| using core_mdspan_t = raft::device_vector_view<float, int64_t>; | ||
|
|
||
| auto indices = cuvs::core::from_dlpack<indices_mdspan_t>(indices_tensor); | ||
|
|
||
| std::optional<distances_mdspan_t> distances = std::nullopt; | ||
| if (distances_tensor) { | ||
| distances = cuvs::core::from_dlpack<distances_mdspan_t>(distances_tensor); | ||
| } | ||
|
|
||
| std::optional<core_mdspan_t> core_distances = std::nullopt; | ||
| if (core_distances_tensor) { | ||
| core_distances = cuvs::core::from_dlpack<core_mdspan_t>(core_distances_tensor); | ||
| } | ||
|
|
||
| cuvs::neighbors::all_neighbors::build( | ||
| cpp_res, cpp_params, dataset, indices, distances, core_distances, alpha); | ||
| } | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kinda odd to see Python terminology in the C header, I guess one of the main consumers are the python apis, but still... not necessarily suggesting to change it, but was wondering ig it might be better to keep the docstring to C relevant concepts (like dlpack)