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

ivf-pq post integration hotfixes #878

Merged

Conversation

achirkin
Copy link
Contributor

@achirkin achirkin commented Oct 4, 2022

Fixes to ivf_pq::search:

  1. Fixed a typo in argument name of select_cluster (n_queries -> queries_batch) which led to illegal memory access for large batches.
  2. Removed unnecessary argument max_batch_size from the worker function.
  3. Replaced <<<bracket-calling>>> of dynamically selected kernels with cudaLaunchKernel invocations.
    The former led to the kernels silently being not called at all in some cases for no apparent reason (not reproducible in tests).

Fixes to ivf_pq::build:

  1. A missing include raft/util/device_atomics.cuh passed unnoticed due to transient dependencies.

Three fixes to ivf_pq::search:

  1. Type in argument name of select_cluster (n_queries -> queries_batch) which lead to illegal memory access for large batches
  2. Removed unnecessary argument `max_batch_size` from the worker function.
  3. Replaces <<<bracket-calling>>> of dynamically selected kernels with cudaLaunchKernel invocations.
     The former led to the kernels silently being not called at all in some cases for no apparent reason (not reproducible in tests).
@achirkin achirkin requested a review from a team as a code owner October 4, 2022 07:59
@github-actions github-actions bot added the cpp label Oct 4, 2022
@achirkin achirkin added bug Something isn't working 3 - Ready for Review non-breaking Non-breaking change labels Oct 4, 2022
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.

Thanks Artem for the fix! It looks good to me.

@achirkin achirkin requested a review from cjnolet October 4, 2022 13:33
@cjnolet
Copy link
Member

cjnolet commented Oct 4, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e7bf57c into rapidsai:branch-22.10 Oct 4, 2022
@achirkin achirkin deleted the fix-ivf-pq-search-post-integration branch October 4, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review bug Something isn't working cpp non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants