-
Notifications
You must be signed in to change notification settings - Fork 75
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
WIP: [DO NOT MERGE] introduce libcuvs wheels #594
base: branch-25.02
Are you sure you want to change the base?
Conversation
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. |
/ok to test |
/ok to test |
branch: ${{ inputs.branch }} | ||
sha: ${{ inputs.sha }} | ||
date: ${{ inputs.date }} | ||
package-name: cuvs |
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.
package-name: cuvs | |
package-name: libcuvs |
@@ -14,5 +14,12 @@ rapids-dependency-file-generator \ | |||
rapids-mamba-retry env create --yes -f env.yaml -n checks | |||
conda activate checks | |||
|
|||
# get config for cmake-format checks |
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 diff from this change doesn't appear to be too large, so I am okay with including it in this PR.
@@ -44,8 +44,9 @@ echo "${NEXT_FULL_TAG}" > VERSION | |||
DEPENDENCIES=( | |||
dask-cuda | |||
cuvs | |||
pylibraft | |||
libraft |
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.
libraft | |
libcuvs | |
libraft |
[project.entry-points."cmake.prefix"] | ||
libcuvs = "libcuvs" | ||
|
||
[tool.isort] |
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.
Do we need all the isort settings here?
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 yes. It could probably be reduced (I'll do that) but at least the formatting options I think should stay.
Given that this project seems to:
- not set any defaults for
isort
in itspyproject.toml
- prefer to have sub-directory-specific configuration for
isort
Lines 8 to 10 in f1de1b2
# Use the config file specific to each subproject so that each | |
# project can specify its own first/third-party packages. | |
args: ["--config-root=python/", "--resolve-all-configs"] |
endif() | ||
|
||
# --- RAFT ---# | ||
set(CUVS_USE_RAFT_STATIC 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.
One of the wheel-test
jobs (CUDA 11.8, arm64) is failing, with a few cases like this:
FAILED python/cuvs/cuvs/test/test_ivf_pq.py::test_ivf_pq_search_params[params2] - cuvs.common.exceptions.CuvsException: cuBLAS error encountered at: file=/pyenv/versions/3.10.16/lib/python3.10/site-packages/libraft/include/raft/linalg/detail/cublaslt_wrappers.hpp line=261: call='cublasLtMatmul(resource::get_cublaslt_handle(res), mm_desc->desc, alpha, a_ptr, mm_desc->a, b_ptr, mm_desc->b, beta, c_ptr, mm_desc->c, c_ptr, mm_desc->c, &(mm_desc->heuristics.algo), nullptr, 0, stream)', Reason=13:CUBLAS_STATUS_EXECUTION_FAILED
Obtained 49 stack frames
#1 in /pyenv/versions/3.12.8/lib/python3.12/site-packages/libcuvs/lib64/libcuvs.so: raft::cublas_error::cublas_error(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) +0xb0 [0xfffdca6a5360]
#2 in /pyenv/versions/3.12.8/lib/python3.12/site-packages/libcuvs/lib64/libcuvs.so: void raft::linalg::detail::legacy_matmul<false, float, float, float, float>(raft::resources const&, bool, bool, unsigned long, unsigned long, unsigned long, float const*, float const*, unsigned long, float const*, unsigned long, float const*, float*, unsigned long, CUstream_st*) +0x6c0 [0xfffdca6c4900]
All the failures are in one set of test cases:
FAILED python/cuvs/cuvs/test/test_ivf_pq.py
I don't think those are related to this PR... I see them on other PRs like #593 (which only remove whitespace):
https://github.com/rapidsai/cuvs/actions/runs/12875298461/job/35896833763?pr=593
SKBUILD_EXTRA_CMAKE_ARGS="${EXTRA_CMAKE_ARGS}" | ||
if [[ "${EXTRA_CMAKE_ARGS}" != *"DFIND_CUVS_CPP"* ]]; then | ||
SKBUILD_EXTRA_CMAKE_ARGS="${SKBUILD_EXTRA_CMAKE_ARGS};-DFIND_CUVS_CPP=ON" | ||
fi |
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.
Just picking a mostly-arbitrary place on the diff to start this thread... the rust
build is failing here.
warning: argument unused during compilation: '-L/opt/conda/envs/rust/targets/x86_64-linux/lib' [-Wunused-command-line-argument]
warning: argument unused during compilation: '-L/opt/conda/envs/rust/targets/x86_64-linux/lib/stubs' [-Wunused-command-line-argument]
/usr/include/limits.h:124:16: fatal error: 'limits.h' file not found
clang diag: warning: argument unused during compilation: '-L/opt/conda/envs/rust/targets/x86_64-linux/lib' [-Wunused-command-line-argument]
clang diag: warning: argument unused during compilation: '-L/opt/conda/envs/rust/targets/x86_64-linux/lib/stubs' [-Wunused-command-line-argument]
thread 'main' panicked at cuvs-sys/build.rs:97:10:
Unable to generate cagra_c bindings: ClangDiagnostic("/usr/include/limits.h:124:16: fatal error: 'limits.h' file not found\n")
I strongly suspect it's related to this PR, because I've seen it on multiple runs and because that build is succeeding on branch builds and other PRs. For example, it just passed 6 minutes ago on #596: https://github.com/rapidsai/cuvs/actions/runs/12895689316/job/35960708413?pr=596
Contributes to rapidsai/build-planning#33.
Proposes packaging
libcuvs
as a wheel, which is then re-used bycuvs-cu{11,12}
.Similar changes were recently made in RAFT: rapidsai/raft#2531
As part of this, also proposes:
CUVS_COMPILE_DYNAMIC_ONLY
, to allow building/installing only the dynamic shared library (i.e. skipping the static library)rapids-cmake
's preferred CMake style (similar introduce libraft wheels raft#2531 (comment))Notes for Reviewers
if you see this note, this is not yet ready for review
Benefits of these changes
Wheel contents
libcuvs
:libcuvs.so
andlibcuvs_c.so
(shared library)cuvs
:cuvs
Python / Cython code and compiled Cython extensionsSize changes (CUDA 12, Python 3.12, x86_64)
| wheel | num files (before) | num files (this PR) | size (before) | size (this PR) |
|:------------- -:|------------------:|-----------------:|--------------:|-------------:|
|
libcuvs
| --- | --- | --- | --- ||
cuvs
| --- | --- | --- | --- ||TOTAL | --- | --- | --- | --- |
How I tested this