Skip to content
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

Select k instantiations #159

Merged
merged 8 commits into from
May 30, 2024

Conversation

benfred
Copy link
Member

@benfred benfred commented May 27, 2024

Since we're now using raft in header only mode, we don't have the compiled select_k instantiations in raft available to us anymore. Instead instantiate inside cuvs so we don't recompile in multiple spots.

Since we're now using raft in header only mode, we don't have the
compiled select_k instantiations in raft available to us anymore. Instead
instantiate inside cuvs so we don't recompile in multiple spots.
@benfred benfred added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 27, 2024
@benfred benfred marked this pull request as ready for review May 28, 2024 16:11
@benfred benfred requested review from a team as code owners May 28, 2024 16:11
@benfred benfred requested a review from a team as a code owner May 28, 2024 22:31
* @param[in] len_i
* optional array of size (batch_size) providing lengths for each individual row
*/
template <typename T, typename IdxT>
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this earlier- we don't want templates in the public APIs for the computational functions. (Really we don't want them for anything, but the classes/structs that don't end up calling CUDA kernels are impacted much less.

We want to make sure only the wrappers for all concrete types are exposed here and the template function is exposed as a cuh file in src/, which the source file includes and invokes.

Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Apart from the issue mentioned by Corey, the PR looks good to me.

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

I think at some point we might want to consider a params struct for the options just to make them easier to use, but for now I think the direct wrap is fine.

@cjnolet
Copy link
Member

cjnolet commented May 29, 2024

/merge

@rapids-bot rapids-bot bot merged commit 758d147 into rapidsai:branch-24.06 May 30, 2024
54 checks passed
difyrrwrzd added a commit to difyrrwrzd/cuvs that referenced this pull request Aug 10, 2024
Since we're now using raft in header only mode, we don't have the compiled select_k instantiations in raft available to us anymore. Instead instantiate inside cuvs so we don't recompile in multiple spots.

Authors:
  - Ben Frederickson (https://github.com/benfred)

Approvers:
  - Tamas Bela Feher (https://github.com/tfeher)
  - Corey J. Nolet (https://github.com/cjnolet)

URL: rapidsai/cuvs#159
@benfred benfred deleted the select_k_instantiations branch September 5, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
Development

Successfully merging this pull request may close these issues.

3 participants