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

Split parquet test into multiple files #14663

Merged
merged 37 commits into from
Jan 9, 2024

Conversation

etseidl
Copy link
Contributor

@etseidl etseidl commented Dec 20, 2023

Description

Compiling the parquet C++ tests takes a long time. This work attempts to address this by splitting the (mostly) monolithic parquet_test.cpp into multiple compilation units. On my workstation (10 core i7) I was able to reduce the compile time for PARQUET_TEST from 48s down to 23s.

This PR also splits the testing base_fixture.hpp file into three parts to cut down on unnecessary rmm includes. This change resulted in a time savings of around 20% when compiling the entire test suite locally.

Checklist

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

Copy link

copy-pr-bot bot commented Dec 20, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Dec 20, 2023
@etseidl
Copy link
Contributor Author

etseidl commented Dec 21, 2023

Latest changes get the compile time down to 19s.

@vuule vuule added tests Unit testing for project improvement Improvement / enhancement to an existing function non-breaking Non-breaking change cuIO cuIO issue labels Dec 26, 2023
@vuule
Copy link
Contributor

vuule commented Dec 26, 2023

/ok to test

@etseidl
Copy link
Contributor Author

etseidl commented Jan 2, 2024

A bit of scope creep...while trying to figure out where compile time was going, I found that including base_fixture.hpp actually added a significant chunk of time. I split that file into two, moving parts associated with defining main to a new file testing_main.hpp. This reduced the overall compile time for the full test suite from around 10.5 min to just over 7.5 min.

@vuule
Copy link
Contributor

vuule commented Jan 3, 2024

/ok to test

@vuule
Copy link
Contributor

vuule commented Jan 4, 2024

I'll measure the build time locally (tomorrow) to help confirm that this change does reduce the total build time.

@vuule
Copy link
Contributor

vuule commented Jan 4, 2024

/ok to test

@vuule
Copy link
Contributor

vuule commented Jan 4, 2024

The new quote_normalization_test file needs to be updated, got an error building locally.

@davidwendt
Copy link
Contributor

/ok to test

@etseidl etseidl marked this pull request as ready for review January 5, 2024 22:51
@etseidl etseidl requested a review from a team as a code owner January 5, 2024 22:51
@etseidl etseidl requested a review from vyasr January 5, 2024 22:51
@vuule
Copy link
Contributor

vuule commented Jan 9, 2024

/ok to test

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.

90 files. 8K lines. All solid :D

@vuule
Copy link
Contributor

vuule commented Jan 9, 2024

/merge

@rapids-bot rapids-bot bot merged commit 856c985 into rapidsai:branch-24.02 Jan 9, 2024
67 checks passed
@etseidl etseidl deleted the test_parquet_refactor branch January 9, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue cuIO cuIO issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change tests Unit testing for project
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants