Add CUDF_TEST_PROGRAM_MAIN macro to tests lacking it by etseidl · Pull Request #14751 · NVIDIA/cudf

etseidl · 2024-01-12T01:02:06Z

Description

JSON_PATH_TEST and ROW_CONVERSION_TEST were not using the CUDF_TEST_PROGRAM_MAIN, and thus were not picking up the GTEST_CUDF_RMM_MODE env variable during nightly testing.

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-pr-bot · 2024-01-12T01:02:09Z

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.

etseidl · 2024-01-12T01:02:25Z

CC @davidwendt

vyasr · 2024-01-12T01:03:13Z

/ok to test

vyasr · 2024-01-12T02:24:26Z

It looks like with the change to using a pool you may need to up the GPU percentages used for both Parquet and JSON. Maybe try 50 first and see if that works? I don't know the tests well enough to have any intuition for the data sizes.

davidwendt · 2024-01-12T13:33:29Z

It looks like with the change to using a pool you may need to up the GPU percentages used for both Parquet and JSON. Maybe try 50 first and see if that works? I don't know the tests well enough to have any intuition for the data sizes.

That is strange since neither of these two are part of those tests.
Maybe the JSON_PATH_TEST and ROW_CONVERSION_TEST need their own GPU settings instead?

etseidl · 2024-01-12T16:58:46Z

That is strange since neither of these two are part of those tests.
Maybe the JSON_PATH_TEST and ROW_CONVERSION_TEST need their own GPU settings instead?

Did some local profiling, and it looks like ROW_CONVERSION_TEST blows up the memory pool right at the end (the last 1 or two tests it runs). I see that ROW_CONVERSION_TEST ends just after the two tests that have the memory error. If I run RCT with the cuda mr, I don't see the mem explosion. I'll have to look at the ManyStrings test...maybe it's fragmenting the pool memory?

davidwendt · 2024-01-12T18:00:16Z

That is strange since neither of these two are part of those tests.
Maybe the JSON_PATH_TEST and ROW_CONVERSION_TEST need their own GPU settings instead?

Did some local profiling, and it looks like ROW_CONVERSION_TEST blows up the memory pool right at the end (the last 1 or two tests it runs). I see that ROW_CONVERSION_TEST ends just after the two tests that have the memory error. If I run RCT with the cuda mr, I don't see the mem explosion. I'll have to look at the ManyStrings test...maybe it's fragmenting the pool memory?

The ROW_CONVERSION_TEST was recently moved from the JNI source tree to libcudf. My understanding is the move is temporary until the build team can figure out some CUDA 12 build issues within Spark. Given that, I would be ok with changes here to make it use less resources (and run faster as well).

etseidl · 2024-01-12T18:11:46Z

There are a bunch of things going on here. One is that the random number generator isn't seeded, so the order of tests, or selecting a subset, can lead to different results. For instance, on my machine, if I run just the RowToColumnTests suite, I get an OOM error in ManyStrings regardless of which memory resource I use. If I run the full set of tests, or just ManyStrings, no OOM. If I add a srand() call, then no OOM.

But the real culprit is the RowToColumnTests.Biggest test which starts the explosive growth of the pooled MR.

The ROW_CONVERSION_TEST was recently moved from the JNI source tree to libcudf. My understanding is the move is temporary until the build team can figure out some CUDA 12 build issues within Spark. Given that, I would be ok with changes here to make it use less resources (and run faster as well).

Well, if that's the case should we just revert to the default MR for that test? Or modify the tests to use less RAM?

davidwendt · 2024-01-12T18:14:01Z

Well, if that's the case should we just revert to the default MR for that test? Or modify the tests to use less RAM?

I'd prefer to modify the tests to use less RAM. How feasible is that?

vyasr · 2024-01-12T18:19:28Z

That is strange since neither of these two are part of those tests. Maybe the JSON_PATH_TEST and ROW_CONVERSION_TEST need their own GPU settings instead?

It could be (and sounds like) the case that the tests changed in this PR are now using up enough memory that other tests (the ones I mentioned) are failing. So yes, we could change the setting for those. OTOH if reducing the memory footprint of the ROW_CONVERSION_TEST is feasible that sounds even better.

etseidl · 2024-01-12T18:22:58Z

@ttnghia can you take a look and see if it's ok to reduce the number of rows or columns in the big string tests?

etseidl · 2024-01-12T19:15:51Z

I reduced the row counts for two tests, and now the entire test stays within 6GB (when starting with a 3GB allocation).

ttnghia · 2024-01-12T20:17:46Z

@ttnghia can you take a look and see if it's ok to reduce the number of rows or columns in the big string tests?

I'm not sure about this. @hyperbolic2346 what do you think?

davidwendt · 2024-01-18T01:03:29Z

/ok to test

hyperbolic2346 · 2024-01-18T07:18:10Z


 TEST_F(RowToColumnTests, ManyStrings)
 {
+  // this test is very sensitive, so seed the random number generator


Sensitive in what way?

Prone to blowing up if too many instances of the largest string are used. Seeding the RNG prevents that.

@hyperbolic2346 did you want a change to the comment here?

It would be nice. If I have the question others will as well.

davidwendt · 2024-01-19T19:20:42Z

/ok to test

davidwendt · 2024-01-19T20:41:20Z

/ok to test

hyperbolic2346 · 2024-01-22T17:45:50Z

@ttnghia can you take a look and see if it's ok to reduce the number of rows or columns in the big string tests?

I'm not sure about this. @hyperbolic2346 what do you think?

I think it is fine. The test sizes are somewhat arbitrarily chosen. It needs to be "large enough" to uncover bugs. Given this is temporary and 3 gigs is most likely large enough I am fine with the change.

hyperbolic2346

Thanks!

davidwendt · 2024-01-22T18:30:08Z

/ok to test

davidwendt · 2024-01-22T21:55:52Z

/merge

etseidl added 2 commits January 11, 2024 16:52

add CUDF_TEST_PROGRAM_MAIN()

8928087

Merge remote-tracking branch 'origin/branch-24.02' into gtest_mains

e220260

etseidl requested a review from a team as a code owner January 12, 2024 01:02

etseidl requested review from harrism and hyperbolic2346 January 12, 2024 01:02

github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Jan 12, 2024

vyasr added bug Something isn't working non-breaking Non-breaking change labels Jan 12, 2024

davidwendt approved these changes Jan 12, 2024

View reviewed changes

etseidl and others added 2 commits January 12, 2024 11:13

reduce memory footprint a bit, clean up some comments/literals

7a7a0aa

Merge branch 'branch-24.02' into gtest_mains

4eeea30

properly cap off huge string

2739a2b

etseidl commented Jan 12, 2024

View reviewed changes

Comment thread cpp/tests/transform/row_conversion.cpp

etseidl added 2 commits January 16, 2024 14:14

Merge branch 'branch-24.02' into gtest_mains

b2a1435

Merge branch 'branch-24.02' into gtest_mains

38e0a0f

hyperbolic2346 reviewed Jan 18, 2024

View reviewed changes

Merge remote-tracking branch 'origin/branch-24.02' into gtest_mains

6032d91

finish merge

690ae38

davidwendt requested a review from hyperbolic2346 January 22, 2024 16:03

etseidl and others added 2 commits January 22, 2024 09:47

add clarification

9b4fdd5

Merge branch 'branch-24.02' into gtest_mains

578329f

hyperbolic2346 approved these changes Jan 22, 2024

View reviewed changes

rapids-bot Bot merged commit b1468a5 into NVIDIA:branch-24.02 Jan 22, 2024

etseidl deleted the gtest_mains branch January 22, 2024 23:08

Uh oh!

Conversation

etseidl commented Jan 12, 2024

Description

Checklist

Uh oh!

copy-pr-bot Bot commented Jan 12, 2024

Uh oh!

etseidl commented Jan 12, 2024

Uh oh!

vyasr commented Jan 12, 2024

Uh oh!

vyasr commented Jan 12, 2024

Uh oh!

davidwendt commented Jan 12, 2024

Uh oh!

etseidl commented Jan 12, 2024

Uh oh!

davidwendt commented Jan 12, 2024

Uh oh!

etseidl commented Jan 12, 2024

Uh oh!

davidwendt commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vyasr commented Jan 12, 2024

Uh oh!

etseidl commented Jan 12, 2024

Uh oh!

etseidl commented Jan 12, 2024

Uh oh!

ttnghia commented Jan 12, 2024

Uh oh!

Uh oh!

davidwendt commented Jan 18, 2024

Uh oh!

hyperbolic2346 Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

etseidl Jan 18, 2024

Choose a reason for hiding this comment

Uh oh!

etseidl Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

hyperbolic2346 Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

davidwendt commented Jan 19, 2024

Uh oh!

davidwendt commented Jan 19, 2024

Uh oh!

hyperbolic2346 commented Jan 22, 2024

Uh oh!

hyperbolic2346 left a comment

Choose a reason for hiding this comment

Uh oh!

davidwendt commented Jan 22, 2024

Uh oh!

davidwendt commented Jan 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidwendt commented Jan 12, 2024 •

edited

Loading