-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Add retry to pytest for transient failure #37046
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @shunping, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust retry mechanism for pytest tests across various Python environments. By integrating the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
cc'ed @damccorm |
| commands = | ||
| python apache_beam/examples/complete/autocomplete_test.py | ||
| bash {toxinidir}/scripts/run_pytest.sh {envname} "{posargs}" | ||
| bash {toxinidir}/scripts/run_pytest.sh {envname} "{posargs}" '--reruns 1 --reruns-delay 5' |
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.
While I understand the pragmatic nature of this fix, I'm not sure it is the right approach. Doing this has a high likelihood of masking real failures, and generally I think we should be tracking down/fixing flakes. If there are things that innately mean we're going to have this level of flakiness, we're likely passing those on to users as well.
I'm open to more discussion here, but at a minimum I think if we're going to make a change like this it should be surfaced to the dev list, and probably should come with data that describes the problem and the reason we have this kind of flakiness. We could also evaluate alternate approaches there (for example, reducing the number of tests we run to reduce flakiness, only doing this for PR runs, etc...)
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.
My rationale for this approach was based on PR #35915, where pytest retry arguments were introduced:
https://github.com/apache/beam/pull/35915/files#diff-33fb11ecf72212eda83aaf8e36f94816ad447d8d12896dc3b5e5ac3727adbbd1R114,
even though there are no actual effects for those arguments after my investigation.
I misinterpreted the acceptance of that PR as a general agreement to use retries for flaky tests and tried to fix the retry 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.
That makes sense - my understanding from that PR is that those were going to be used for Grpc retries somehow, but I may be wrong (I probably shouldn't have merged with the meaningless options though).
Regardless, I'd be more open to this for a specific GHA suite (especially a precommit), but I think it warrants broader discussion/data first
| commands = | ||
| python apache_beam/examples/complete/autocomplete_test.py | ||
| bash {toxinidir}/scripts/run_pytest.sh {envname} "{posargs}" | ||
| bash {toxinidir}/scripts/run_pytest.sh {envname} "{posargs}" '--reruns 1 --reruns-delay 5' |
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.
Nit: If we're going to do this for every invocation, we should include it as part of the run_pytest.sh script itself
|
Assigning reviewers: R: @damccorm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
This change enables a retry mechanism for
pytestto improve the developer experience and the reliability of our test/release pipelines.Recently, we have observed an increase in flaky Python tests. These failures disrupt development and release validation, often requiring manual and time-consuming test reruns. Some post-commit workflows can take hours to complete, and multiple flaky tests can significantly delay getting a green build.
To address this, this change leverages the
pytest-rerunfailuresplugin with the following configuration:--reruns 1: Allows for a single retry of a failed test. We want to keep this a small number so that we do not risk hiding a truly flaky test.--reruns-delay 5: Introduces a 5-second delay before the retry.This approach provides a balance between mitigating transient failures (such as GRPC deadline exceeded) and ensuring that consistently flaky tests are still surfaced and addressed.
fixes #37038