-
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
Fix QuantumInstance compatibility with BackendV2 #7563
Fix QuantumInstance compatibility with BackendV2 #7563
Conversation
This commit fixes compatibility with the QuantumInstance class when targetting a BackendV2 backend. When BackendV2 was introduced testing with the QuantumInstance (and therefore all of qiskit.algorithms) was neglected and there are several compatibility issues with the class around hard coded assumptions that the backend being wrapped was a BaseBackend or BackendV1 object.
The tests for the QuantumInstance with BackendV2 were just ported over from the equivalent testing with BackendV1. However, one of those tests was to test error mitigation with a custom noise model which requires qiskit-aer. We can't test this without aer having been migrated to BackendV2. Until qiskit-aer is backendv2 enabled this commit just removes the test.
Pull Request Test Coverage Report for Build 1780687845
💛 - Coveralls |
This is deprecated, and will be removed. But, in the meantime is it possible that it will be a |
This is supposed to be a kind of template class for testing. But it doesn't seem to be used in the qiskit-terra repo: However, if someone did use it and used a |
The following is apparently currently only used with basicaerprovider, so it won't cause an error. But, it is a utility that is not advertised to work only with V1. And |
This is in |
Apropos of Another option is to make a mixin class like: class BackendCompat:
@property
def _backend_name(self):
backend_version = _get_backend_version(self)
if backend_version <= 1:
return self.name()
else:
return self.name
@property
def _backend_provider(self):
backend_version = _get_backend_version(self)
if backend_version <= 1:
return self.provider()
else:
return self.provider , and add whatever other methods are useful. |
The QuantumInstance's is_simulator() and is_local() methods had duplicate checking on the backend version. This was an artifact of testing where in earlier revisions of this PR branch it was done in the QuantumInstance before being moved to the inner helper functions. Since the helper functions are updated to handle the version checking now we no longer need it in the QuantumInstance methods too. This commit removes this and just relies on the helper functions to do the version checking.
This one I don't think it matters, I think |
TBH, I hadn't even seen that before (or didn't remember it). I checked the only thing in repo using it is one (of several) basic aer test classes. We should just update that one basic aer test class to inline the test methods so we can handle it as part of the backend v2 migration for basicaer. I would honestly prefer to just delete it as part of #6862 but the ibmq provider might be using this class, since most of the weird exported base class stuff like this in terra was done for the ibmq provider originally. |
This commit attempts to clarify how the version fields are used by naming the local variables backend_interface_version to make it clear what we're checking in that context.
This commit fixes the check for whether we're running with a statevector simulator (to determine whether we need to insert save_statevector instructions) to be BackendV2 compatible. This isn't strictly needed yet as qiskit-aer is still based on BackendV1, but this would cause a bug when it does switch to the newer backend version. This commit just gets in front of that potential issue so we don't have a bug in the future.
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 have added some comments, mainly about the confusions between backend_version
and backend_interface_version
. Should we agree on the naming convention and make them consistent across related files? We can also open an issue about naming and fix them in other files in a separate PR.
name="FakeSimpleV2", | ||
description="A fake simple BackendV2 example", | ||
online_date=datetime.datetime.utcnow(), | ||
backend_version="0.0.1", |
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 am a bit confused. There seem to be two different backend_version
. One is the integer-only versioned backend which is later referenced as backend_interface_version
: 0
for LegacyBackend, 1
for BackendV1 and 2
for BackendV2. the other one is the backend_version
has SemVer format like 0.0.1
that comes from conf.json
from real backends? Am I right?
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.
Yes. It is explained pretty clearly in a docstring, but I forgot which and just now I was unable to locate it quickly. As I was digging through the code it became clear that this is what's happening even before I found the docstring. The semvar-ish version is for the particular backends, i.e. child classes. The integer version is for the parent classes of these.
It would be nice to use a completely different word than "version" for one of these, to avoid confusion. But, @mtreinish pointed out that that would require a deprecation.
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.
Are you thinking of the backend_version
entry in the BackendV1->V2 migration guide: https://qiskit.org/documentation/apidoc/providers.html#migrating-between-backend-api-versions it might be somewhere else too.
But yeah, @jlapeyre is correct as to the difference. The version
attribute is set in the abstract class definition as a class variable and is used to indicate which version of that interface a particular backend object implements. While the backend_version
attribute is arbitrary metadata that a backend can optionally supply when it's implemented to indicate any version information. The semver like string is just what IBM happen to uses for its backends (and that comes from the backend configuration json payload), but it can be any string as it's up to each backend. I agree the dual use of version can be confusing, but it is something that's not easy to change since this is part of a defined interface and it would require a deprecation cycle to change (and honestly I think it'll just be better to put it on the list for the inevitable BackendV3
and wait for that)
@@ -176,3 +180,40 @@ def qubit_properties(self, qubit): | |||
if isinstance(qubit, int): | |||
return self._qubit_properties[qubit] | |||
return [self._qubit_properties[i] for i in qubit] | |||
|
|||
|
|||
class FakeBackendSimple(BackendV2): |
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.
Is this class a temporary solution for while FakeBackendV2
doesn't have .run
method implemented? What prevents us to implemented .run
method to FakeBackendV2
now? Is it blocked by #7391 that I am working on?
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.
Yeah, this is just to have something until #7391 is implemented. But also because the goal of this PR is to fix support I wanted to have something smaller to backport than what #7391 will end up being. To test the quantum instance works with BackendV2 there needs to be a backend implementing the v2 interface with run()
defined. So I threw this together so we can have a minimal backend that does this.
There were a few straglers still using backend_version instead of backend_interface_version in the code. This commit updates all these instances so there is consistent naming around this inside the utils subpackage.
This commit fixes an oversight from an earlier development version that slipped through later revisions where the statevector simulator backend detection in the QuantumInstance class was hard coded to False for BackendV2. This was just meant as a temporary debugging step while getting the initial commit ready, but was never updated to remove this until now.
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.
a couple of questions, but nothing I'd hold up merge over
return backend.configuration().simulator | ||
backend_interface_version = _get_backend_interface_version(backend) | ||
if backend_interface_version <= 1: | ||
return backend.configuration().simulator | ||
return False |
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.
This function (is_simulator_backend
) doesn't look especially accurate, but I'm guessing we're not really bothered about that?
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 it's more that BackendV2
doesn't have a simulator
bool attribute in the same way as the BackendConfiguration
does. It was never really an actionable flag anyway, if simulator is set to True
what is a backend consumer to do with it?
That being said the code that uses this in the QuantumInstance
just makes incorrect assumptions about it being aer xor basicar if this returns True and either can use a noise model, and also disable retry logic (which also is similarly not-portable).
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.
Haha, let's just leave it as-is then.
return backend.configuration().local | ||
backend_interface_version = _get_backend_interface_version(backend) | ||
if backend_interface_version <= 1: | ||
return backend.configuration().local | ||
return False |
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.
(Similar question to is_simulator_backend
- the version 2+ check seems inaccurate, but we might not care)
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.
This is basically the same answer as is_simulator_backend
, BackendV2
doesn't have a local
attribute the same was as BackendConfiguration
. It's also not really user actionable, like what does a consumer of backend do with this information? It doesn't really change anything to know if it's a local backend vs a remote one. The closest thing in BackendV2 to this is that the JobV1
class has a provision for async or sync jobs which could be used to influence behavior.
That being said all this does in QuantumInstance
is to influence the max # of circuits per job (to fallback to max list size if it's local) because it's making an incorrect assumption that the local backend is basically aer or basic aer and that local backends always accept an arbitrary number of circuits per job. This doesn't make a difference in V2, because the .max_circuits
attribute is explicitly defined as returning None
if there is no limit to the number of circuits per job, regardless of whether it's None or not.
_get_backend_provider, | ||
_get_backend_interface_version, |
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.
If we're importing them elsewhere, they kind of ought to be public.
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.
It's mostly so we don't have to backport a new public interface. We can promote it on main if we want
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.
feel free to tag as automerge if you're not bothered about the comments.
* Fix QuantumInstance compatibility with BackendV2 This commit fixes compatibility with the QuantumInstance class when targetting a BackendV2 backend. When BackendV2 was introduced testing with the QuantumInstance (and therefore all of qiskit.algorithms) was neglected and there are several compatibility issues with the class around hard coded assumptions that the backend being wrapped was a BaseBackend or BackendV1 object. * Remove invalid test The tests for the QuantumInstance with BackendV2 were just ported over from the equivalent testing with BackendV1. However, one of those tests was to test error mitigation with a custom noise model which requires qiskit-aer. We can't test this without aer having been migrated to BackendV2. Until qiskit-aer is backendv2 enabled this commit just removes the test. * Remove unused imports * Remove duplicate backend version checks from QuantumInstance The QuantumInstance's is_simulator() and is_local() methods had duplicate checking on the backend version. This was an artifact of testing where in earlier revisions of this PR branch it was done in the QuantumInstance before being moved to the inner helper functions. Since the helper functions are updated to handle the version checking now we no longer need it in the QuantumInstance methods too. This commit removes this and just relies on the helper functions to do the version checking. * Use more descriptive local variable name for backend version This commit attempts to clarify how the version fields are used by naming the local variables backend_interface_version to make it clear what we're checking in that context. * Fix handling of statevector simulator check for BackendV2 This commit fixes the check for whether we're running with a statevector simulator (to determine whether we need to insert save_statevector instructions) to be BackendV2 compatible. This isn't strictly needed yet as qiskit-aer is still based on BackendV1, but this would cause a bug when it does switch to the newer backend version. This commit just gets in front of that potential issue so we don't have a bug in the future. * Use backend_interface_version everywhere There were a few straglers still using backend_version instead of backend_interface_version in the code. This commit updates all these instances so there is consistent naming around this inside the utils subpackage. * Fix handling of statevector simulator detection This commit fixes an oversight from an earlier development version that slipped through later revisions where the statevector simulator backend detection in the QuantumInstance class was hard coded to False for BackendV2. This was just meant as a temporary debugging step while getting the initial commit ready, but was never updated to remove this until now. * Actually commit local variable backend_interface_version renames * Fix typo Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 6637516)
* Fix QuantumInstance compatibility with BackendV2 This commit fixes compatibility with the QuantumInstance class when targetting a BackendV2 backend. When BackendV2 was introduced testing with the QuantumInstance (and therefore all of qiskit.algorithms) was neglected and there are several compatibility issues with the class around hard coded assumptions that the backend being wrapped was a BaseBackend or BackendV1 object. * Remove invalid test The tests for the QuantumInstance with BackendV2 were just ported over from the equivalent testing with BackendV1. However, one of those tests was to test error mitigation with a custom noise model which requires qiskit-aer. We can't test this without aer having been migrated to BackendV2. Until qiskit-aer is backendv2 enabled this commit just removes the test. * Remove unused imports * Remove duplicate backend version checks from QuantumInstance The QuantumInstance's is_simulator() and is_local() methods had duplicate checking on the backend version. This was an artifact of testing where in earlier revisions of this PR branch it was done in the QuantumInstance before being moved to the inner helper functions. Since the helper functions are updated to handle the version checking now we no longer need it in the QuantumInstance methods too. This commit removes this and just relies on the helper functions to do the version checking. * Use more descriptive local variable name for backend version This commit attempts to clarify how the version fields are used by naming the local variables backend_interface_version to make it clear what we're checking in that context. * Fix handling of statevector simulator check for BackendV2 This commit fixes the check for whether we're running with a statevector simulator (to determine whether we need to insert save_statevector instructions) to be BackendV2 compatible. This isn't strictly needed yet as qiskit-aer is still based on BackendV1, but this would cause a bug when it does switch to the newer backend version. This commit just gets in front of that potential issue so we don't have a bug in the future. * Use backend_interface_version everywhere There were a few straglers still using backend_version instead of backend_interface_version in the code. This commit updates all these instances so there is consistent naming around this inside the utils subpackage. * Fix handling of statevector simulator detection This commit fixes an oversight from an earlier development version that slipped through later revisions where the statevector simulator backend detection in the QuantumInstance class was hard coded to False for BackendV2. This was just meant as a temporary debugging step while getting the initial commit ready, but was never updated to remove this until now. * Actually commit local variable backend_interface_version renames * Fix typo Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 6637516) Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Fix QuantumInstance compatibility with BackendV2 This commit fixes compatibility with the QuantumInstance class when targetting a BackendV2 backend. When BackendV2 was introduced testing with the QuantumInstance (and therefore all of qiskit.algorithms) was neglected and there are several compatibility issues with the class around hard coded assumptions that the backend being wrapped was a BaseBackend or BackendV1 object. * Remove invalid test The tests for the QuantumInstance with BackendV2 were just ported over from the equivalent testing with BackendV1. However, one of those tests was to test error mitigation with a custom noise model which requires qiskit-aer. We can't test this without aer having been migrated to BackendV2. Until qiskit-aer is backendv2 enabled this commit just removes the test. * Remove unused imports * Remove duplicate backend version checks from QuantumInstance The QuantumInstance's is_simulator() and is_local() methods had duplicate checking on the backend version. This was an artifact of testing where in earlier revisions of this PR branch it was done in the QuantumInstance before being moved to the inner helper functions. Since the helper functions are updated to handle the version checking now we no longer need it in the QuantumInstance methods too. This commit removes this and just relies on the helper functions to do the version checking. * Use more descriptive local variable name for backend version This commit attempts to clarify how the version fields are used by naming the local variables backend_interface_version to make it clear what we're checking in that context. * Fix handling of statevector simulator check for BackendV2 This commit fixes the check for whether we're running with a statevector simulator (to determine whether we need to insert save_statevector instructions) to be BackendV2 compatible. This isn't strictly needed yet as qiskit-aer is still based on BackendV1, but this would cause a bug when it does switch to the newer backend version. This commit just gets in front of that potential issue so we don't have a bug in the future. * Use backend_interface_version everywhere There were a few straglers still using backend_version instead of backend_interface_version in the code. This commit updates all these instances so there is consistent naming around this inside the utils subpackage. * Fix handling of statevector simulator detection This commit fixes an oversight from an earlier development version that slipped through later revisions where the statevector simulator backend detection in the QuantumInstance class was hard coded to False for BackendV2. This was just meant as a temporary debugging step while getting the initial commit ready, but was never updated to remove this until now. * Actually commit local variable backend_interface_version renames * Fix typo Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Fix QuantumInstance compatibility with BackendV2 This commit fixes compatibility with the QuantumInstance class when targetting a BackendV2 backend. When BackendV2 was introduced testing with the QuantumInstance (and therefore all of qiskit.algorithms) was neglected and there are several compatibility issues with the class around hard coded assumptions that the backend being wrapped was a BaseBackend or BackendV1 object. * Remove invalid test The tests for the QuantumInstance with BackendV2 were just ported over from the equivalent testing with BackendV1. However, one of those tests was to test error mitigation with a custom noise model which requires qiskit-aer. We can't test this without aer having been migrated to BackendV2. Until qiskit-aer is backendv2 enabled this commit just removes the test. * Remove unused imports * Remove duplicate backend version checks from QuantumInstance The QuantumInstance's is_simulator() and is_local() methods had duplicate checking on the backend version. This was an artifact of testing where in earlier revisions of this PR branch it was done in the QuantumInstance before being moved to the inner helper functions. Since the helper functions are updated to handle the version checking now we no longer need it in the QuantumInstance methods too. This commit removes this and just relies on the helper functions to do the version checking. * Use more descriptive local variable name for backend version This commit attempts to clarify how the version fields are used by naming the local variables backend_interface_version to make it clear what we're checking in that context. * Fix handling of statevector simulator check for BackendV2 This commit fixes the check for whether we're running with a statevector simulator (to determine whether we need to insert save_statevector instructions) to be BackendV2 compatible. This isn't strictly needed yet as qiskit-aer is still based on BackendV1, but this would cause a bug when it does switch to the newer backend version. This commit just gets in front of that potential issue so we don't have a bug in the future. * Use backend_interface_version everywhere There were a few straglers still using backend_version instead of backend_interface_version in the code. This commit updates all these instances so there is consistent naming around this inside the utils subpackage. * Fix handling of statevector simulator detection This commit fixes an oversight from an earlier development version that slipped through later revisions where the statevector simulator backend detection in the QuantumInstance class was hard coded to False for BackendV2. This was just meant as a temporary debugging step while getting the initial commit ready, but was never updated to remove this until now. * Actually commit local variable backend_interface_version renames * Fix typo Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Fix QuantumInstance compatibility with BackendV2 This commit fixes compatibility with the QuantumInstance class when targetting a BackendV2 backend. When BackendV2 was introduced testing with the QuantumInstance (and therefore all of qiskit.algorithms) was neglected and there are several compatibility issues with the class around hard coded assumptions that the backend being wrapped was a BaseBackend or BackendV1 object. * Remove invalid test The tests for the QuantumInstance with BackendV2 were just ported over from the equivalent testing with BackendV1. However, one of those tests was to test error mitigation with a custom noise model which requires qiskit-aer. We can't test this without aer having been migrated to BackendV2. Until qiskit-aer is backendv2 enabled this commit just removes the test. * Remove unused imports * Remove duplicate backend version checks from QuantumInstance The QuantumInstance's is_simulator() and is_local() methods had duplicate checking on the backend version. This was an artifact of testing where in earlier revisions of this PR branch it was done in the QuantumInstance before being moved to the inner helper functions. Since the helper functions are updated to handle the version checking now we no longer need it in the QuantumInstance methods too. This commit removes this and just relies on the helper functions to do the version checking. * Use more descriptive local variable name for backend version This commit attempts to clarify how the version fields are used by naming the local variables backend_interface_version to make it clear what we're checking in that context. * Fix handling of statevector simulator check for BackendV2 This commit fixes the check for whether we're running with a statevector simulator (to determine whether we need to insert save_statevector instructions) to be BackendV2 compatible. This isn't strictly needed yet as qiskit-aer is still based on BackendV1, but this would cause a bug when it does switch to the newer backend version. This commit just gets in front of that potential issue so we don't have a bug in the future. * Use backend_interface_version everywhere There were a few straglers still using backend_version instead of backend_interface_version in the code. This commit updates all these instances so there is consistent naming around this inside the utils subpackage. * Fix handling of statevector simulator detection This commit fixes an oversight from an earlier development version that slipped through later revisions where the statevector simulator backend detection in the QuantumInstance class was hard coded to False for BackendV2. This was just meant as a temporary debugging step while getting the initial commit ready, but was never updated to remove this until now. * Actually commit local variable backend_interface_version renames * Fix typo Co-authored-by: Jake Lishman <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
This commit fixes compatibility with the QuantumInstance class when
targetting a BackendV2 backend. When BackendV2 was introduced testing
with the QuantumInstance (and therefore all of qiskit.algorithms) was
neglected and there are several compatibility issues with the class
around hard coded assumptions that the backend being wrapped was a
BaseBackend or BackendV1 object.
Details and comments