Prevent tests from leaking their respective RNG #4497
Conversation
| @@ -714,6 +714,7 @@ def test_random_apply(device): | |||
| @pytest.mark.parametrize('channels', [1, 3]) | |||
| def test_gaussian_blur(device, channels, meth_kwargs): | |||
| tol = 1.0 + 1e-10 | |||
| torch.manual_seed(12) | |||
There was a problem hiding this comment.
I had to add this to prevent the test from failing (see https://app.circleci.com/pipelines/github/pytorch/vision/10830/workflows/946973f0-bad1-48ed-aaf3-f3720ab5f56a/jobs/828462).
If anything, this shows that the PR is working as expected and that this test is a bit flaky, and sensible to the _create_data() call.
To confirm I parametrized it over 100 random seeds, and I got 8 failures over 1000+ test instances. Each time, just 1 pixel was off. Considering the low failure rate, I think it's fine to keep the manual seeding here.
| @pytest.fixture(autouse=True) | ||
| def prevent_leaking_rng(): |
There was a problem hiding this comment.
This is pretty much the same as freeze_rng_state() https://github.com/pytorch/vision/blob/main/test/common_utils.py#L86
autouse=True means that this fixture will be used by every single test automatically.
NicolasHug
changed the title
NicolasHug
changed the title
|
Nicolas and I had an offline discussion about this a while back and I wanted to also give my opinion here: IMO, this is not needed. There should be only two types of tests for this purpose
In case 1. we don't need a fixed RNG state, because it shouldn't matter. If it does, the test actually belongs to category 2. and should be fixed. In case 2. the RNG state doesn't matter since we overwrite the seed anyway. @NicolasHug Is there an actual issue that this will fix? |
There was a problem hiding this comment.
LGTM, thanks!
PyTorch does provide a generator that we can pass to (some) functions, but last time I checked it didn't work for CUDA (but that was a long time ago). If we had a better support for passing the generator in PyTorch (as we do in numpy), we could use it, but that's not the case yet IIRC.
Your solution seems ok with me, thanks!
|
@pmeier I think that #3032 (comment) and the very fact that
While you're right in principle, the reality is much less simple unfortunately. We have tests for which the RNG is not entirely controlled and in fact depends on other tests. |
|
Also, and possibly most importantly:
Before this PR, we had no such tests: all tests were using a pre-defined RNG state that was leaked by the previous tests that were run before. There was almost zero test that was relying on the global pytorch RNG that would change across executions. The only tests that did were the tests that were run before the first test that called |
|
Maybe I misunderstood the purpose. From your comments I get that you want this to detect flaky tests that we don't realize are there, because they pass until the the order of execution changes and the get different "random" data. Is that correct? |
This is indeed a positive consequence of the changes, but that's not the main intent. The main intent is that any test |
Summary: * Add autouse fixture to save and reset RNG in tests * Add other RNG generators * delete freeze_rng_state * Hopefully fix GaussianBlur test * Alternative fix, probably better * revert changes to test_models Reviewed By: datumbox Differential Revision: D31270915 fbshipit-source-id: e60273bc90985aaac5e0aaa20cfbfdb86639b8b8 Co-authored-by: Francisco Massa <[email protected]>
* Add autouse fixture to save and reset RNG in tests * Add other RNG generators * delete freeze_rng_state * Hopefully fix GaussianBlur test * Alternative fix, probably better * revert changes to test_models Co-authored-by: Francisco Massa <[email protected]>
This PR adds a new autouse pytest fixture to prevent each test from leaking its RNG consumption to other tests.
We've had strange errors and major headaches because of this in the past #3032 (comment)
After a bit of thoughts and discussions with @pmeier and @vfdev-5, I don't think there's a better alternative than relying on an autouse fixture. It's a bit implicit, but that's the best we can do considering that pytorch only offers
manual_seed()to contrrol the RNG, which leaks to the entire program execution.cc @pmeier