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

Add a decorator for the tests for checking c++ simulator availability #662

Closed
diego-plan9 opened this issue Jul 18, 2018 · 6 comments · Fixed by #689
Closed

Add a decorator for the tests for checking c++ simulator availability #662

diego-plan9 opened this issue Jul 18, 2018 · 6 comments · Fixed by #689
Labels
good first issue Good for newcomers type: qa Issues and PRs that relate to testing and code quality

Comments

@diego-plan9
Copy link
Member

What is the expected enhancement?

In a number of test files in the test/python/ folder, there is a check at the top of the file for the existence of the C++ simulator: if it is not present, some tests are skipped. In order to avoid duplication and for consistency, it would be nice to have a decorator in test/python/common.py that we can reuse!

Good first contribution

This would actually be a first good contribution, as the tests are a good stepping stone for looking into the internals of qiskit-terra if desired, and at the same time the changes should be quite self-contained indeed. It also includes exploring a bit Python decorators, which are a powerful mechanism and rather fun actually!

If intending to tackle this issue, please reply in this issue before starting the implementation, and feel free to discuss and suggest other alternatives! 🎉

Suggested implementation

This is just a proposal, there might be better ways - part of the issue is finding the best way indeed and discuss the solutions!

  1. Look for the blocks in the test files that look similar to:

    try:
        cpp_backend = QasmSimulatorCpp()
    except FileNotFoundError:
        _skip_cpp = True
    else:
        _skip_cpp = False

    This is the check that is used for determining if the C++ simulator is available.

  2. Create a new function in test/python/common.py, named for example requires_cpp_simulator(). In order to make it a decorator, probably the most convenient implementation is via making use of functools.wraps().
    A good inspiration is the slow_test() function in the same file: the behaviour of your decorator will be pretty similar, raising a unittest.SkipTest if the c++ simulator is not available, and returning the original function otherwise.

  3. In the files where the try blocks from 1) where found, remove the try blocks and instead append the @requires_cpp_simulator decorator to the class (if all tests need to be skipped) or to the individual tests that need it:

    @requires_cpp_simulator
    class TestSomething(QiskitTestCase):
        ...
@diego-plan9 diego-plan9 added good first issue Good for newcomers type: qa Issues and PRs that relate to testing and code quality labels Jul 18, 2018
@srujanm
Copy link
Contributor

srujanm commented Jul 24, 2018

Hi @diego-plan9, I would like to work on this issue. As you suggested, implementing it along the lines of the existing slow_test() decorator in test/python/common.py seems like the most straightforward way to do this.

A string search in test/python showed me seven files in which the existence of the C++ simulator is checked. I can use the decorator in all of these, confirm they run ok and then issue a PR.

@diego-plan9
Copy link
Member Author

Wonderful, @srujanm !

Yes, the number of files to be changed (~7) seems about right - we can double check during the review of the PR, as outlined in the contribution guidelines. Looking forward to the upcoming PR, and thanks for tackling this issue - don't hesitate on commenting on this issue or on the PR for more clarifications or anything we can help with.

@srujanm
Copy link
Contributor

srujanm commented Jul 25, 2018

Hi @diego-plan9, I am almost done with this decorator implementation. There is just one error I am running into that is independent of the decorator.

When I run test/python/test_extensions_simulator.py, the test_noise function fails with the C++ simulator crashing. See full error trace below, which seems exactly identical to the one in issue #537. From the discussion on that thread, this may also be some sort of numerical issue? Before digging deeper, I wanted to quickly run it by you in case you have an easy fix.

I am working with

  • current qiskit-terra master branch
  • Python 3.6.2
  • mingw-w64 5.0.3
  • Windows 10 Pro
======================================================================
ERROR: test_noise (test.python.test_extensions_simulator.TestExtensionsSimulator)
turn on a pauli x noise for qubits 0 and 2
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\sru\qiskit-terra\test\python\common.py", line 257, in _
    return func(*args, **kwargs)
  File "C:\Users\sru\qiskit-terra\test\python\test_extensions_simulator.py", line 89, in test_noise
    result = execute(circ, sim, config=config, shots=shots).result()
  File "C:\Users\sru\qiskit-terra\qiskit\backends\local\localjob.py", line 57, in result
    return self._future.result(timeout=timeout)
  File "C:\Users\sru\AppData\Local\conda\conda\envs\QISKitenv\lib\concurrent\futures\_base.py", line 405, in result
    return self.__get_result()
  File "C:\Users\sru\AppData\Local\conda\conda\envs\QISKitenv\lib\concurrent\futures\_base.py", line 357, in __get_result
    raise self._exception
  File "C:\Users\sru\AppData\Local\conda\conda\envs\QISKitenv\lib\concurrent\futures\thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "C:\Users\sru\qiskit-terra\qiskit\backends\local\qasm_simulator_cpp.py", line 80, in _run_job
    result = run(qobj, self._configuration['exe'])
  File "C:\Users\sru\qiskit-terra\qiskit\backends\local\qasm_simulator_cpp.py", line 227, in run
    return json.loads(sim_output, cls=QASMSimulatorDecoder)
  File "C:\Users\sru\AppData\Local\conda\conda\envs\QISKitenv\lib\json\__init__.py", line 367, in loads
    return cls(**kw).decode(s)
  File "C:\Users\sru\AppData\Local\conda\conda\envs\QISKitenv\lib\json\decoder.py", line 339, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "C:\Users\sru\AppData\Local\conda\conda\envs\QISKitenv\lib\json\decoder.py", line 357, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

----------------------------------------------------------------------
Ran 3 tests in 2.111s

FAILED (errors=1)

@diego-plan9
Copy link
Member Author

When I run test/python/test_extensions_simulator.py, the test_noise function fails with the C++ simulator crashing.

Hmm, can you specify if you are running into the error in master, or in the branch that contains your upcoming changes for the PR?

If it is the first case, please do temporarily ignore it in your local tests, as it should be something to be handled in issue #537 (although that test seems to be passing both for linux and for osx https://travis-ci.org/Qiskit/qiskit-terra/jobs/408399777#L971 at least). If it is the second case and you are only experiencing it in the branch with your changes, we can better take a look at it during the actual PR review, once you create it - this way we can try to reproduce it more easily.

srujanm added a commit to srujanm/qiskit-terra that referenced this issue Jul 26, 2018
* Added two functions that check for C++ simulator availability

* Replaced repetitive code in test files with these functions
@srujanm
Copy link
Contributor

srujanm commented Jul 26, 2018

Hey @diego-plan9,

I saw the error on the master branch.

I switched to linux though, and confirmed that both master and the branch for my PR pass that test. Also just put up the PR :)

srujanm added a commit to srujanm/qiskit-terra that referenced this issue Jul 30, 2018
* Function is now just the unittest skipIf decorator
* It acts on both methods and classes
delapuente pushed a commit that referenced this issue Aug 2, 2018
…689)

* Added decorator to check for C++ simulator availability (#662)
* Added two functions that check for C++ simulator availability
@delapuente
Copy link
Contributor

Fixed by #689

lia-approves pushed a commit to edasgupta/qiskit-terra that referenced this issue Jul 30, 2019
…ility (Qiskit#689)

* Added decorator to check for C++ simulator availability (Qiskit#662)
* Added two functions that check for C++ simulator availability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants