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

FAISS with cuVS enabled in cuvs-bench #561

Draft
wants to merge 7 commits into
base: branch-25.02
Choose a base branch
from

Conversation

tarang-jain
Copy link
Contributor

Mostly adapted from rapidsai/raft#2026

Copy link

copy-pr-bot bot commented Jan 8, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@tarang-jain tarang-jain added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Jan 9, 2025
@@ -483,7 +483,14 @@ void register_search(std::shared_ptr<const dataset<T>> dataset,
->MeasureProcessCPUTime()
->UseRealTime();

if (metric_objective == Mode::kThroughput) { b->ThreadRange(threads[0], threads[1]); }
if (metric_objective == Mode::kThroughput) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's not put algorithm-specific stuff in the common files. I agree with @achirkin about this one. If needed, you could propoagate this down to the specific algorithms, but the common stuff should be agnostic of he algoithmmic specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @cjnolet for spotting this. @tarang-jain , could you please move the warning into the faiss-related header (perhaps into the set_search_params)? There, you should be able to query the cuvs::bench::benchmark_n_threads > 1 from common/util.hpp.

Copy link
Contributor

@achirkin achirkin left a comment

Choose a reason for hiding this comment

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

Here are couple suggestions on my side to keep the faiss behavior in line with other algorithms.

Comment on lines +156 to +169
int main(int argc, char** argv)
{
rmm::mr::cuda_memory_resource cuda_mr;
// Construct a resource that uses a coalescing best-fit pool allocator
// and is initially sized to half of free device memory.
rmm::mr::pool_memory_resource<rmm::mr::cuda_memory_resource> pool_mr{
&cuda_mr, rmm::percent_of_free_device_memory(50)};
// Updates the current device resource pointer to `pool_mr`
auto old_mr = rmm::mr::set_current_device_resource(&pool_mr);
auto ret = cuvs::bench::run_main(argc, argv);
// Restores the current device resource pointer to its previous value
rmm::mr::set_current_device_resource(old_mr);
return ret;
}
Copy link
Contributor

@achirkin achirkin Jan 10, 2025

Choose a reason for hiding this comment

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

We've run in many issues in ann-bench in the past doing the program-wide setting of memory resource. This also changes the behavior depending on whether ANN_BENCH_BUILD_MAIN flag is enabled. Therefore, it's best if these lines stay unmodified and same for all algorithms.
Please move this resource setting to the place where the index wrapper is constructed/destructed in the algo header file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improves an existing functionality non-breaking Introduces a non-breaking change Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants