-
Notifications
You must be signed in to change notification settings - Fork 891
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
Add CUDF_TEST_PROGRAM_MAIN macro to tests lacking it #14751
Conversation
CC @davidwendt |
/ok to test |
That is strange since neither of these two are part of those tests. |
Did some local profiling, and it looks like |
The |
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 But the real culprit is the
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? |
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 |
@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 reduced the row counts for two tests, and now the entire test stays within 6GB (when starting with a 3GB allocation). |
I'm not sure about this. @hyperbolic2346 what do you think? |
/ok to test |
@@ -934,6 +935,8 @@ TEST_F(RowToColumnTests, BigStrings) | |||
|
|||
TEST_F(RowToColumnTests, ManyStrings) | |||
{ | |||
// this test is very sensitive, so seed the random number generator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sensitive in what way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prone to blowing up if too many instances of the largest string are used. Seeding the RNG prevents that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hyperbolic2346 did you want a change to the comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice. If I have the question others will as well.
/ok to test |
/ok to test |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/ok to test |
/merge |
Description
JSON_PATH_TEST
andROW_CONVERSION_TEST
were not using theCUDF_TEST_PROGRAM_MAIN
, and thus were not picking up theGTEST_CUDF_RMM_MODE
env variable during nightly testing.Checklist