-
Notifications
You must be signed in to change notification settings - Fork 73
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
ANN_BENCH #130
ANN_BENCH #130
Conversation
Signed-off-by: Mickael Ide <[email protected]>
Co-authored-by: Tamas Bela Feher <[email protected]>
I've just realized the benchmarks are not compiled during |
NB: some algorithms are likely to fail with OOM thrown by the limiting_resource_adapter with very large datasets (e.g. DEEP-1B); the fix is in #181 |
…marks when the --limit-bench-ann argument is not passed to build.sh
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.
Thanks Artem for the updates. This PR ports the existing infrastructure from raft and enables us to run all the existing the benchmarks. I believe it would be useful to have this merged, and work o follow up improvements in separate PRs. I have added open discussion points to the tracker issue #160 (comment).
The PR looks good to me. I have also contributed to the PR, so I shall not be the only approver.
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.
I've reviewed on behalf of packaging-codeowners
. Left 2 small comments, neither needs to block merging this.
@@ -1,5 +1,5 @@ | |||
#!/usr/bin/env bash | |||
# Copyright (c) 2022-2024, NVIDIA CORPORATION. | |||
|
|||
./build.sh tests --allgpuarch --no-nvtx --build-metrics=tests_bench --incl-cache-stats | |||
./build.sh tests bench-ann --allgpuarch --no-nvtx --build-metrics=tests_bench --incl-cache-stats |
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.
Over in RAFT, bench-ann
is its own package with its own dependencies, build scripts, etc.:
For my own understanding in reviewing this... why is this being added to the libcuvs-tests
package here instead of creating a new standalone package as exists in RAFT?
Having these separate things be their own packages can be helpful for parallelizing development, limiting the potential impact of packaging changes, and speeding up debugging.
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.
I agree that benchmarks should probably be a separate package. I'm only hesitating to add this because I'm not very familiar with this conda/CI setup. I hoped to postpone this question till a follow-on PR together with the naming decision (#160 (comment)). @benfred, @cjnolet , what do you think?
@@ -55,6 +55,7 @@ option(BUILD_SHARED_LIBS "Build cuvs shared libraries" ON) | |||
option(BUILD_TESTS "Build cuvs unit-tests" ON) | |||
option(BUILD_C_LIBRARY "Build raft C API library" OFF) | |||
option(BUILD_C_TESTS "Build raft C API tests" OFF) | |||
option(BUILD_ANN_BENCH "Build cuVS ann benchmarks" OFF) |
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.
Can we add an option to build this in CI?
It looks to me that this is off by default (which is fine), but that does mean that we don't have any guarantee that any of this is compiling.
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.
We enable this currently together with tests in build.sh (see #130 (comment))
/merge |
Porting the ANN benchmarks from RAFT.
Sanity check that benchmarks work (runs and gives reasonable recall for Deep-1M dataset)
NB: the indices built using the old ANN_BENCH in raft tend to crash in cuvs search benchmarks during index deserialization - don't forget to build the indexes anew when testing.