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

QuantumCircuit.save_statevector: Inform user when Aer backend is not initialized #8359

Closed
wants to merge 1 commit into from

Conversation

rht
Copy link
Contributor

@rht rht commented Jul 18, 2022

Summary

The QuantumCircuit class doesn't have the save_statevector until it is monkey-patched during the Aer backend initialization. Adding a placeholder method (by this PR) gives an informative error message.

Fixes #8325. (And maybe #6346)

@rht rht requested a review from a team as a code owner July 18, 2022 04:53
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rht rht force-pushed the default_save_statevector branch 5 times, most recently from c37ed25 to 031424b Compare July 18, 2022 10:57
@rht
Copy link
Contributor Author

rht commented Jul 18, 2022

I'm not sure why the test said that QiskitError was not raised. I tested by manually adding the save_statevector() method to a local installed version of qiskit-terra (from PyPI), and I got the expected QiskitError. Later, I will prepare an environment to actually build from Git from scratch.

@rht rht force-pushed the default_save_statevector branch from 031424b to 61f0913 Compare July 29, 2022 04:20
@rht
Copy link
Contributor Author

rht commented Jul 29, 2022

I have set up a fresh Qiskit Terra from my Git branch on Docker environment (using the Rust official Docker image, tagged 1-bullseye). I used Python 3.7.13, as it is the one that was failing in the CI.
However, the test ran successfully

$ tox -epy37 -- -n test.python.circuit.test_initializer.TestInitialize.test_save_statevector
...
{0} test.python.circuit.test_initializer.TestInitialize.test_save_statevector [0.005662s] ... ok

I can't reproduce the CI failure.

@Cryoris
Copy link
Contributor

Cryoris commented Jul 29, 2022

Maybe that's because the CI does have Aer installed and never calls the method you added 🤔

@rht
Copy link
Contributor Author

rht commented Jul 29, 2022

That could be the cause. The tests are likely not run in isolation (even though they are parallelized with stestr), and in one of the tests, the Aer backend is initialized, before test_save_statevector is executed.

@rht
Copy link
Contributor Author

rht commented Jul 29, 2022

I misread your statement. You meant that I don't have qiskit-aer installed in my fresh setup. I tried pip install qiskit-aer, and then did

tox -epy37 -- -n test.python.circuit.test_initializer.TestInitialize

so that test_save_statevector is not the only one being tested, nor that it is the first one. No parallelization. I still got no error.

I'm testing the full test stestr run next.

@rht
Copy link
Contributor Author

rht commented Jul 29, 2022

I ran stestr run on my laptop. But it crashed my system with OOM. I was able to capture this error

Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):

      File "/moun/test/python/circuit/test_initializer.py", line 85, in test_save_statevector
    self.assertRaises(QiskitError, qc.save_statevector)

      File "/root/.pyenv/versions/3.7.13/lib/python3.7/unittest/case.py", line 756, in assertRaises
    return context.handle('assertRaises', args, kwargs)

      File "/root/.pyenv/versions/3.7.13/lib/python3.7/unittest/case.py", line 178, in handle
    callable_obj(*args, **kwargs)

      File "/root/.pyenv/versions/3.7.13/lib/python3.7/unittest/case.py", line 201, in __exit__
    self.obj_name))

      File "/root/.pyenv/versions/3.7.13/lib/python3.7/unittest/case.py", line 135, in _raiseFailure
    raise self.test_case.failureException(msg)

    AssertionError: QiskitError not raised by save_statevector

@rht rht force-pushed the default_save_statevector branch 5 times, most recently from c2896a6 to ea2d24e Compare July 30, 2022 07:08
@rht rht force-pushed the default_save_statevector branch from ea2d24e to 74f1fdb Compare July 30, 2022 07:37
@rht
Copy link
Contributor Author

rht commented Jul 30, 2022

I have tried undoing the monkey patch in my latest commit. It works locally with tox, but fails in CI via stestr.

There are 2 course of actions:

  1. Define original_save_statevector, and then define save_statevector as an alias to the former. Test only the former.
  2. Separate test_save_statevector to its own file and test separately. Though I doubt this will fix the issue, because the Python instance is long-running and doesn't get shut down for each file.

Should I do point 1?

@jakelishman
Copy link
Member

I'm not sure we should add this at all, sorry. Aer's design of monkey-patching methods onto QuantumCircuit is certainly not desirable, and it does lead to some weird error messages, but Terra shouldn't be in the business of correcting Aer's weirdnesses, because that's just bloating the Terra code. There's also a lot more methods than just save_statevector. Within the current monkey-patch setup, it would be better to document Aer-specific methods with a .. note:: in the built documentation (that would need to be done in the Aer repo), so it displays a big warning box letting people know.

Another reason to not have these messages is that it stymies code-completion workflows. If we add stubs that just throw errors, then you can't tell if save_<x> is available by normal IDE/Jupyter tab-completion, because we've stubbed it out with a method whose only purpose is to raise errors.

@rht
Copy link
Contributor Author

rht commented Aug 15, 2022

I agree with the save_statevector monkey patching being confusing. I got tripped by it and hence I made this PR.
This PR is not an ideal solution, but at least it is a temporary solution that will prevent people from having to dig the issues in search engine results / GH issues.

@jakelishman
Copy link
Member

jakelishman commented Aug 15, 2022

I don't believe there is a need for any modification to Terra here, temporary or not. A correct solution can be done immediately, and that's to add documentation to the relevant methods within Aer that says they can only be used once Aer is loaded. Thanks for the interest in contributing - if you'd like to take this further, I'd suggest making the documentation changes as a PR to Aer. There's a bit more discussion from another Terra maintainer here: #6346 (comment), from a while ago.

With the release of Aer 0.11, we're going to start moving away from the namespace packaging for Aer, which will help ease the confusion caused by the lazy-loading qiskit.Aer object - after import qiskit_aer, the methods will be immediately available. (That's not possible yet - we're still untangling the namespace packaging.)

I'm going to close this PR now, because there's no code changes necessary to Terra. Sorry about the delay between you opening it and this - I've been on leave for a month.

@rht rht deleted the default_save_statevector branch August 16, 2022 01:09
@rht
Copy link
Contributor Author

rht commented Aug 16, 2022

I appreciate that I finally received a feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants