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

Refactor stream mode setup for gtests #15337

Merged
merged 4 commits into from
Apr 1, 2024

Conversation

davidwendt
Copy link
Contributor

Description

Setting up the stream mode logic was duplicated in testing_main.hpp and error_handing_test.cu.
Refactoring the logic will help setup for a large strings test fixture in a follow-on PR.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 19, 2024
@davidwendt davidwendt self-assigned this Mar 19, 2024
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Mar 20, 2024
@davidwendt davidwendt marked this pull request as ready for review March 21, 2024 12:42
@davidwendt davidwendt requested a review from a team as a code owner March 21, 2024 12:42
Copy link
Contributor

@nvdbaranec nvdbaranec left a comment

Choose a reason for hiding this comment

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

One small nit.

* @param cmd_opts Command line options returned by parse_cudf_test_opts
* @return Memory resource adaptor
*/
inline auto make_stream_mode_adaptor(cxxopts::ParseResult const& cmd_opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a mild preference for actually declaring the return type here instead of using auto. Gives the caller a little more context with what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I prefer the return types as well. The type here is something created through templated RMM classes only to be passed back to an RMM function. This means RMM could change the type/name on us and would then require this code to be updated even though we don't actually rely on the type for anything--we don't make calls to it, we just pass it to set_current_device_resource. The returned object then just needs to stay alive for the life of the tests. Leaving it as auto here helps future proofs this code a bit.

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Nice.

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit e5f9e2d into rapidsai:branch-24.06 Apr 1, 2024
68 checks passed
@davidwendt davidwendt deleted the ref-stream-mode-setup branch April 1, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants