Qiskit-terra: testsuite input should be deterministic

[1ucian0]

The use of the random module and test/python/_random_circuit_generator.py for creating input in the testsuite should be avoid. Ideally, the output of the tests in test/python should be deterministic and well define. I understand that, because the nature of QISKit, that's not always possible.

Expected Behavior

if I'm passing the tests locally, I should be quite confident that the same situation will hold in the future. Changing nothing and get tests failing is frustrating. Feed random input to tests is good and desired in a context of fuzzing. The random input can be moved to a different place with the goal of generating new tests.

Possible Solution

Remove as much as possible sources of non-determinism in the testsuite.

[1ucian0]

This list is outdated. Here is the new list https://github.com/Qiskit/qiskit-terra/issues/205#issuecomment-462579107

So far, the tests that depend on random are:

• test/python/test_projectq_cpp_simulator.py
• test/python/test_qi.py
• test/python/test_quantumprogram.py (trivial, random is used to generate a random hostname)
• test/python/test_mapper.py
• test/python/test_unitary_python_simulator.py
• test/python/test_qasm_python_simulator.py
• test/python/test_job_processor.py
• test/python/test_local_qiskit_simulator.py (the problem here looks deeper, in how the seed is consumed. Probably non-trivial.)

[1ucian0]

test/python/test_qi.py fixed by #258

[delapuente]

@1ucian0, does this overlap somehow with your efforts on #697? I mention it because we've discussed several times that the tests attacking the mock servers should be as deterministic as possible.

As a side note, avoiding the random module is a solution but finding a way of fixing the seed of the random functions could be another and it might require fewer changes.

[1ucian0]

This issue is about test files that call random as part of the test. Situations like the one in test/python/test_quantumprogram.py:

    def test_offline(self):
        import string
        import random
        qp = QuantumProgram()
        FAKE_TOKEN = 'this_token_is_not_going_to_be_sent_nowhere'
        FAKE_URL = 'http://{0}.com'.format(
            ''.join(random.choice(string.ascii_lowercase) for _ in range(63))
        )
        # SDK will throw ConnectionError on every call that implies a connection
        self.assertRaises(QISKitError, qp.set_api, FAKE_TOKEN, FAKE_URL)

The list of file is outdated tho. I can have a new look to it to see if this is still relevant.

[delapuente]

As far as I know, all the random functions depend on the same random generator so if we get some way of restoring the seed before each test, we won't remove the randomness but we can ensure it is deterministic from run to run.

[yaelbh]

We should not fix the random seed in tests that rely on asserts. Namely tests that pass for any random seed, as long as the code is correct. The random seed should be fixed in the testsuite only for tests that fail otherwise.

[1ucian0]

A reminder, we are talking about random inserted just for testing, not about the intrinsic random used by Terra.

@delapuente: Using only constant-seeded generators is equivalent of using constants directly.

@yaelbh I disagree. Making sure that the tests pass with a big variety of inputs it's the job of a fuzzer, not the test suite. If I write code and the test suite fails, I want to make sure it is because my new code broke something, not because in that particular execution of the test suite the input covered a different path of existing buggy code.

Fuzzing Terra is something definitely worth trying, and it is something I did in the past. Let me know if you are interested in this effort.

[delapuente]

Closed by error, reopening.

[1ucian0]

Updating the list:

• [ ] test/python/test_visualization.py:
  
  
  
  • The method TestLatexSourceGenerator.random_circuit generates a random circuit (used in test_tiny_circuit, test_normal_circuit, test_deep_circuit, and test_huge_circuit.
  
  
• [ ] test/python/test_apps.py:
  
  
  
  • Numpy random is called
  
  
• [ ] test/python/test_qasm_simulator_py.py:
  
  
  
  • The tests profile_qasm_simulator and profile_nqubit_speed_grow_depth (currently both of them skiped) uses RandomQasmGenerator.
  
  
• [ ] test/python/test_qi.py:
  
  
  
  • The library random_pauli is being used in test_random_pauli5.
  
  
• [ ] test/python/test_quaternions.py:
  
  
  
  • The test test_random_euler calls Numpy random.
  
  
• [ ] test/python/test_quantumprogram.py:
  
  
  
  • trivial, random is used to generate a random hostname
  
  
• [ ] test/python/test_unitary_simulator_py.py:
  
  
  
  • The test profile_unitary_simulator uses RandomQasmGenerator.
  
  

[ajavadia]

@1ucian0 can you update the list here if it's still relevant? many of these tests no longer exist.

[1ucian0]

Update:

• [ ] test/python/test_visualization.py: The method TestLatexSourceGenerator.random_circuit generates a random circuit (used in test_tiny_circuit, test_normal_circuit, test_deep_circuit, and test_huge_circuit.
• [x] test/python/quantum_info/test_quaternions.py: Calls Numpy random.
• [x] test/python//test_qi.py: Uses random_density_matrix, which does not have a way to set a seed.
• [x] test/python/quantum_info/test_states.py: Uses random_state, which does not have a way to set a seed.
• [x] test/python/quantum_info/test_pauli.py: Uses Pauli.random, which does not have a way to set a seed.

[1ucian0]

The visualization part will be fixed with a better version of https://github.com/Qiskit/qiskit-terra/pull/1617 (i.e. meta-test for visualizers)

[diego-plan9]

Reopening as #1800 was "partially fixing" this issue

[nonhermitian]

I have to say I disagree with this idea. It should be a mix of both. Experience shows that letting tests be random can identify edge cases, or donains of invalidity. Indeed, several swap mapper errors have been found via random circuit generation.

[yaelbh]

@nonhermitian Please see related comments https://github.com/Qiskit/qiskit-terra/issues/205#issuecomment-412488072 and https://github.com/Qiskit/qiskit-terra/issues/205#issuecomment-412510620.

[nonhermitian]

I have no idea what fuzzying is, but it sounds like a fancy name for randomized testing, which agrees with my point above.

I would also say that finding bugs via randomized teststing out weighs the difficulty in determining if a new piece of code caused the error or not. Given their random, and typically rare nature, they usually do not overlap with code that is actually modified in a PR, and are easy to spot. Fixing them tends to be the challenge.

[1ucian0]

The value of the test suite is in its reproducibility. If I modify code, and a test is not passing, there is something wrong with my modification. If a test fails _sometimes_, there is little information that I can extract from that fact.

In fuzzing, random inputs are provided to find the "edge cases" that you are referring in https://github.com/Qiskit/qiskit-terra/issues/205#issuecomment-463588187. If one of this randomly generated inputs breaks the invariant, the input is saved so you can reproduce the failure. Then you can add this input into the test suite as a new case.

We can have fuzzing testing and run it periodically (o continuously) to find new cases. However, we are going to need some parallel infrastructure for it. This infrastructure should have a way to extract the cases that are breaking code so we can add them as new cases. We are not currently doing this but I would support this effort.

[jaygambetta]

I agree with Paul. There are cases in quantum where we need random for a reason. To test all possible states is not feasible

[1ucian0]

I don't disagree with that. We need parallel infrastructure for it where we can preserve the reproducibility of the breaking state.