Repository: Lightning-AI/pytorch-lightning
Pull request: Rerun flaky profiler tests on failure
## What does this PR do?

<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.

If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

The following links the related issue to the PR (https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
-->

Fixes #9462 

Uses a pytest plugin `pytest-rerunfailures` to rerun flaky profiler tests on failure.

### Does your PR introduce any breaking changes? If yes, please list them.

<!-- FILL IN or None -->

## Before submitting

- [X] Was this **discussed/approved** via a GitHub issue? (not for typos and docs)
- [X] Did you read the [contributor guideline](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.github/CONTRIBUTING.md), **Pull Request** section?
- [X] Did you make sure your **PR does only one thing**, instead of bundling different changes together?
- [X] Did you make sure to **update the documentation** with your changes? (if necessary)
- [ ] Did you write any **new necessary tests**? (not for typos and docs)
- [X] Did you verify new and **existing tests pass** locally with your changes?
- [X] Did you list all the **breaking changes** introduced by this pull request?
- [X] Did you **update the [CHANGELOG](https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md)**? (not for typos, docs, test updates, or internal minor changes/refactorings)

<!-- In the CHANGELOG, separate each item in the unreleased section by a blank line to reduce collisions -->

## PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read [Review guidelines](https://github.com/PyTorchLightning/pytorch-lightning/wiki/Review-guidelines). In short, see the following bullet-list:

- [X] Is this pull request ready for review? (if not, please submit in draft mode)
- [X] Check that all items from **Before submitting** are resolved
- [X] Make sure the title is self-explanatory and the description concisely explains the PR
- [X] Add labels and milestones (and optionally projects) to the PR so it can be classified

## Did you have fun?

Make sure you had fun coding 🙃


Problem statement:
fix flaky `test_simple_profiler_overhead`
## 🐛 Bug

A flaky profiler test blocks our auto-merge. Bad!

### To Reproduce

Test run `tests/profiler/test_profiler/test_simple_profiler_overhead`
or `test_simple_profiler_iterable_durations`


```bash
________________________ test_simple_profiler_overhead _________________________

simple_profiler = <pytorch_lightning.profiler.simple.SimpleProfiler object at 0x7f30f86b8eb8>
n_iter = 5

    def test_simple_profiler_overhead(simple_profiler, n_iter=5):
        """Ensure that the profiler doesn't introduce too much overhead during training."""
        for _ in range(n_iter):
            with simple_profiler.profile("no-op"):
                pass
    
        durations = np.array(simple_profiler.recorded_durations["no-op"])
>       assert all(durations < PROFILER_OVERHEAD_MAX_TOLERANCE)
E       assert False
E        +  where False = all(array([3.19999992e-06, 1.90000003e-06, 1.60000002e-06, 5.33505000e-04,\n       1.90000003e-06]) < 0.0005)

/home/runner/work/pytorch-lightning/pytorch-lightning/tests/profiler/test_profiler.py:91: AssertionError



_____________ test_simple_profiler_iterable_durations[a-expected0] _____________

simple_profiler = <pytorch_lightning.profiler.simple.SimpleProfiler object at 0x13529c0a0>
action = 'a', expected = [3, 1]

    @pytest.mark.parametrize(
        ["action", "expected"], [pytest.param("a", [3, 1]), pytest.param("b", [2]), pytest.param("c", [1])]
    )
    def test_simple_profiler_iterable_durations(simple_profiler, action: str, expected: list):
        """Ensure the reported durations are reasonably accurate."""
        iterable = _sleep_generator(expected)
    
        for _ in simple_profiler.profile_iterable(iterable, action):
            pass
    
        # we exclude the last item in the recorded durations since that's when StopIteration is raised
>       np.testing.assert_allclose(simple_profiler.recorded_durations[action][:-1], expected, rtol=0.2)
E       AssertionError: 
E       Not equal to tolerance rtol=0.2, atol=0
E       
E       Mismatch: 50%
E       Max absolute difference: 0.75093935
E       Max relative difference: 0.25031312
E        x: array([3.750939, 1.083181])
E        y: array([3, 1])

/Users/runner/work/pytorch-lightning/pytorch-lightning/tests/profiler/test_profiler.py:81: AssertionError

```
### Expected behavior

Obvious.

### Environment

Our own CI:
pytest (ubuntu-18.04, 3.6, minimal, stable)


### Additional context

<!-- Add any other context about the problem here. -->

Hints from issue discussion:
I had a look but this is really tricky because testing runtime is inherently flaky.

The only options I see are:
- Remove these tests.
- Mark them to be re-tried on failure (just reduces the flakiness but it's still there)
Thanks for taking a look. Let's try the second option and see if that helps. If not we can later consider the first option. 
Was reading here about handling flakiness with pytest: https://docs.pytest.org/en/latest/explanation/flaky.html
So looks like we would need to use one of the plugins listed there. 

Also very interesting:

> Plugins to deliberately randomize tests can help expose tests with state problems:
> 
> pytest-random-order
> pytest-randomly

Could be something to try in the future.

Okay. Should be easy enough. I'd use `https://github.com/pytest-dev/pytest-rerunfailures` which seems to be the most popular.

We can also mark this as a "good first issue" if you want.
yup!
Is this issue still open? I can pick this up if no one is working on it. 👍

Commit message: Fix flaky profiler tests

Patch to review:
diff --git a/requirements/test.txt b/requirements/test.txt
index 8d0e0712d7058..0e39d0821c63d 100644
--- a/requirements/test.txt
+++ b/requirements/test.txt
@@ -1,6 +1,7 @@
 coverage>5.2.0
 codecov>=2.1
 pytest>=6.0
+pytest-rerunfailures>=10.2
 check-manifest
 twine==3.2
 mypy>=0.900
diff --git a/tests/profiler/test_profiler.py b/tests/profiler/test_profiler.py
index 9e22c3f8ec9f7..e2f5c193319d8 100644
--- a/tests/profiler/test_profiler.py
+++ b/tests/profiler/test_profiler.py
@@ -52,6 +52,7 @@ def simple_profiler():
     return SimpleProfiler()
 
 
+@pytest.mark.flaky(reruns=5)
 @pytest.mark.parametrize(["action", "expected"], [("a", [3, 1]), ("b", [2]), ("c", [1])])
 def test_simple_profiler_durations(simple_profiler, action: str, expected: list):
     """Ensure the reported durations are reasonably accurate."""
@@ -65,6 +66,7 @@ def test_simple_profiler_durations(simple_profiler, action: str, expected: list)
     np.testing.assert_allclose(simple_profiler.recorded_durations[action], expected, rtol=0.2)
 
 
+@pytest.mark.flaky(reruns=5)
 @pytest.mark.parametrize(["action", "expected"], [("a", [3, 1]), ("b", [2]), ("c", [1])])
 def test_simple_profiler_iterable_durations(simple_profiler, action: str, expected: list):
     """Ensure the reported durations are reasonably accurate."""
@@ -159,6 +161,7 @@ def advanced_profiler(tmpdir):
     return AdvancedProfiler(dirpath=tmpdir, filename="profiler")
 
 
+@pytest.mark.flaky(reruns=5)
 @pytest.mark.parametrize(["action", "expected"], [("a", [3, 1]), ("b", [2]), ("c", [1])])
 def test_advanced_profiler_durations(advanced_profiler, action: str, expected: list):
 
@@ -173,6 +176,7 @@ def test_advanced_profiler_durations(advanced_profiler, action: str, expected: l
     np.testing.assert_allclose(recored_total_duration, expected_total_duration, rtol=0.2)
 
 
+@pytest.mark.flaky(reruns=5)
 @pytest.mark.parametrize(["action", "expected"], [("a", [3, 1]), ("b", [2]), ("c", [1])])
 def test_advanced_profiler_iterable_durations(advanced_profiler, action: str, expected: list):
     """Ensure the reported durations are reasonably accurate."""


Code review comments:
diff_hunk: @@ -52,6 +52,7 @@ def simple_profiler():
     return SimpleProfiler()
 
 
+@pytest.mark.flaky(reruns=5)
path: tests/profiler/test_profiler.py
text: @user1:
I think we can lower the number to 3 and increase if necessary in the future

Or did you choose 5 for a particular reason?

@author:
No particular reason. That's just the number used in `pytest-rerunfailures` examples.