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

Fix groupby gtests coded in namespace cudf::test #12784

Merged
merged 4 commits into from
Feb 16, 2023

Conversation

davidwendt
Copy link
Contributor

Description

Fixes GROUPBY_TEST gtests source files coded in namespace cudf::test
Additional creates a groupby_test_util.cpp for utilities rather than inlining the utilities into all 33 source files.
The groupby tests for aggregations std and var are also not compiled for Debug builds.
This changes those to compile but be runtime disabled to make them more noticeable in a Debug test run.

Reference #11734

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. tech debt improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 15, 2023
@davidwendt davidwendt self-assigned this Feb 15, 2023
@github-actions github-actions bot added the CMake CMake build issue label Feb 15, 2023
@davidwendt
Copy link
Contributor Author

I know there are a lot of files here. They are mostly namespace changes. Here are some exceptions:

  • groupby_test_util.cpp: new file for implementing functions declared in groupy_test_util.hpp
  • groupby_test_util.hpp: definitions moved to new groupby_test_util.cpp
  • tests/gtest/replace/replace_nulls_tests.cpp: had unneeded include to groupby_test_util.hpp
  • std_tests.cpp and var_tests.cpp: tests are not disable instead of compiled out in a Debug build

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Feb 16, 2023
@davidwendt davidwendt marked this pull request as ready for review February 16, 2023 17:19
@davidwendt davidwendt requested a review from a team as a code owner February 16, 2023 17:19
@davidwendt davidwendt requested review from ttnghia and vuule February 16, 2023 17:20
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

What a PR. Were you able to automate at least some of these changes?

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 4e32bfe into rapidsai:branch-23.04 Feb 16, 2023
@davidwendt davidwendt deleted the fix-gtest-ns-groupby1 branch February 16, 2023 22:55
rapids-bot bot pushed a commit that referenced this pull request Feb 22, 2023
…ild (#12799)

Re-enable groupby with `std` and `var` aggregations that were disabled for Debug builds due to a runtime issue.
Retesting with nvcc 11.5, the error is no longer present so the code and the gtests have been re-enabled.
Found while working on PR #12784

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Mike Wilson (https://github.com/hyperbolic2346)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #12799
rapids-bot bot pushed a commit that referenced this pull request Mar 22, 2023
Remove the `using namespace cudf;` from the top of the source file to make it easier to follow. Seemed an unnecessary usage and violated the spirit of #12784

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Nghia Truong (https://github.com/ttnghia)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #12985
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 CMake CMake build issue 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