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

[ENH] [3/5] Header structure: force explicit instantiation in tests and benchmarks #1439

Conversation

ahendriksen
Copy link
Contributor

@ahendriksen ahendriksen commented Apr 20, 2023

Since PR #1437, expensive templates can be marked as RAFT_EXPLICIT to indicate they must be explicitly instantiated. This PR forces the tests and benchmarks to use these explicit instantiations.

In addition, this PR adds tests to make sure that the header files that provide the expensive templates include all necessary files when compiled under different combinations of RAFT_EXPLICIT and RAFT_EXPLICIT_INSTANTIATE_ONLY. This is done in the following steps:

1. Fix tests so that they can be built with RAFT_EXPLICIT_INSTANTIATE_ONLY defined.

Change the index type in the tests to match the instantiated index type:

Add an instance in the tests in a separate source file:

Allow creating instances in the test itself: (filed issues #1447, #1448)

2. Force explicit instantiation in the benchmarks and tests in CMake:

3. Add tests to make sure that the headers work under all prevalent combinations of RAFT_COMPILED and RAFT_EXPLICIT_INSTANTIATE_ONLY:

Instead of having the specializations in sub-directories, the
raft_runtime source files now mimic the include/ directory hierarchy.
Used .data() instead of .data_handle()
These types are not used in the ext header, but are useful to have.
Under multiple combinations of RAFT_EXPLICIT_INSTANTIATE_ONLY and RAFT_COMPILED
@ahendriksen
Copy link
Contributor Author

Reviewer tip: this PR is a follow up to PR #1438. Only the last 9 commits are part of this PR. That should reduce the diff considerably.

@ahendriksen ahendriksen changed the title [ENH] [2/5] Header structure: force explicit instantiation in tests and benchmarks [ENH] [3/5] Header structure: force explicit instantiation in tests and benchmarks Apr 20, 2023
@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
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 @ahendriksen for the PR. Looks good overall, just have one question.

cpp/test/CMakeLists.txt Show resolved Hide resolved
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.

Reviewed the new commits in this PR. Issues are created for the few outstanding discussion points. Looks good to me. Thanks Allard for the clear description of the 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 changes from #1437 and successfull run of downstream CIs (cugraph, cugraph-ops, cuopt, cuml). I verified the RAFT_COMPILED was properly removed from all the tests and benchmarks.

Co-authored-by: Divye Gala <divyegala@gmail.com>
@cjnolet cjnolet closed this Apr 28, 2023
rapids-bot bot pushed a commit that referenced this pull request Apr 28, 2023
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
Labels
Build Time Improvement CMake cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants