Skip to content
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

Add FAISS with RAFT enabled Benchmarking to raft-ann-bench #2026

Merged
merged 165 commits into from
Jun 17, 2024

Conversation

tarang-jain
Copy link
Contributor

@tarang-jain tarang-jain commented Nov 29, 2023

  • RMM pooled resource for RAFT enabled FAISS
  • Update yaml config for faiss_gpu_ivf_flat, faiss_gpu_ivf_pq
  • Small fix FAISS GPU IVFPQ params
  • Update get_faiss.cmake
  • Fix SIGSEGV

Notes: The StandardGpuResources object is part of FAISS' index classes. As a result, there is no way of creating a separate Raft handle for each thread without creating a new instance of the whole index object for a new thread. As such, multi-threaded benchmarking for GPU indices will not work.
For CPU indices, @divyegala had seen some other issues. Though this PR is mainly about raft-enabled FAISS GPU indices.

tarang-jain and others added 30 commits June 30, 2023 17:03
@tarang-jain
Copy link
Contributor Author

/ok to test

@tarang-jain
Copy link
Contributor Author

/ok to test

@tarang-jain tarang-jain requested review from a team as code owners June 7, 2024 20:04
@github-actions github-actions bot added the ci label Jun 7, 2024
@tarang-jain tarang-jain changed the base branch from branch-24.06 to branch-24.08 June 7, 2024 20:05
@github-actions github-actions bot removed the ci label Jun 7, 2024
@tarang-jain
Copy link
Contributor Author

/ok to test

@tarang-jain
Copy link
Contributor Author

/ok to test

Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Hi Tarang, looks good, but one issue is not addressed yet.

@@ -208,7 +234,7 @@ void FaissGpu<T>::build(const T* dataset, size_t nrow, cudaStream_t stream)
nlist_,
index_ivf->cp.min_points_per_centroid);
}
index_ivf->cp.max_points_per_centroid = max_ppc;
index_ivf->cp.max_points_per_centroid = 300;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change it back to max_ppc?

base:
build:
nlist: [2048]
ratio: [1, 4, 10]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not add multiple build ratios, just pick one.

@tarang-jain
Copy link
Contributor Author

/ok to test

Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, the PR looks good to me.

@raydouglass raydouglass removed the request for review from a team June 13, 2024 18:28
@tarang-jain
Copy link
Contributor Author

tarang-jain commented Jun 13, 2024

/ok to test

1 similar comment
@tarang-jain
Copy link
Contributor Author

/ok to test

@tfeher
Copy link
Contributor

tfeher commented Jun 17, 2024

/merge

Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Approving on behalf of packaging-codeowners. I just narrowly looked at packaging concerns here, not the functional changes.

@@ -115,4 +116,4 @@ endfunction()
find_and_configure_faiss(
BUILD_STATIC_LIBS ${RAFT_USE_FAISS_STATIC}
ENABLE_GPU ${RAFT_FAISS_ENABLE_GPU}
)
)
Copy link
Member

Choose a reason for hiding this comment

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

I noticed several files in this PR that do not end with a newline.

In practice I've found that this rarely matters, but it can occasionally cause issues for tools doing line-by-line parsing of files. In a future PR, you may want to consider automatically fixing these with pre-commit, like cudf does: https://github.com/rapidsai/cudf/blob/107753ccaacdb62287c4dd4351e5caf3bf8bc62a/.pre-commit-config.yaml#L13

Copy link
Member

@benfred benfred left a comment

Choose a reason for hiding this comment

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

aside from a couple super minor nitpicks - lgtm! Preapproving,



def faiss_gpu_ivf_pq_build_constraints(params, dims):
ret = True
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ret = True

Comment on lines +62 to +64
pq_bits = 8
if "bitsPerCode" in params:
pq_bits = params["bitsPerCode"]
Copy link
Member

Choose a reason for hiding this comment

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

nitpick:

Suggested change
pq_bits = 8
if "bitsPerCode" in params:
pq_bits = params["bitsPerCode"]
pq_bits = params.get("bitsPerCode", 8)

@rapids-bot rapids-bot bot merged commit 8fe2983 into rapidsai:branch-24.08 Jun 17, 2024
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Benchmarks CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
Development

Successfully merging this pull request may close these issues.

6 participants