Skip to content

[Fix] Various fixes for 25.02.01 point release #695

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

Merged
merged 7 commits into from
Feb 24, 2025
Merged

[Fix] Various fixes for 25.02.01 point release #695

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac5cc15
[Fix] CAGRA 'merge' API compilation error under CUDA 12.4
rhdong Feb 14, 2025
8447421
fix by index::dataset_index_t
rhdong Feb 14, 2025
d4366ce
[Fix] Duplicated node issue on Cagra NN-descent
rhdong Feb 14, 2025
88b018d
Fix for a crash situation when using different threads (#680)
Feb 13, 2025
1b36e8c
Java: Fixing the broken pre-filtering for Brute Force
Feb 19, 2025
85d8644
FIX add sklearn as dependency of cuvs-bench
dantegd Feb 20, 2025
6a36092
[Fix] Brute force with prefilter java test failure
rhdong Feb 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions cpp/src/neighbors/detail/cagra/cagra_merge.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ index<T, IdxT> merge(raft::resources const& handle,
for (auto index : indices) {
RAFT_EXPECTS(index != nullptr,
"Null pointer detected in 'indices'. Ensure all elements are valid before usage.");
using ds_idx_type = decltype(index->data().n_rows());
if (auto* strided_dset = dynamic_cast<const strided_dataset<T, ds_idx_type>*>(&index->data());
if (auto* strided_dset = dynamic_cast<const strided_dataset<T, int64_t>*>(&index->data());
Copy link
Contributor

@achirkin achirkin Feb 14, 2025

Choose a reason for hiding this comment

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

Hardcoding the dataset index type like this is prone to error: if we decide to change it (let say to IdxT) in the index header at some point, it will compile and break the code unnoticed - the cast will simply fail.
If you really can't use the decltype as above for some reason, perhaps a better way would be to define a new alias in the index definition (e.g. using dataset_index_type = int64_t) and then make the dataset member use that type in the template parameter.

Copy link
Contributor

@achirkin achirkin Feb 14, 2025

Choose a reason for hiding this comment

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

You can also try to just rename the index variable here. Maybe it confuses the compiler because of the same template type name.

Copy link
Member Author

@rhdong rhdong Feb 14, 2025

Choose a reason for hiding this comment

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

You can also try to just rename the index variable here. Maybe it confuses the compiler because of the same template type name.

Hi @achirkin @cjnolet, I've tried it, but it still fails.

/cuvs/cpp/src/neighbors/detail/cagra/cagra_merge.cuh:56:21: error: '__T23' does not name a type
   56 |     using ds_idx_type = decltype(l_index->data().n_rows());

9357f8b

Copy link
Member Author

@rhdong rhdong Feb 14, 2025

Choose a reason for hiding this comment

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

Hardcoding the dataset index type like this is prone to error: if we decide to change it (let say to IdxT) in the index header at some point, it will compile and break the code unnoticed - the cast will simply fail. If you really can't use the decltype as above for some reason, perhaps a better way would be to define a new alias in the index definition (e.g. using dataset_index_type = int64_t) and then make the dataset member use that type in the template parameter.

Yh, this works: 70e3036
And, if you @achirkin @cjnolet all believe this is the satisfied solution, I will commit it on this branch(now it is in my personal branch.) Thanks!

Copy link
Member Author

@rhdong rhdong Feb 14, 2025

Choose a reason for hiding this comment

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

Hi @cjnolet @achirkin , I committed the dataset_index_t solution for time-saving,

strided_dset != nullptr) {
if (dim == 0) {
dim = index->dim();
Expand All @@ -75,8 +74,7 @@ index<T, IdxT> merge(raft::resources const& handle,

auto merge_dataset = [&](T* dst) {
for (auto index : indices) {
using ds_idx_type = decltype(index->data().n_rows());
auto* strided_dset = dynamic_cast<const strided_dataset<T, ds_idx_type>*>(&index->data());
auto* strided_dset = dynamic_cast<const strided_dataset<T, int64_t>*>(&index->data());

RAFT_CUDA_TRY(cudaMemcpy2DAsync(dst + offset * dim,
sizeof(T) * dim,
Expand Down
Loading