From 82bfa8f981417a7f10ac8e3f8869e103d482c28a Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 8 Sep 2022 16:17:37 -0700 Subject: [PATCH 01/12] Remove RNG seeding where redundant These instances of seeding do not appear to affect test results. --- cirq-core/cirq/ops/boolean_hamiltonian_test.py | 4 +--- cirq-core/cirq/sim/clifford/clifford_simulator_test.py | 2 -- .../controlled_gate_decomposition_test.py | 4 ---- examples/direct_fidelity_estimation_test.py | 1 - 4 files changed, 1 insertion(+), 10 deletions(-) diff --git a/cirq-core/cirq/ops/boolean_hamiltonian_test.py b/cirq-core/cirq/ops/boolean_hamiltonian_test.py index c4a3f781720..7bb136b4a04 100644 --- a/cirq-core/cirq/ops/boolean_hamiltonian_test.py +++ b/cirq-core/cirq/ops/boolean_hamiltonian_test.py @@ -127,9 +127,7 @@ def test_gray_code_sorting(n_bits, expected_hs): x //= 2 hs_template.append(tuple(sorted(h))) - for seed in range(10): - random.seed(seed) - + for _ in range(10): hs = hs_template.copy() random.shuffle(hs) diff --git a/cirq-core/cirq/sim/clifford/clifford_simulator_test.py b/cirq-core/cirq/sim/clifford/clifford_simulator_test.py index 0be26e9cc1e..72ff4198e7f 100644 --- a/cirq-core/cirq/sim/clifford/clifford_simulator_test.py +++ b/cirq-core/cirq/sim/clifford/clifford_simulator_test.py @@ -335,8 +335,6 @@ def test_clifford_circuit(split): (q0, q1) = (cirq.LineQubit(0), cirq.LineQubit(1)) circuit = cirq.Circuit() - np.random.seed(0) - for _ in range(100): x = np.random.randint(7) diff --git a/cirq-core/cirq/transformers/analytical_decompositions/controlled_gate_decomposition_test.py b/cirq-core/cirq/transformers/analytical_decompositions/controlled_gate_decomposition_test.py index f7286c58bbd..0756e860a50 100644 --- a/cirq-core/cirq/transformers/analytical_decompositions/controlled_gate_decomposition_test.py +++ b/cirq-core/cirq/transformers/analytical_decompositions/controlled_gate_decomposition_test.py @@ -88,7 +88,6 @@ def test_decompose_specific_matrices(): def test_decompose_random_unitary(): - np.random.seed(0) for controls_count in range(5): for _ in range(10): _test_decompose(_random_unitary(), controls_count) @@ -97,7 +96,6 @@ def test_decompose_random_unitary(): def test_decompose_random_special_unitary(): - np.random.seed(0) for controls_count in range(5): for _ in range(10): _test_decompose(_random_special_unitary(), controls_count) @@ -112,7 +110,6 @@ def _decomposition_size(U, controls_count): def test_decompose_size_special_unitary(): - np.random.seed(0) u = _random_special_unitary() assert _decomposition_size(u, 0) == (1, 0, 0) assert _decomposition_size(u, 1) == (3, 2, 0) @@ -125,7 +122,6 @@ def test_decompose_size_special_unitary(): def test_decompose_size_unitary(): - np.random.seed(0) u = _random_unitary() assert _decomposition_size(u, 0) == (1, 0, 0) assert _decomposition_size(u, 1) == (4, 2, 0) diff --git a/examples/direct_fidelity_estimation_test.py b/examples/direct_fidelity_estimation_test.py index a597ef5b724..3b9a7d02b4f 100644 --- a/examples/direct_fidelity_estimation_test.py +++ b/examples/direct_fidelity_estimation_test.py @@ -111,7 +111,6 @@ def noisy_moment(self, moment, system_qubits): ), ] - np.random.seed(0) noise = NoiseOnLastQubitOnly() noisy_simulator = cirq.DensityMatrixSimulator(noise=noise) From 03ea1fccb6a0cbd1e99edf6aa73d676324b04757 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 8 Sep 2022 16:33:24 -0700 Subject: [PATCH 02/12] Remove unnecessary test parametrization No need to parametrize for a single set of values, let us set them in the test body instead. --- cirq-core/cirq/contrib/routing/initialization_test.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cirq-core/cirq/contrib/routing/initialization_test.py b/cirq-core/cirq/contrib/routing/initialization_test.py index 66c2aea5b73..4400d6ed469 100644 --- a/cirq-core/cirq/contrib/routing/initialization_test.py +++ b/cirq-core/cirq/contrib/routing/initialization_test.py @@ -39,8 +39,9 @@ def test_initialization_reproducible_with_seed(seed): eq.add_equality_group(*mappings) -@pytest.mark.parametrize('graph_seed,state', [(random.randint(0, 2**32), np.random.get_state())]) -def test_initialization_with_no_seed(graph_seed, state): +def test_initialization_with_no_seed(): + graph_seed = random.randint(0, 2**32) + state = np.random.get_state() mappings = [] for _ in range(3): np.random.set_state(state) From 5b8217579c69d7269841b73c8a00bd71eaf9c026 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 8 Sep 2022 16:36:23 -0700 Subject: [PATCH 03/12] Require the pytest-randomly plugin --- dev_tools/requirements/deps/pytest.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/dev_tools/requirements/deps/pytest.txt b/dev_tools/requirements/deps/pytest.txt index 47846e16d04..ceba7fb2352 100644 --- a/dev_tools/requirements/deps/pytest.txt +++ b/dev_tools/requirements/deps/pytest.txt @@ -3,6 +3,7 @@ pytest pytest-asyncio pytest-cov +pytest-randomly # Notebook >=6.4.8 + coverage > 6.2 hangs CI: https://github.com/quantumlib/Cirq/issues/4897 coverage<=6.2 From 09627d2d1f2927469d2042fcd38204379aca496e Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 8 Sep 2022 16:40:42 -0700 Subject: [PATCH 04/12] Remove RNG seeding with CIRQ_TESTING_RANDOM_SEED Replaced by pytest-randomly. The seed from pytest-randomly appears to be in effect at the test discovery stage and makes RNG-generated test parameters reproducible. --- check/pytest | 3 --- check/pytest-changed-files | 3 --- conftest.py | 7 ------- 3 files changed, 13 deletions(-) diff --git a/check/pytest b/check/pytest index 1c247a74e3f..ae6de691695 100755 --- a/check/pytest +++ b/check/pytest @@ -41,9 +41,6 @@ if [ "$PYTHON_VERSION" -lt "37" ]; then PYTEST_ARGS+=("--ignore=cirq-rigetti") fi -: ${CIRQ_TESTING_RANDOM_SEED=$(git log -1 --pretty="%ct")} -export CIRQ_TESTING_RANDOM_SEED - if [ -z "${ACTUALLY_QUIET}" ]; then pytest "${PYTEST_ARGS[@]}" RESULT=$? diff --git a/check/pytest-changed-files b/check/pytest-changed-files index 75e19fa0f44..f7dbacd5b94 100755 --- a/check/pytest-changed-files +++ b/check/pytest-changed-files @@ -77,7 +77,4 @@ fi source dev_tools/pypath -: ${CIRQ_TESTING_RANDOM_SEED=$(git log -1 --pretty="%ct")} -export CIRQ_TESTING_RANDOM_SEED - pytest ${rest} "${changed[@]}" diff --git a/conftest.py b/conftest.py index d2a5475c392..5b9029a4216 100644 --- a/conftest.py +++ b/conftest.py @@ -55,10 +55,3 @@ def closefigures(): yield plt.close('all') - - -# skip seeding for unset or empty CIRQ_TESTING_RANDOM_SEED -if numpy is not None and os.environ.get('CIRQ_TESTING_RANDOM_SEED'): - rngseed = int(os.environ['CIRQ_TESTING_RANDOM_SEED']) - random.seed(rngseed) - numpy.random.seed(rngseed) From fac9738202875603ebaba4099f1084e356a5c9a9 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 8 Sep 2022 16:58:15 -0700 Subject: [PATCH 05/12] Clean up unused imports in conftest.py --- conftest.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/conftest.py b/conftest.py index 5b9029a4216..60747c65305 100644 --- a/conftest.py +++ b/conftest.py @@ -12,18 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -import os -import random - import pytest -# allow CI execution of isolated_packages_test.py without numpy -try: - import numpy -except ImportError: - # coverage: ignore - numpy = None - def pytest_configure(config): # Ignore deprecation warnings in python code generated from our protobuf definitions. From 13890c62815188116fa57b9432cd99fd5036e175 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 8 Sep 2022 18:02:22 -0700 Subject: [PATCH 06/12] Remove check/pytest option --actually-quiet Avoid using `pytest --quiet` which disables pytest-randomly hook that configures consistent seeding for parallel test jobs. Reverts #1825. --- .github/workflows/ci.yml | 4 ++-- check/pytest | 26 ++++++-------------------- check/pytest-and-incremental-coverage | 3 +-- dev_tools/bash_scripts_test.py | 24 ++++++++---------------- 4 files changed, 17 insertions(+), 40 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 82b2025048a..47c637b6571 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -159,7 +159,7 @@ jobs: - name: Run Quil dependencies run: docker-compose -f cirq-rigetti/docker-compose.test.yaml up -d - name: Pytest check - run: check/pytest -n auto --ignore=cirq-core/cirq/contrib --actually-quiet --rigetti-integration + run: check/pytest -n auto --ignore=cirq-core/cirq/contrib --rigetti-integration - name: Stop Quil dependencies run: docker-compose -f cirq-rigetti/docker-compose.test.yaml down build_docs: @@ -240,7 +240,7 @@ jobs: - name: Pytest Windows run: | source dev_tools/pypath - check/pytest -n auto --ignore=cirq-core/cirq/contrib --actually-quiet + check/pytest -n auto --ignore=cirq-core/cirq/contrib shell: bash macos: name: Pytest MacOS diff --git a/check/pytest b/check/pytest index ae6de691695..8e446a7367f 100755 --- a/check/pytest +++ b/check/pytest @@ -4,9 +4,7 @@ # Runs pytest on the repository. # # Usage: -# check/pytest [--actually-quiet] [--flags for pytest] [file-paths-relative-to-repo-root] -# -# The --actually-quiet argument filters out any progress output from pytest. +# check/pytest [--flags for pytest] [file-paths-relative-to-repo-root] # # You may specify pytest flags and specific files to test. The file paths # must be relative to the repository root. If no files are specified, everything @@ -18,17 +16,12 @@ cd "$(dirname "${BASH_SOURCE[0]}")" cd "$(git rev-parse --show-toplevel)" PYTEST_ARGS=() -ACTUALLY_QUIET="" PARALLEL="" for arg in "$@"; do - if [[ "${arg}" == "--actually-quiet" ]]; then - ACTUALLY_QUIET=1 - else - if [[ "${arg}" == "-n" ]]; then - PARALLEL=1 - fi - PYTEST_ARGS+=("${arg}") + if [[ "${arg}" == "-n" ]]; then + PARALLEL=1 fi + PYTEST_ARGS+=("${arg}") done if [ -z "${PARALLEL}" ]; then @@ -41,15 +34,8 @@ if [ "$PYTHON_VERSION" -lt "37" ]; then PYTEST_ARGS+=("--ignore=cirq-rigetti") fi -if [ -z "${ACTUALLY_QUIET}" ]; then - pytest "${PYTEST_ARGS[@]}" - RESULT=$? -else - # Filter out lines like "...F....x... [ 42%]", with coloring. - pytest -q --color=yes "${PYTEST_ARGS[@]}" | - perl -nle'print if not m{^(.\[0m)?[\.FEsx]+(.\[36m)?\s+\[\s*\d+%\](.\[0m)?$}' - RESULT=${PIPESTATUS[0]} -fi +pytest "${PYTEST_ARGS[@]}" +RESULT=$? if [ "$RESULT" = 5 ]; then echo "[exit 5] No tests collected, but ignored." diff --git a/check/pytest-and-incremental-coverage b/check/pytest-and-incremental-coverage index c9684033f16..abbb05f2a70 100755 --- a/check/pytest-and-incremental-coverage +++ b/check/pytest-and-incremental-coverage @@ -71,8 +71,7 @@ fi source dev_tools/pypath # Run tests while producing coverage files. -check/pytest --actually-quiet \ - --cov \ +check/pytest --cov \ --cov-config=dev_tools/conf/.coveragerc \ "${PYTEST_ARGS[@]}" pytest_result=$? diff --git a/dev_tools/bash_scripts_test.py b/dev_tools/bash_scripts_test.py index 7090b76a9ec..792bd582907 100644 --- a/dev_tools/bash_scripts_test.py +++ b/dev_tools/bash_scripts_test.py @@ -349,8 +349,7 @@ def test_pytest_and_incremental_coverage_branch_selection(tmpdir_factory): assert result.returncode == 0 assert result.stdout == ( 'INTERCEPTED check/pytest ' - '--actually-quiet --cov ' - '--cov-config=dev_tools/conf/.coveragerc\n' + '--cov --cov-config=dev_tools/conf/.coveragerc\n' 'The annotate command will be removed in a future version.\n' 'Get in touch if you still use it: ned@nedbatchelder.com\n' 'No data to report.\n' @@ -376,8 +375,7 @@ def test_pytest_and_incremental_coverage_branch_selection(tmpdir_factory): assert result.returncode == 0 assert result.stdout == ( 'INTERCEPTED check/pytest ' - '--actually-quiet --cov ' - '--cov-config=dev_tools/conf/.coveragerc\n' + '--cov --cov-config=dev_tools/conf/.coveragerc\n' 'The annotate command will be removed in a future version.\n' 'Get in touch if you still use it: ned@nedbatchelder.com\n' 'No data to report.\n' @@ -395,8 +393,7 @@ def test_pytest_and_incremental_coverage_branch_selection(tmpdir_factory): assert result.returncode == 0 assert result.stdout == ( 'INTERCEPTED check/pytest ' - '--actually-quiet --cov ' - '--cov-config=dev_tools/conf/.coveragerc\n' + '--cov --cov-config=dev_tools/conf/.coveragerc\n' 'The annotate command will be removed in a future version.\n' 'Get in touch if you still use it: ned@nedbatchelder.com\n' 'No data to report.\n' @@ -414,8 +411,7 @@ def test_pytest_and_incremental_coverage_branch_selection(tmpdir_factory): assert result.returncode == 0 assert result.stdout == ( 'INTERCEPTED check/pytest ' - '--actually-quiet --cov ' - '--cov-config=dev_tools/conf/.coveragerc\n' + '--cov --cov-config=dev_tools/conf/.coveragerc\n' 'The annotate command will be removed in a future version.\n' 'Get in touch if you still use it: ned@nedbatchelder.com\n' 'No data to report.\n' @@ -433,8 +429,7 @@ def test_pytest_and_incremental_coverage_branch_selection(tmpdir_factory): assert result.returncode == 0 assert result.stdout == ( 'INTERCEPTED check/pytest ' - '--actually-quiet --cov ' - '--cov-config=dev_tools/conf/.coveragerc\n' + '--cov --cov-config=dev_tools/conf/.coveragerc\n' 'The annotate command will be removed in a future version.\n' 'Get in touch if you still use it: ned@nedbatchelder.com\n' 'No data to report.\n' @@ -464,8 +459,7 @@ def test_pytest_and_incremental_coverage_branch_selection(tmpdir_factory): assert result.returncode == 0 assert result.stdout == ( 'INTERCEPTED check/pytest ' - '--actually-quiet --cov ' - '--cov-config=dev_tools/conf/.coveragerc\n' + '--cov --cov-config=dev_tools/conf/.coveragerc\n' 'The annotate command will be removed in a future version.\n' 'Get in touch if you still use it: ned@nedbatchelder.com\n' 'No data to report.\n' @@ -483,8 +477,7 @@ def test_pytest_and_incremental_coverage_branch_selection(tmpdir_factory): assert result.returncode == 0 assert result.stdout == ( 'INTERCEPTED check/pytest ' - '--actually-quiet --cov ' - '--cov-config=dev_tools/conf/.coveragerc\n' + '--cov --cov-config=dev_tools/conf/.coveragerc\n' 'The annotate command will be removed in a future version.\n' 'Get in touch if you still use it: ned@nedbatchelder.com\n' 'No data to report.\n' @@ -509,8 +502,7 @@ def test_pytest_and_incremental_coverage_branch_selection(tmpdir_factory): assert result.returncode == 0 assert result.stdout.startswith( 'INTERCEPTED check/pytest ' - '--actually-quiet --cov ' - '--cov-config=dev_tools/conf/.coveragerc\n' + '--cov --cov-config=dev_tools/conf/.coveragerc\n' 'The annotate command will be removed in a future version.\n' 'Get in touch if you still use it: ned@nedbatchelder.com\n' 'No data to report.\n' From c39fa19cf603e4fbe80d241caa326a6c48a629e6 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 8 Sep 2022 18:41:50 -0700 Subject: [PATCH 07/12] Temporarily disable test reordering with pytest-randomly --- check/pytest | 3 +++ 1 file changed, 3 insertions(+) diff --git a/check/pytest b/check/pytest index 8e446a7367f..aabf036dd1c 100755 --- a/check/pytest +++ b/check/pytest @@ -16,6 +16,9 @@ cd "$(dirname "${BASH_SOURCE[0]}")" cd "$(git rev-parse --show-toplevel)" PYTEST_ARGS=() +# TODO(juhas) - remove this after fixing order-related test failures +PYTEST_ARGS+=("--randomly-dont-reorganize") + PARALLEL="" for arg in "$@"; do if [[ "${arg}" == "-n" ]]; then From 34dbb44e2e5f51e429907a0a6d6c91a6c00d3554 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Thu, 8 Sep 2022 19:49:01 -0700 Subject: [PATCH 08/12] Link TODO comment to the associated issue #5870 --- check/pytest | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/check/pytest b/check/pytest index aabf036dd1c..035912074e4 100755 --- a/check/pytest +++ b/check/pytest @@ -16,7 +16,7 @@ cd "$(dirname "${BASH_SOURCE[0]}")" cd "$(git rev-parse --show-toplevel)" PYTEST_ARGS=() -# TODO(juhas) - remove this after fixing order-related test failures +# TODO(#5870) - remove this after fixing order-related test failures PYTEST_ARGS+=("--randomly-dont-reorganize") PARALLEL="" From 8953f15f4114eccdb298c74ab7187ef3a5609e84 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Mon, 12 Sep 2022 14:21:40 -0700 Subject: [PATCH 09/12] Temporarily disable test reordering in Isolated pytest Ubuntu --- .github/workflows/ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 47c637b6571..e74232ee4f6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -135,7 +135,8 @@ jobs: - name: Install dependencies run: pip install -r dev_tools/requirements/isolated-base.env.txt - name: Test each module in isolation - run: pytest -n auto -m slow dev_tools/packaging/isolated_packages_test.py + # TODO(#5870) - remove --randomly-dont-reorganize after fixing order-related test failures + run: pytest --randomly-dont-reorganize -n auto -m slow dev_tools/packaging/isolated_packages_test.py pytest: name: Pytest Ubuntu strategy: From b32ec5782fa07d495e6af421642d4e66b411bb89 Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Wed, 14 Sep 2022 16:32:58 -0700 Subject: [PATCH 10/12] Wake up GitHub CI workflows! From c5d273530effcaa8903bd613fac772cd7a9f7b4a Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Wed, 14 Sep 2022 18:29:34 -0700 Subject: [PATCH 11/12] Restore RNG seeding where required These tests are tied to the zero seed of numpy RNG. --- .../controlled_gate_decomposition_test.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cirq-core/cirq/transformers/analytical_decompositions/controlled_gate_decomposition_test.py b/cirq-core/cirq/transformers/analytical_decompositions/controlled_gate_decomposition_test.py index 0756e860a50..7abf0c6e1f2 100644 --- a/cirq-core/cirq/transformers/analytical_decompositions/controlled_gate_decomposition_test.py +++ b/cirq-core/cirq/transformers/analytical_decompositions/controlled_gate_decomposition_test.py @@ -110,6 +110,7 @@ def _decomposition_size(U, controls_count): def test_decompose_size_special_unitary(): + np.random.seed(0) u = _random_special_unitary() assert _decomposition_size(u, 0) == (1, 0, 0) assert _decomposition_size(u, 1) == (3, 2, 0) @@ -122,6 +123,7 @@ def test_decompose_size_special_unitary(): def test_decompose_size_unitary(): + np.random.seed(0) u = _random_unitary() assert _decomposition_size(u, 0) == (1, 0, 0) assert _decomposition_size(u, 1) == (4, 2, 0) From 07b2c3637499e8947e0f1754e731f21550ad3d2a Mon Sep 17 00:00:00 2001 From: Pavol Juhas Date: Wed, 14 Sep 2022 22:26:47 -0700 Subject: [PATCH 12/12] Temporarily disable test shuffling in test_isolated_packages for real --- dev_tools/packaging/isolated_packages_test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dev_tools/packaging/isolated_packages_test.py b/dev_tools/packaging/isolated_packages_test.py index 7532fd5586b..d7303c99369 100644 --- a/dev_tools/packaging/isolated_packages_test.py +++ b/dev_tools/packaging/isolated_packages_test.py @@ -48,8 +48,9 @@ def test_isolated_packages(cloned_env, module): ) assert result.returncode == 0, f"Failed to install {module.name}:\n{result.stderr}" + # TODO(#5870) - remove --randomly-dont-reorganize after fixing order-related test failures result = shell_tools.run( - f"{env}/bin/pytest ./{module.root} --ignore ./cirq-core/cirq/contrib".split(), + f"{env}/bin/pytest --randomly-dont-reorganize ./{module.root} --ignore ./cirq-core/cirq/contrib".split(), capture_output=True, check=False, )