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.

Sign up for GitHub

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

Jump to bottom

[ENH] [4/5] Header structure: split search of ivf methods #1440

Closed
wants to merge 28 commits into from
Closed

[ENH] [4/5] Header structure: split search of ivf methods #1440

wants to merge 28 commits into from

Conversation

ahendriksen
Copy link
Contributor

@ahendriksen ahendriksen commented Apr 20, 2023 •
edited
Loading

This PR splits the ivf-pq and ivf-flat search methods. It is a follow up to PR #1439.

This PR:

  • Halves the number of instances of interleaved_scan_kernel, by instantiating only for veclen=1 or veclen=16/sizeof(DataT).
  • Extracts the interleaved_scan_kernel code from the ivf_flat_search.cuh header
  • Extracts the compute_similarity_kernel code from ivf_pq_search.cuh header
  • Makes the template parameters of IVF-flat consistent

@ahendriksen
MV: add -inl suffix to header paths
d9801e8
@ahendriksen
MV: raft_runtime src files
50b374d 
Instead of having the specializations in sub-directories, the
raft_runtime source files now mimic the include/ directory hierarchy.
@ahendriksen
FIX: add missing includes
8974ae3
@ahendriksen
FIX: getWorkspaceSize
95ef31b 
Used .data() instead of .data_handle()
@ahendriksen
PREP: Separate rbf_fin_op
7edcb6a
@ahendriksen
PREP: registers: Add _types header
71de7bd
@ahendriksen
Change RAFT_COMPILED from INTERFACE to PUBLIC
541cabc
@ahendriksen
Define RAFT_EXPLICIT and RAFT_EXPLICIT_INSTANTIATE_ONLY
d81b14e
@ahendriksen
Update docs
48ea769
@ahendriksen
Replace specializations by split headers
e6bb5d5
@ahendriksen
Deprecate specialization headers
ff79abf
@ahendriksen
Add interleaved scan instances
c9e7413
@ahendriksen
Separate fused_l2_nn_helpers
0c889dc 
These types are not used in the ext header, but are useful to have.
@ahendriksen
Remove pairwise_matrix_instantiation_point
f97b2a8
@ahendriksen
Rename specialization => instantiation
fb637f7
@ahendriksen
test/neighbors/selection.cu: Expose kFaissMaxK
7b065af
@ahendriksen
Remove includes of specialization headers
d5b5673
@ahendriksen
test/distance/dist_adj.cu: Add instance
94d8117
@ahendriksen
test/cluster/linkage.cu: Allow instance
361570b
@ahendriksen
test/sparse/neighbors/connect_components.cu: Allow instance
5171de3
@ahendriksen
test/neighbors/ann_ivf_pq/test_float_uint32_t.cu: Allow instance
4426c50
@ahendriksen
test/matrix/select_k.cu: Change index type
e527efc
@ahendriksen
test/neighbors/fused_l2_knn.cu: Change index type
17902e9
@ahendriksen
Force explicit instantiations in tests
976189b
@ahendriksen
Force explicit instantiations in benchmarks
bdae61d
@ahendriksen
Test that headers are free standing
b0b8fe5 
Under multiple combinations of RAFT_EXPLICIT_INSTANTIATE_ONLY and RAFT_COMPILED
@ahendriksen
Split ivf-pq and ivf-flat search
7d1aa77 
The compute_similarity and interleaved_scan kernel are quite expensive
to compile. Splitting the headers in this commit.
@ahendriksen ahendriksen requested review from a team as code owners April 20, 2023 17:46
@github-actions github-actions bot added the CMake label Apr 20, 2023
@github-actions github-actions bot added the cpp label Apr 20, 2023
@ahendriksen
Copy link
Contributor Author

Review tip: the only difference compared to PR #1439 is the last commit. The full diff wrt branch-23.06 includes PR #1437, so it is a bit much.

@ahendriksen ahendriksen mentioned this pull request Apr 20, 2023
Closed
@ahendriksen ahendriksen changed the title [ENH] [3/5] Header structure: split search of ivf methods [ENH] [4/5] Header structure: split search of ivf methods Apr 20, 2023
This was referenced Apr 20, 2023
Closed
Closed
@ahendriksen ahendriksen added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Build Time Improvement labels Apr 21, 2023
@ahendriksen ahendriksen self-assigned this Apr 21, 2023
tfeher
tfeher approved these changes Apr 26, 2023
View reviewed changes
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 Allard, I have just one question for clarification, otherwise the PR looks good to me.

cpp/include/raft/neighbors/detail/ivf_flat_interleaved_scan-inl.cuh Show resolved Hide resolved
cpp/include/raft/neighbors/detail/ivf_flat_search-ext.cuh Outdated Show resolved Hide resolved
cpp/include/raft/neighbors/detail/ivf_pq_compute_similarity-inl.cuh Show resolved Hide resolved
cpp/include/raft/neighbors/detail/ivf_pq_compute_similarity-inl.cuh Show resolved Hide resolved
divyegala
divyegala approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@divyegala divyegala left a comment

Choose a reason for hiding this comment

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

Just one comment about expanding documentation, but it doesn't have to be a part of this PR!

cjnolet
cjnolet approved these changes Apr 26, 2023
View reviewed changes
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.

LGTM pending requested changes in #1437

This was referenced Apr 27, 2023
Closed
Merged
@tfeher tfeher mentioned this pull request Apr 27, 2023
Open
@cjnolet cjnolet closed this Apr 28, 2023
rapids-bot bot pushed a commit that referenced this pull request Apr 28, 2023
@ahendriksen
[ENH] [FINAL] Header structure: combine all PRs into one (#1469)
fbce1a4 
This is a rebase of all the commits in PRs:
- #1437 
- #1438 
- #1439 
- #1440 
- #1441 

The original PRs have not been rebased to preserve review comments. This PR is up to date with branch 23.06.

Closes #1416

Authors:
  - Allard Hendriksen (https://github.com/ahendriksen)

Approvers:
  - Divye Gala (https://github.com/divyegala)
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #1469
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divyegala divyegala divyegala approved these changes

@tfeher tfeher tfeher approved these changes

@cjnolet cjnolet cjnolet approved these changes

Assignees

@ahendriksen ahendriksen

Labels
Build Time Improvement CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
VS/ML/DM Primitives Release Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ahendriksen @cjnolet @tfeher @divyegala