Make tests restore global random seed #5629
Conversation
pavoljuhas
commented
Jun 27, 2022
pavoljuhas
commented
- Remove RNG seeding where redundant
- Restore global RNG state after tests that seed it to a constant
- Add pytest fixture for restoring RNG state
Shared cleanup after tests that use np.random.seed() or random.seed().
Cleanup after tests that seed global random number generators in either of stock random or numpy.random modules.
These instances of seeding do not appear to affect test results.
pavoljuhas
requested review from
mrwojtek,
a team,
vtomole and
cduck
as code owners
There was a problem hiding this comment.
It seems to me that if test A requires a particular random seed it should be responsible for setting that seed, rather than relying on some other test B to reset the PRNG state before A executes. The restore_random_state decorator seems like it could introduce undesirable coupling between tests.
An alternative would be to create an autouse fixture that seeds the PRNGs to a known state before every test, which would give deterministic behavior everywhere without coupling.
| @@ -39,6 +39,7 @@ def test_initialization_reproducible_with_seed(seed): | |||
| eq.add_equality_group(*mappings) | |||
|
|
|||
|
|
|||
| @pytest.mark.usefixtures('restore_random_state') | |||
| @pytest.mark.parametrize('graph_seed,state', [(random.randint(0, 2**32), np.random.get_state())]) | |||
There was a problem hiding this comment.
I don't understand why the np.random.get_state() call is here in the decorator. Normally parametrization is for running a test multiple times, but here we run only once. Also, it's not clear why we have to call np.random.set_state internally at all. Rather, it looks like we could do something like:
mappings = [get_seeded_initial_mapping(graph_seed, state) for _ in range(3)]
There was a problem hiding this comment.
I don't understand why the np.random.get_state() call is here in the decorator.
Good point. I will tweak it to get those values in the test body.
Also, it's not clear why we have to call np.random.set_state internally at all...
I think the intent is to test get_initial_mapping() with init_seed=None -
I did not dig into the code, but it presumably uses a global numpy RNG in such case.
The test then fails without the np.random.set_state call. At any rate,
I can remove the restore_random_state fixture here, because there is no seeding of RNG to a constant.
Done in 4b89b70
| for seed in range(10): | ||
| random.seed(seed) | ||
|
|
||
| for _ in range(10): |
There was a problem hiding this comment.
This test might have random behavior, if any other tests forget to restore the random state. I think we should try to avoid such coupling between tests.
There was a problem hiding this comment.
I'd prefer random.shuffle in this test to depend on CIRQ_TESTING_RANDOM_SEED.
In its initial form it would always produce the same output per constant seeds 0,..,9.
and it also reseeded RNG to 9 for any tests calling it later.
The tests are supposed to run with a reproducible, but non-constant seeding of RNGs. This is accomplished with What I am trying to accomplish is that test C which does not set any seed gets pseudo-random values from RNG functions that depend on the CIRQ_TESTING_RANDOM_SEED value. If it is preceded by test B that seeds global RNG and does not clean after itself, test C is always going to get the same "random" values dependent on the seed in B.
That could be done with requiring pytest-randomly, but I feel that is out-of-scope here. |
No need to parametrize one group of values, set them in the test body instead. Also no need to restore_random_state as there is no seeding to a constant.
|
@maffoo - thank you for comments. PTAL when you get a chance. |
|
Replaced by #5868 where pytest-randomly resets random seed for each test case. Closing as obsolete. |
