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

Add flaky decorator for tests with remote resources to reduce cold start failure rate. #700

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

charles-turner-1
Copy link
Collaborator

@charles-turner-1 charles-turner-1 commented Feb 3, 2025

Change Summary

  • Add flaky pytest plugin
  • Add @pytest.mark.flaky(max_runs=3, min_passes=1) to test that request cloud resources. This should give cloud containers time to spin up if needed.
  • Updated CI environments to match.

Related issue number

#699

Checklist

  • Unit tests for the changes exist N/A
  • Tests pass on CI
  • Documentation reflects the changes where applicable N/A
    • Potentially could think about adding notes about this issue with cloud resources to documentation.

…ncidence of cold start related test failures
pytest.mark.flaky() decorator seems to work fine on my local machine but
is causing CI issues - not quite sure why
@charles-turner-1
Copy link
Collaborator Author

Test failures on CI are resulting from

FAILED tests/test_source.py::test_open_dataset_kerchunk - ValueError: Reference-FS's target filesystem must have same valueof asynchronous

Investigating the cause now.

@mgrover1
Copy link
Collaborator

mgrover1 commented Feb 3, 2025

Test failures on CI are resulting from

FAILED tests/test_source.py::test_open_dataset_kerchunk - ValueError: Reference-FS's target filesystem must have same valueof asynchronous

Investigating the cause now.

Interesting... let me know what you find! I have not seen that before!

@charles-turner-1
Copy link
Collaborator Author

Interesting... let me know what you find! I have not seen that before!

Weirdly enough, it consistently passes on 3.10, fails on 3.11 & 3.12. My local environment is 3.13 but on MacOS system.

I'm gonna mark this as draft whilst I work on it - tests all pass locally, so I'm pretty stumped as to whats going on...

@charles-turner-1 charles-turner-1 marked this pull request as draft February 3, 2025 02:20
@mgrover1
Copy link
Collaborator

mgrover1 commented Feb 3, 2025

Sounds good! I am working on Australian time this week and can review later... 👍 currently on travel to Tasmania...

@andersy005
Copy link
Member

I'm gonna mark this as draft whilst I work on it - tests all pass locally, so I'm pretty stumped as to whats going on...

there is a chance this might be caused by zarr v3. when you get a chance, can you confirm if you are using zarr v2 or zarr v3?

it might be worth pinning the zarr requirements to versions earlier than v3 until we have a robust solution for dealing with the breaking changes in zarr v3.

@charles-turner-1
Copy link
Collaborator Author

I've checked and my local & CI 3.10 are both on zarr==2.18.3, CI 3.11+ are on zarr 3.

Pinning the deps now.

@charles-turner-1 charles-turner-1 marked this pull request as ready for review February 3, 2025 03:11
@charles-turner-1
Copy link
Collaborator Author

Sounds good! I am working on Australian time this week and can review later... 👍 currently on travel to Tasmania...

Looks like this is ready to review now - @andersy005's zarr suggestion fixed the issue.

I'm gonna see if I can use this same cold start trick to stop the documentation builds failing, so hopefully I'll drop that in too soon

@charles-turner-1 charles-turner-1 linked an issue Feb 3, 2025 that may be closed by this pull request
Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the fix!

@mgrover1 mgrover1 merged commit 4e8a8d5 into main Feb 3, 2025
9 checks passed
@mgrover1 mgrover1 deleted the cold-start branch February 3, 2025 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky tests due to cloud services - cold starts
3 participants