-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 support on QuantumInstance/run_circuits for Backends that don't handle QasmQobj #6299
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just took a quick pass through it to start, overall lgtm just a couple of questions inline. As for the unit tests I agree they're good to have, but they won't be backportable (which is something we need to do here) because we can't backport #6286 (it's a breaking change for users of the mock backends). But, I guess it'll be simple enough to strip the tests in the backport so that's not an issue.
qiskit/utils/quantum_instance.py
Outdated
not is_aer_provider(self._backend) and \ | ||
not is_basicaer_provider(self._backend) and \ | ||
not is_ibmq_provider(self._backend): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about this, qobj support is dated and will be going away in ~<3months for the ibmq provider; for aer and basic aer it'll be around a bit longer, but not much. They're also all emitting warnings either a DeprecationWarning
or a PendingDeprecationWarning
which means anytime someone runs an algorithm on up-to-date versions of these backends we'll be emitting a warning for each job. But, I get that this is a workaround just to fix the bug for now. Maybe just expand on the comment here that we can't rely on Qobj for much longer even if the backends are from ibmq, aer, or basicaer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the ideal would be to just not use Qobj anymore however many things will change possibly including how AQGD works and the whole error mitigation logic. Excluding error mitigation, currently, if I use Aer without Qobj, I get several unit tests failures while the same pass using BasicAer without QObj.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect this is because when running against aer it expects to be able to hack the qobj up to pass a paramterized circuit, and the circuit interface to aer is missing equivalent functionality (it was hacked in aer as a custom path for aqua and not really exposed as real interface). We should open an issue on aer to add an alternative for passing in parameterized circuits and a list of bound values as a kwarg to run without needing qobj.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should open an issue on aer to add an alternative for passing in parameterized circuits and a list of bound values as a kwarg to run without needing qobj.
Actually it would be nice for for such a capability no matter the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well most backends don't have a method to natively run parameterized circuits over a parameter list like aer does, but in such cases the provider could just expand the circuits before sending them to the backend.
I think the place to define this would be in the next version of the backend interface (which I have a rfc/prototype up in #5885 )
I think its a question here of how much rework/fixing can be easily (sensibly) done as a bug fix. Mitigation logic is all written around qobj. qobj too is used with Aer and has dedicated logic for it to allow it to be reused across different parameterizations. #5717 was written up a while back with a view to re-doing the QI. With the runtime work it indeed needs this - the above patch, should allow newer backends to run albeit without noise mitigation for now. |
Sure I agree with this, and as I said before this is a fine compromise, except I think we should have the measurement mitigation in place, it's honestly not hard to do with circuits (it's basically identical you just skip the assemble here: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/utils/measurement_error_mitigation.py#L148 and it's basically all the same). There are 2 reasons I think this, first everyone keeps saying the builtin measurement mitigation is the reason the quantum instance exists as a standalone thing, so not supporting that seems to still be a problem, and second there is no user facing indication that measurement mitigation isn't being run when you use a |
FYI QI existed before measurement mitigation. It was done to hold the settings etc for an algorithm running over multiple circuit executions and have a common place for such function so it was not done in each algorithm. Anyway thats somewhat irrelevant to the current situation. An external indication could simply be added to log a warning that measure mitigation is ignored, in the absence of it being supported for V1 backend for this fix. |
ac22589
to
4312086
Compare
36e4dcc
to
5964b72
Compare
Co-authored-by: Matthew Treinish <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, I found one small issue in the run_circuits
code but other than that I think it's good to go. One question (but I don't think it's worth blocking over), do we have any test coverage for the v1 interface with error mitigation? The tests added didn't seem to set it, but I wasn't sure.
Add check for providers that don't support it. Co-authored-by: Matthew Treinish <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the quick update.
…andle QasmQobj (#6299) * Support BackendV1 * Add error mitigation logic. Co-authored-by: Matthew Treinish <[email protected]> * Fix run_circuits calls * add unit tests * Update qiskit/utils/run_circuits.py Add check for providers that don't support it. Co-authored-by: Matthew Treinish <[email protected]> * Add error mitigation test Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit a0ed7ff)
2 of the tests added in #6299 depend on the fake backends in qiskit.test.mock being strict BackendV1 backends (that reject qobj) and also having other aspects of the interface implemented (mainly run kwargs). However the change which updated the fake backends to use BackendV1 (#6286) is not backportable as it's an API change, so these tests are runnable in stable/0.17. This commit just removes these tests because even if we could get them running without #6286 we wouldn't be actually testing the v1 interface (which is their purpose).
…andle QasmQobj (backport #6299) (#6343) * Add support on QuantumInstance/run_circuits for Backends that don't handle QasmQobj (#6299) * Support BackendV1 * Add error mitigation logic. Co-authored-by: Matthew Treinish <[email protected]> * Fix run_circuits calls * add unit tests * Update qiskit/utils/run_circuits.py Add check for providers that don't support it. Co-authored-by: Matthew Treinish <[email protected]> * Add error mitigation test Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit a0ed7ff) * Remove tests that depend on BackendV1 fake backends 2 of the tests added in #6299 depend on the fake backends in qiskit.test.mock being strict BackendV1 backends (that reject qobj) and also having other aspects of the interface implemented (mainly run kwargs). However the change which updated the fake backends to use BackendV1 (#6286) is not backportable as it's an API change, so these tests are runnable in stable/0.17. This commit just removes these tests because even if we could get them running without #6286 we wouldn't be actually testing the v1 interface (which is their purpose). * Fix lint Co-authored-by: Manoel Marques <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
In Qiskit#6299 support was fixed for strict BackendV1 backends that only take QuantumCircuit objects (instead of qobj) for the input. That was fixed by adding a parallel path when running with a new backend. However that parallel path wasn't identical and was missing the support the qobj path had for splitting an algorithm run into multiple jobs if the backend if the number of circuits was greater than the max_experiments set in the backend. This would result on some providers' backends, such as ionq and aqt, both of which have max_experiments set to 1. This commit fixes this issue by splitting the circuits list into smaller sublists when the len(circuits) > max_experiments (or the old env var, which we should change the name of at some point).
* Respect max_experiments in QuantumInstance BackendV1 path In #6299 support was fixed for strict BackendV1 backends that only take QuantumCircuit objects (instead of qobj) for the input. That was fixed by adding a parallel path when running with a new backend. However that parallel path wasn't identical and was missing the support the qobj path had for splitting an algorithm run into multiple jobs if the backend if the number of circuits was greater than the max_experiments set in the backend. This would result on some providers' backends, such as ionq and aqt, both of which have max_experiments set to 1. This commit fixes this issue by splitting the circuits list into smaller sublists when the len(circuits) > max_experiments (or the old env var, which we should change the name of at some point). * Fix issues with results and split circuits path * Fix copy paste issue * Update qiskit/utils/run_circuits.py * Add release notes * Fix whitespace Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: Manoel Marques <[email protected]>
* Respect max_experiments in QuantumInstance BackendV1 path In #6299 support was fixed for strict BackendV1 backends that only take QuantumCircuit objects (instead of qobj) for the input. That was fixed by adding a parallel path when running with a new backend. However that parallel path wasn't identical and was missing the support the qobj path had for splitting an algorithm run into multiple jobs if the backend if the number of circuits was greater than the max_experiments set in the backend. This would result on some providers' backends, such as ionq and aqt, both of which have max_experiments set to 1. This commit fixes this issue by splitting the circuits list into smaller sublists when the len(circuits) > max_experiments (or the old env var, which we should change the name of at some point). * Fix issues with results and split circuits path * Fix copy paste issue * Update qiskit/utils/run_circuits.py * Add release notes * Fix whitespace Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: Manoel Marques <[email protected]> (cherry picked from commit 9d4bb91) # Conflicts: # qiskit/utils/run_circuits.py # test/python/algorithms/test_backendv1.py
…6391) (#6414) * Respect max_experiments in QuantumInstance BackendV1 path (#6391) * Respect max_experiments in QuantumInstance BackendV1 path In #6299 support was fixed for strict BackendV1 backends that only take QuantumCircuit objects (instead of qobj) for the input. That was fixed by adding a parallel path when running with a new backend. However that parallel path wasn't identical and was missing the support the qobj path had for splitting an algorithm run into multiple jobs if the backend if the number of circuits was greater than the max_experiments set in the backend. This would result on some providers' backends, such as ionq and aqt, both of which have max_experiments set to 1. This commit fixes this issue by splitting the circuits list into smaller sublists when the len(circuits) > max_experiments (or the old env var, which we should change the name of at some point). * Fix issues with results and split circuits path * Fix copy paste issue * Update qiskit/utils/run_circuits.py * Add release notes * Fix whitespace Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: Manoel Marques <[email protected]> (cherry picked from commit 9d4bb91) # Conflicts: # qiskit/utils/run_circuits.py # test/python/algorithms/test_backendv1.py * Fix merge conflicts This commit fixes the merge conflicts from backporting #6391 to stable/0.17. The tests added in #6391 are just removed because they can't actually run without #6286 which isn't backportable. * Fix lint Co-authored-by: Matthew Treinish <[email protected]>
…andle QasmQobj (Qiskit#6299) * Support BackendV1 * Add error mitigation logic. Co-authored-by: Matthew Treinish <[email protected]> * Fix run_circuits calls * add unit tests * Update qiskit/utils/run_circuits.py Add check for providers that don't support it. Co-authored-by: Matthew Treinish <[email protected]> * Add error mitigation test Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Respect max_experiments in QuantumInstance BackendV1 path In Qiskit#6299 support was fixed for strict BackendV1 backends that only take QuantumCircuit objects (instead of qobj) for the input. That was fixed by adding a parallel path when running with a new backend. However that parallel path wasn't identical and was missing the support the qobj path had for splitting an algorithm run into multiple jobs if the backend if the number of circuits was greater than the max_experiments set in the backend. This would result on some providers' backends, such as ionq and aqt, both of which have max_experiments set to 1. This commit fixes this issue by splitting the circuits list into smaller sublists when the len(circuits) > max_experiments (or the old env var, which we should change the name of at some point). * Fix issues with results and split circuits path * Fix copy paste issue * Update qiskit/utils/run_circuits.py * Add release notes * Fix whitespace Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: Manoel Marques <[email protected]>
…andle QasmQobj (Qiskit/qiskit#6299) * Support BackendV1 * Add error mitigation logic. Co-authored-by: Matthew Treinish <[email protected]> * Fix run_circuits calls * add unit tests * Update qiskit/utils/run_circuits.py Add check for providers that don't support it. Co-authored-by: Matthew Treinish <[email protected]> * Add error mitigation test Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…kit#6391) * Respect max_experiments in QuantumInstance BackendV1 path In Qiskit/qiskit#6299 support was fixed for strict BackendV1 backends that only take QuantumCircuit objects (instead of qobj) for the input. That was fixed by adding a parallel path when running with a new backend. However that parallel path wasn't identical and was missing the support the qobj path had for splitting an algorithm run into multiple jobs if the backend if the number of circuits was greater than the max_experiments set in the backend. This would result on some providers' backends, such as ionq and aqt, both of which have max_experiments set to 1. This commit fixes this issue by splitting the circuits list into smaller sublists when the len(circuits) > max_experiments (or the old env var, which we should change the name of at some point). * Fix issues with results and split circuits path * Fix copy paste issue * Update qiskit/utils/run_circuits.py * Add release notes * Fix whitespace Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: Manoel Marques <[email protected]>
…andle QasmQobj (Qiskit/qiskit#6299) * Support BackendV1 * Add error mitigation logic. Co-authored-by: Matthew Treinish <[email protected]> * Fix run_circuits calls * add unit tests * Update qiskit/utils/run_circuits.py Add check for providers that don't support it. Co-authored-by: Matthew Treinish <[email protected]> * Add error mitigation test Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…kit#6391) * Respect max_experiments in QuantumInstance BackendV1 path In Qiskit/qiskit#6299 support was fixed for strict BackendV1 backends that only take QuantumCircuit objects (instead of qobj) for the input. That was fixed by adding a parallel path when running with a new backend. However that parallel path wasn't identical and was missing the support the qobj path had for splitting an algorithm run into multiple jobs if the backend if the number of circuits was greater than the max_experiments set in the backend. This would result on some providers' backends, such as ionq and aqt, both of which have max_experiments set to 1. This commit fixes this issue by splitting the circuits list into smaller sublists when the len(circuits) > max_experiments (or the old env var, which we should change the name of at some point). * Fix issues with results and split circuits path * Fix copy paste issue * Update qiskit/utils/run_circuits.py * Add release notes * Fix whitespace Co-authored-by: Kevin Krsulich <[email protected]> Co-authored-by: Manoel Marques <[email protected]>
Summary
Until a bigger QuantumInstance/run_circuits refactor happens, this handles backends that don't accept QasmQobj.
Details and comments
@mtreinish added to Quantum Instance the error mitigation logic to also support circuits.
Fixes #6280