-
Notifications
You must be signed in to change notification settings - Fork 547
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 libcuml wheels #6199
base: branch-25.02
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
/ok to test |
# --- cumlprims_mg --- # | ||
# ship cumlprims_mg in the 'libcuml' wheel (for re-use by 'cuml' wheels) | ||
set(CUML_USE_CUMLPRIMS_MG_STATIC OFF) | ||
set(CUML_EXCLUDE_CUMLPRIMS_MG_FROM_ALL 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.
Proposing bundling libcumlprims_mg.so
+ its headers into libcuml
wheels:
- so
libcuml++.so
and the Cython extensions incuml
share 1 instance of this library - to avoid needing to compile cumlprims_mg for both
libcuml
andcuml
wheel builds
For conda packages, this isn't necessary because we distributed separate libcumlprims_mg
conda packages that can be dynamically linked to.
We don't have the equivalent wheels and I don't think they'd be worth doing (given that they're only needed for cuML).
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.
This plan seems fine to me. It reduces binary size and compile time, and provides the same behavior we already have for conda packages (except this is one wheel, rather than two conda packages).
# --- cumlprims_mg --- # | ||
# ship cumlprims_mg in the 'libcuml' wheel (for re-use by 'cuml' wheels) | ||
set(CUML_USE_CUMLPRIMS_MG_STATIC OFF) | ||
set(CUML_EXCLUDE_CUMLPRIMS_MG_FROM_ALL 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.
This plan seems fine to me. It reduces binary size and compile time, and provides the same behavior we already have for conda packages (except this is one wheel, rather than two conda packages).
Returns ``None`` if the library cannot be loaded. | ||
""" | ||
# cumlprims_mg installs to lib/ |
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.
Is this a bug in cumlprims_mg
's CMake?
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. Yes it is.
Let's wait for https://github.com/rapidsai/cumlprims_mg/pull/223 to merge, and then simplify this logic to use lib64/
.
Co-authored-by: Bradley Dice <[email protected]>
Replaces #6006, contributes to rapidsai/build-planning#33.
Proposes packaging
libcuml
as a wheel, which is then re-used bycuml-cu{11,12}
wheels.Blocked by rapidsai/cuvs#594
Notes for Reviewers
Benefits of these changes
Wheel contents
libcuml
:libcuml++.so
(shared library) and its headerslibcumlprims_mg.so
(shared library) and its headersfmt
)cuml
:cuml
Python / Cython code and compiled Cython extensionsDependency Flows
In short....
libcuml
containslibcuml.so
andlibcumlprims_mg.so
dynamic libraries and the headers to link against them.libcugraph
wheels as a build dependency.libcuml.load_library()
.For more details and some flowcharts, see rapidsai/build-planning#33 (comment)
Size changes (CUDA 12, Python 3.12, x86_64)
libcuml
cuml
NOTES: size = compressed, "before" = 2025-01-22 nightlies
how I calculated those (click me)
How I tested this
These other PRs: