Skip to content

Moving dice distance from raft to cuvs and support for half types #971

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

Open
wants to merge 14 commits into
base: branch-25.08
Choose a base branch
from

Conversation

aamijar
Copy link
Member

@aamijar aamijar commented Jun 3, 2025

Resolves #966

Copy link

copy-pr-bot bot commented Jun 3, 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.

@aamijar aamijar self-assigned this Jun 3, 2025
@aamijar aamijar added non-breaking Introduces a non-breaking change feature request New feature or request labels Jun 3, 2025
@aamijar aamijar marked this pull request as ready for review June 3, 2025 02:31
@aamijar aamijar requested review from a team as code owners June 3, 2025 02:31
@aamijar
Copy link
Member Author

aamijar commented Jun 3, 2025

Adding @rhdong as a reviewer since he worked on half types for the other distances #225

@cjnolet
Copy link
Member

cjnolet commented Jun 3, 2025

@aamijar I know this isn't your fault, but we're closely monitoring the cuVS binary size going forward and the pairwise distance APIs are by far the largest offender. Can you check the metrics logs that are produced in this PR to see how much this new distance affects the libcuvs binary size?

@aamijar
Copy link
Member Author

aamijar commented Jun 4, 2025

Hi @cjnolet, sure where can I check the binary size/metrics for this pr?

@bdice
Copy link
Contributor

bdice commented Jun 5, 2025

@aamijar See the conda C++ build logs for the build metrics reports: https://github.com/rapidsai/cuvs/actions/runs/15453622193/job/43501422197#step:14:31

The HTML report here (https://downloads.rapids.ai/ci/cuvs/pull-request/971/59cb596/cuda12_x86_64.compile_lib.html) shows some data for the new files:

CMakeFiles/cuvs_objs.dir/src/distance/detail/pairwise_matrix/dispatch_dice_float_float_float_int.cu.o	8.331 s	3.280 MB
CMakeFiles/cuvs_objs.dir/src/distance/detail/pairwise_matrix/dispatch_dice_half_float_float_int.cu.o	8.622 s	2.857 MB
CMakeFiles/cuvs_objs.dir/src/distance/detail/pairwise_matrix/dispatch_dice_double_double_double_int.cu.o	8.796 s	1.106 MB

So at least 7 MB just from the new files. There may be other changes that affect the binary size of existing objects too. Maybe distance.cu?

CMakeFiles/cuvs_objs.dir/src/distance/distance.cu.o	13.587 s	6.872 MB

@aamijar
Copy link
Member Author

aamijar commented Jun 5, 2025

Thank you @bdice! It would be nice if there was a way to highlight in the html report which files are edited from the PR and the increase or decrease in binary size.

Copy link
Member

@rhdong rhdong left a comment

Choose a reason for hiding this comment

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

LGTM

@divyegala
Copy link
Member

@aamijar I suspect the binary size is higher because we are using cutlass. @cjnolet given that this will be an infrequently used distance type, can we skip using cutlass and just use our normal pairwise kernel?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp feature request New feature or request non-breaking Introduces a non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

[FEA] Move dice dense distance from raft and support for half types
5 participants