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

Internal library to share headers between test and bench #1162

Merged
merged 7 commits into from
Jan 26, 2023

Conversation

achirkin
Copy link
Contributor

Add a new component to raft for headers, which are used in both tests and benchmarks.

Closes #1153

@achirkin achirkin added feature request New feature or request non-breaking Non-breaking change 2 - In Progress Currenty a work in progress labels Jan 23, 2023
@achirkin
Copy link
Contributor Author

NB: this PR depend on and includes all changes in #1085

# =============================================================================

if(BUILD_TESTS OR BUILD_BENCH)
add_library(raft_internal INTERFACE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm particularly not sure and welcome any suggestions regarding the naming of the component. Currently:

  1. The name is raft_internal, because I wanted to call it internal, but it may be a reserved word in cmake.
  2. All headers are placed into double-nested internal/internal. The first internal is for cmake to isolate it from the other components, the second is for includes, so that all includes from this component are easily identifiable, e.g.:
#include <internal/neighbors/naive_knn.cuh>

Copy link
Member

Choose a reason for hiding this comment

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

Thinking through this a little bit- what do you think about keeping the outer directory cpp/internal as-is but nesting everything inside cpp/internal/raft instead of cpp/internal/internal. This would effectively allows us to include the cpp/internal directory in the build for the bench and test source files whilst making the include paths themselves look the same as the other raft includes. So for example, this would still allow us to include the naive_knn.cuh file as #include <raft/neighbors/naive_knn.cuh> but the file would be coming from cpp/internal instead. This assumes we wouldn't have name collisions there, which I highly doubt we would.

Another option could be to do something like cpp/internal/raft/internal but I kind of prefer the option above more. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

The reason I'd prefer to have raft in the name is so that we don't have to worry about potential header collisions if, for example, a package named internal is ever shipped for linux. Separating the namespace by including raft makes it very clear where our scope ends.

Copy link
Contributor Author

@achirkin achirkin Jan 24, 2023

Choose a reason for hiding this comment

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

Actually, it turns out we have name collisions already! In branch-23.02 we have include/raft/matrix/select_k.cuh and its corresponding test test/raft/matrix/select_k.cuh; the latter is included using quotes syntax, so it's not a problem so far. In this PR I moved it to the internal component and changed the include syntax, so it would be a problem if we change the folder name to raft. So I see two options:

  1. rename test/raft/matrix/select_k.cuh to something like select_k_helpers.cuh or select_k_utils.cuh
  2. rename the root folder internal to something different from raft, yet not potentially clashing with third-party libraries.

In the new commit I opted for the option (2) with raft_internal as an experiment. Though I don't have a strong preference in this question. I only think that the trailing internal in the folder structure is not the best option, because it feels somewhat harder to maintain.

Copy link
Member

Choose a reason for hiding this comment

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

I think I can live w/ raft_internal in the meantime. We can always revisit if we find this is presenting maintenance challenges.

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2023

Codecov Report

Base: 87.99% // Head: 87.99% // No change to project coverage 👍

Coverage data is based on head (5cbd086) compared to base (7c12b1e).
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff              @@
##           branch-23.02    #1162   +/-   ##
=============================================
  Coverage         87.99%   87.99%           
=============================================
  Files                21       21           
  Lines               483      483           
=============================================
  Hits                425      425           
  Misses               58       58           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@achirkin achirkin marked this pull request as ready for review January 24, 2023 08:34
@achirkin achirkin requested review from a team as code owners January 24, 2023 08:34
@achirkin achirkin added 3 - Ready for Review and removed 2 - In Progress Currenty a work in progress labels Jan 24, 2023
@tfeher tfeher mentioned this pull request Jan 24, 2023
@cjnolet
Copy link
Member

cjnolet commented Jan 24, 2023

@achirkin I think this PR looks good. Can you also add a short description of this new directory to the bottom of the README.md in the root of the repository?

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

@cjnolet
Copy link
Member

cjnolet commented Jan 26, 2023

/merge

@rapids-bot rapids-bot bot merged commit 92820d5 into rapidsai:branch-23.02 Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

[ENH] Remove gtest dependency from benchmark
3 participants