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 some test issues in Spark UT and keep RapidsTestSettings update-to-date #10997

Merged
merged 18 commits into from
Jun 27, 2024

Conversation

thirtiseven
Copy link
Collaborator

@thirtiseven thirtiseven commented Jun 7, 2024

Contributes to #10771 #10773 #10775

This PR:

  • Updated issue links in Spark UT Test settings to specific ones.
  • Ported two cases in RapidsCastSuite because of a spark issue https://issues.apache.org/jira/browse/SPARK-40851, the 330 test cases failed under new version of jdk8, so updated to a newer version of this test to make it pass.
  • Removed UTC settings in TestsBaseTrait to fix three issues in RapidsJsonSuite, Spark use "America/Los_Angeles" as default timezone so we should not set additional utc config. spark code.

Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
@thirtiseven thirtiseven self-assigned this Jun 11, 2024
@thirtiseven thirtiseven marked this pull request as ready for review June 11, 2024 02:16
@sameerz sameerz added the test Only impacts tests label Jun 11, 2024
@thirtiseven
Copy link
Collaborator Author

build

@thirtiseven
Copy link
Collaborator Author

build

Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
@thirtiseven
Copy link
Collaborator Author

Premerge failed because the tests still think timezone is UTC, maybe caused by some settings in CI. Hardcoded to LA and trying again.

@thirtiseven
Copy link
Collaborator Author

build

Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
@thirtiseven
Copy link
Collaborator Author

build

Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
@thirtiseven
Copy link
Collaborator Author

build

Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
@GaryShen2008
Copy link
Collaborator

After removing UTC config, I found the new added UT cases passed by falling back to CPU. So, I think we may not need to copy them out.

@thirtiseven
Copy link
Collaborator Author

Hi @GaryShen2008 , what’s your java version? The new added cases can pass under older Java version but fail on newer ones.

override def beforeAll(): Unit = {
super.beforeAll()
// Set timezone to UTC to avoid fallback, so that tests run on GPU to detect bugs
TimeZone.setDefault(TimeZone.getTimeZone("UTC"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

set it back in AfterAll()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

override def beforeAll(): Unit = {
super.beforeAll()
// Set timezone to UTC to avoid fallback, so that tests run on GPU to detect bugs
TimeZone.setDefault(TimeZone.getTimeZone("UTC"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

set it back in AfterAll()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@thirtiseven
Copy link
Collaborator Author

build

Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
@thirtiseven
Copy link
Collaborator Author

build

@thirtiseven thirtiseven requested a review from binmahone June 19, 2024 07:45
@binmahone
Copy link
Collaborator

LGTM

@thirtiseven thirtiseven merged commit 3cb54c4 into NVIDIA:branch-24.08 Jun 27, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Only impacts tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants