-
Notifications
You must be signed in to change notification settings - Fork 197
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 GPU and CPU packages for ANN benchmarks #1773
Add GPU and CPU packages for ANN benchmarks #1773
Conversation
…in the next commit
…n-ann-bench-use-gbench
build.sh
Outdated
@@ -152,7 +154,7 @@ function limitTests { | |||
# Remove the full LIMIT_TEST_TARGETS argument from list of args so that it passes validArgs function | |||
ARGS=${ARGS//--limit-tests=$LIMIT_TEST_TARGETS/} | |||
TEST_TARGETS=${LIMIT_TEST_TARGETS} | |||
echo "Limiting tests to $TEST_TARGETS" | |||
echo "Limiting tests to $TEST_TARGETS" |
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 think the original indentation was correct
ci/build_python.sh
Outdated
# Build ann-bench-cpu only in CUDA 12 jobs since it only depends on python | ||
# version | ||
if [[ ${CUDA_VERSION} == "11.8.0" ]]; then |
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.
The comment does not match the conditional. Also, I think the variable name that our CI containers use is RAPIDS_CUDA_VERSION
. Can you verify if CUDA_VERSION
is set?
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.
yes, CUDA_VERSION is set, you can see it in the logs https://github.com/rapidsai/raft/actions/runs/6051700515/job/16424916331#step:7:54 which have both with and without RAPIDS_
prefix
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.
Please use RAPIDS_CUDA_VERSION
-- but better yet, use this snippet copied from cugraph (with CUDA 12 to match the comment as @divyegala noted above):
# Build ann-bench-cpu only in CUDA 12 jobs since it only depends on python | |
# version | |
if [[ ${CUDA_VERSION} == "11.8.0" ]]; then | |
# Build ann-bench-cpu only in CUDA 12 jobs since it only depends on python | |
# version | |
RAPIDS_CUDA_MAJOR="${RAPIDS_CUDA_VERSION%%.*}" | |
if [[ ${RAPIDS_CUDA_MAJOR} == "12" ]]; then |
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 require to build in the CUDA 11 jobs for now, but otherwise will use it!
/ok to test |
/ok to test |
/ok to test |
/ok to test |
Make the `cpp/bench/ann/src/common/cuda_stub.hpp` more flexible and include it in all benchmarks (instead of only `ANN_BENCH` target). This makes the targets (benchmark libs), which define `CPU_ONLY`, on depend on CUDA headers, thus enabling the builds without GPU. This should relief #1773 of doing extra workarounds in the bench headers to achieve the same effect. Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #1792
/ok to test |
Co-authored-by: Artem M. Chirkin <[email protected]>
/ok to test |
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.
Approving ops-codeowner
file changes
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.
LGTM, long awaited feature!
/merge |
Builds on top of #1769
libraft-ann-bench
C++ based packageraft-ann-bench
packages that includes C++ tests as well as Python scriptsraft-ann-bench
package includes all tests for CPU and GPUraft-ann-bench-cpu
package that does not depend on CUDA or RAFT GPU codeSome changes include:
RAPIDS_DATASET_ROOT_DIR
env variable to indicate location of datasets (optional) consistent with other repos: https://docs.rapids.ai/maintainers/datasets/python bench/ann/run.py --dataset deep-image-96-inner
is nowpython -m raft-ann-bench.run --dataset deep-image-96-inner
, but still scripts meant to be invoked from the command line.Future improvements:
Closes #1744