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

SNOW-1332287, SNOW-1332288: Fix flaky tests for skew & kurtosis functions #1409

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

sfc-gh-mheimel
Copy link
Contributor

@sfc-gh-mheimel sfc-gh-mheimel commented Apr 22, 2024

Fixes a flaky tests issue for tests involving skew/kurtosis correctness. We recently updated the computation for skew/kurtosis in XP to fix an issue that could lead to catastrophic loss of precision for very large tables. This fix causes skew/kurtosis to produce slightly different results, even for smaller cases. Since the failing tests compares the results of skew/kurtosis computation to expected constants absolutely, even tiny differences can cause a failure.

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

Fixes SNOW-1332287 & SNOW-1332288.

  1. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
  2. Please describe how your code solves the related issue.

Fixed by a) Updating the reference values for skew/kurtosis tests to match the new computation, b) Making sure that skew/kurtosis tests use approximate comparisons for correctness testing.

Copy link

github-actions bot commented Apr 22, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@sfc-gh-mheimel
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@sfc-gh-mheimel sfc-gh-mheimel changed the title SNOW-1332287, SNOW-1332288: Fix failing Snowpark tests SNOW-1332287, SNOW-1332288: Fix flaky tests for skew & kurtosis functions Apr 22, 2024
@sfc-gh-jdu sfc-gh-jdu added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label Apr 22, 2024
Copy link
Collaborator

@sfc-gh-jdu sfc-gh-jdu left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

CHANGELOG.md Outdated Show resolved Hide resolved
@sfc-gh-jdu
Copy link
Collaborator

BTW, test_kurtosis seems still failing, maybe because your skew & kurtosis function changes are not in production yet?

@sfc-gh-mheimel
Copy link
Contributor Author

sfc-gh-mheimel commented Apr 22, 2024

BTW, test_kurtosis seems still failing, maybe because your skew & kurtosis function changes are not in production yet?

Yes, my changes will only be rolled out once 8.17 hits production (± later this week). So, this will fail until then - and the existing code will fail from then on.

I could reduce the floating point threshold of the test further to ensure it passes on both pre and post 8.17. Unsure what's the best way forward is here.

@sfc-gh-jdu
Copy link
Collaborator

BTW, test_kurtosis seems still failing, maybe because your skew & kurtosis function changes are not in production yet?

Yes, my changes will only be rolled out once 8.17 hits production (± later this week). So, this will fail until then - and the existing code will fail from then on.

I could reduce the floating point threshold of the test further to ensure it passes on both pre and post 8.17. Unsure what's the best way forward is here.

Can you merge this change it to branch test-v1.14.0 first, which is pulled in regression test. Once 8.17 is in, we merge again to main branch.

@sfc-gh-jdu
Copy link
Collaborator

It seems ready for merging?

@sfc-gh-jdu sfc-gh-jdu merged commit 88a5697 into main Apr 29, 2024
25 of 26 checks passed
@sfc-gh-jdu sfc-gh-jdu deleted the mheimel-snow-1332287 branch April 29, 2024 17:28
@github-actions github-actions bot locked and limited conversation to collaborators Apr 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants