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

Make test ordering and parametrization reproducible #4788

Merged
merged 8 commits into from
Jan 7, 2022

Conversation

pavoljuhas
Copy link
Collaborator

@pavoljuhas pavoljuhas commented Dec 30, 2021

Implement deterministic ordering of pytest parameters.
Add CIRQ_TESTING_RANDOM_SEED environment variable for seeding
random numbers generation in random and numpy.random modules.
Use commit epoch time as the default seed to have a non-constant,
but deterministic seeding.

NB: Use empty CIRQ_TESTING_RANDOM_SEED to prevent seeding, for example,
CIRQ_TESTING_RANDOM_SEED="" check/pytest

This implements #4787

Avoid order dependent on set or dict ordering.
Make it obey seeding with numpy.random.seed.
Seed random number generators when set to have reproducible
pytest parameters some of which depend on random numbers.
Adjust the check/pytest* scripts to set a zero seed by default.
Set reproducible, but non-constant default random seed for testing.
@pavoljuhas pavoljuhas requested review from cduck, vtomole and a team as code owners December 30, 2021 02:37
@pavoljuhas pavoljuhas requested a review from viathor December 30, 2021 02:37
@CirqBot CirqBot added the size: S 10< lines changed <50 label Dec 30, 2021
Some CI tests run in Python environment without numpy.
Skip seeding of random number generator in such case.
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM

@MichaelBroughton MichaelBroughton self-assigned this Jan 4, 2022
@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jan 4, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jan 4, 2022
@vtomole
Copy link
Collaborator

vtomole commented Jan 4, 2022

We can also use pytest-randomly to get random a seed but it will also come with other stuff; like running the tests in random order, so I'm fine with this.

@vtomole
Copy link
Collaborator

vtomole commented Jan 4, 2022

Oh, we can pass in the --randomly-dont-reorganize flag. But on the other hand, pytest-randomly will be a new dependency we add to Cirq's dev tools on top of the already enormous list of dev tool packages we have..

Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

Let's add export CIRQ_TESTING_RANDOM_SEED to the incremental coverage checks as well.

@MichaelBroughton MichaelBroughton removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jan 4, 2022
@pavoljuhas
Copy link
Collaborator Author

Let's add export CIRQ_TESTING_RANDOM_SEED to the incremental coverage checks as well.

The incremental-coverage scripts run tests with check/pytest which already exports CIRQ_TESTING_RANDOM_SEED:

Is there perhaps some other script that I am missing?

Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

Ah, okay.

If you have the time to try pytest-randomly, please do. In the conftest.py, we can have

@pytest.fixture()
def global_seed(pytestconfig) -> int:
    return pytestconfig.getoption("randomly_seed")

This will shave a lot of code in this PR with the caveat of us adding pytest-randomly as a Dev tools dependency.

@pavoljuhas
Copy link
Collaborator Author

pavoljuhas commented Jan 4, 2022

If you have the time to try pytest-randomly, please do.

They have a wontfix issue pytest-dev/pytest-randomly#75 which implies that having random parameters in @pytest.mark.parametrize (random at import time) breaks distributed run with pytest-xdist.
That is just the use case I'd like to support here as Cirq has a number of such parametrizations, for example,

@pytest.mark.parametrize('seed', [random.randint(0, 2 ** 32) for _ in range(10)])

Resetting random seed for every test case as done in pytest-randomly sounds fishy, because every test then gets the same handful of random numbers from the start of a seeded RNG sequence.

On the other hand, the randomized test order with pytest-randomly sounds useful provided it indeed works with pytest-xdist.
@vtomole - perhaps that should be opened as a separate issue?

@pavoljuhas
Copy link
Collaborator Author

@vtomole - please let me know if there are still some changes required here.

@vtomole
Copy link
Collaborator

vtomole commented Jan 7, 2022

@pavoljuhas Thanks for the ping. I missed the first notification. I'll finish reviewing this today.

Copy link
Collaborator

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

LGTM.

@vtomole vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jan 7, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jan 7, 2022
@vtomole
Copy link
Collaborator

vtomole commented Jan 7, 2022

which implies that having random parameters in @pytest.mark.parametrize (random at import time) breaks distributed run with pytest-xdist.

Okay. Thanks for looking into it.

@CirqBot CirqBot merged commit 45e7ddd into quantumlib:master Jan 7, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jan 7, 2022
@pavoljuhas pavoljuhas deleted the reproducible-test-ordering branch January 7, 2022 23:32
MichaelBroughton pushed a commit to MichaelBroughton/Cirq that referenced this pull request Jan 22, 2022
Implement deterministic ordering of pytest parameters.
Add CIRQ_TESTING_RANDOM_SEED environment variable for seeding
random numbers generation in random and numpy.random modules.
Use commit epoch time as the default seed to have a non-constant,
but deterministic seeding.

NB: Use empty CIRQ_TESTING_RANDOM_SEED to prevent seeding, for example,
    CIRQ_TESTING_RANDOM_SEED="" check/pytest

This implements quantumlib#4787
CirqBot pushed a commit that referenced this pull request Sep 20, 2022
- Require pytest-randomly for the testing to ensure RNG-generated test
  parameters are consistent across parallel test jobs.
- Remove check/pytest option --actually-quiet which disables pytest-randomly
  hook for seeding parallel test jobs.
- Remove `CIRQ_TESTING_RANDOM_SEED` as it is not needed anymore.
- Remove RNG seeding in the tests where it seems redundant.
- Clean up one instance of unnecessary test parametrization.

This upholds #4787 and replaces #4788.
Reverts #1825 and obsoletes #1826.
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Implement deterministic ordering of pytest parameters.
Add CIRQ_TESTING_RANDOM_SEED environment variable for seeding
random numbers generation in random and numpy.random modules.
Use commit epoch time as the default seed to have a non-constant,
but deterministic seeding.

NB: Use empty CIRQ_TESTING_RANDOM_SEED to prevent seeding, for example,
    CIRQ_TESTING_RANDOM_SEED="" check/pytest

This implements quantumlib#4787
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…ib#5868)

- Require pytest-randomly for the testing to ensure RNG-generated test
  parameters are consistent across parallel test jobs.
- Remove check/pytest option --actually-quiet which disables pytest-randomly
  hook for seeding parallel test jobs.
- Remove `CIRQ_TESTING_RANDOM_SEED` as it is not needed anymore.
- Remove RNG seeding in the tests where it seems redundant.
- Clean up one instance of unnecessary test parametrization.

This upholds quantumlib#4787 and replaces quantumlib#4788.
Reverts quantumlib#1825 and obsoletes quantumlib#1826.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Implement deterministic ordering of pytest parameters.
Add CIRQ_TESTING_RANDOM_SEED environment variable for seeding
random numbers generation in random and numpy.random modules.
Use commit epoch time as the default seed to have a non-constant,
but deterministic seeding.

NB: Use empty CIRQ_TESTING_RANDOM_SEED to prevent seeding, for example,
    CIRQ_TESTING_RANDOM_SEED="" check/pytest

This implements quantumlib#4787
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
…ib#5868)

- Require pytest-randomly for the testing to ensure RNG-generated test
  parameters are consistent across parallel test jobs.
- Remove check/pytest option --actually-quiet which disables pytest-randomly
  hook for seeding parallel test jobs.
- Remove `CIRQ_TESTING_RANDOM_SEED` as it is not needed anymore.
- Remove RNG seeding in the tests where it seems redundant.
- Clean up one instance of unnecessary test parametrization.

This upholds quantumlib#4787 and replaces quantumlib#4788.
Reverts quantumlib#1825 and obsoletes quantumlib#1826.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants