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] [2/5] Header structure: remove specialization includes #1438

Closed
wants to merge 17 commits into from
Closed

[ENH] [2/5] Header structure: remove specialization includes #1438

wants to merge 17 commits into from

Conversation

ahendriksen
Copy link
Contributor

@ahendriksen ahendriksen commented Apr 20, 2023 •
edited
Loading

Specializations are deprecated in PR #1437 . This PR removes includes of these deprecated headers.

On top of PR #1437, it adds a single commit:

  • d5b5673: Remove includes of specialization headers within RAFT.

@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 ahendriksen requested review from a team as code owners April 20, 2023 17:04
@ahendriksen ahendriksen changed the title [ENH] [0/5] Header structure: remove specialization includes [ENH] [1/5] Header structure: remove specialization includes Apr 20, 2023
@ahendriksen
Copy link
Contributor Author

Review tip: the only difference compared to PR #1437 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] [1/5] Header structure: remove specialization includes [ENH] [2/5] Header structure: remove specialization includes Apr 20, 2023
This was referenced Apr 20, 2023
Closed
Closed
@ahendriksen ahendriksen self-assigned this Apr 21, 2023
@ahendriksen ahendriksen added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 21, 2023
tfeher
tfeher approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@tfeher tfeher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviwed the last commit, looks good to me.

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.

I love this one

cjnolet
cjnolet approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, pending changes in #1437. I verified all of the existing specializations.cuh files (and specializations not in detail directories were indeed deprecated).

@ahendriksen ahendriksen mentioned this pull request Apr 27, 2023
Merged
@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
None yet
Milestone
No milestone
Development

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