-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Deprecate provider-specific fake backends,FakeProvider class and related tools in 0.46 #11381
Deprecate provider-specific fake backends,FakeProvider class and related tools in 0.46 #11381
Conversation
One or more of the the following people are requested to review this:
|
…e fake backend base classes and special fake backends for testing purposes.
be673e7
to
c5532d0
Compare
releasenotes/notes/deprecate-fake-backends-4dd275cf9a30d41f.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/deprecate-fake-backends-4dd275cf9a30d41f.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Kevin Hartman <kevin@hart.mn>
@kevinhartman I've pushed an extended version of the warnings, let me know what you think. |
#11670 has been merged, so the deprecation messages now point to an existing alternative :) I'm removing the "on hold" label. |
@@ -26,6 +28,22 @@ class FakePulseBackend(FakeQasmBackend): | |||
|
|||
defs_filename = None | |||
|
|||
def __init__(self): | |||
super().__init__() | |||
# This is a deprecation warning for the subclasses. |
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 still used by anything in Qiskit? And, how about FakeBackendV1
which this ultimately uses as a base class? Can all of this be deprecated?
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.
Ah I see, you're doing this in a separate PR it looks like 😄.
Or wait, are you? I'm a bit confused since I would think this file wouldn't have changes at all here since it's part of the V1 hierarchy?
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 confusing, but the V1 base classes (FakeBackendV1
, FakePulseBackend
, FakeQasmBackend
) cannot be deprecated yet because they are used in the "generic" backend version too. The "fake backend alternatives" in #10952 are effectively snapshot-based fake backends with "more generic" names. This was agreed to be the best temporary alternative while BackendV1 is around, the plan is to eventually fully transition to BackendV2 but this will not happen for 1.0.
So now we have the challenge to deprecate ~45 subclasses without deprecating the base classes. This warning is only for the subclasses, and this is why I don't use the decorator here, so that the message doesn't say "FakeBackendV1 is deprecated"
but "All fake backend instances based on real device snapshots..."
. And about the V1 hierarchy, I am taking advantage of the fact that these "V1 alternatives" to fake backends live on the main branch and the deprecations live on 0.46, so the warning would never appear on the non-deprecated alternatives.
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.
Ah okay, thank you! I was missing that subtly 😄.
# This is a deprecation warning for the subclasses. | ||
# FakePulseBackend is not deprecated. | ||
warnings.warn( | ||
message="All fake backend instances based on real device snapshots (`FakeVigo`," |
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 might be nice to have this message in the deprecation for FakeBackendV2
as well, since as it is I believe it won't tell users that the classes which inherit from FakeBackendV2
are deprecated.
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.
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.
Ah, I specifically meant the bit "All fake backend instances based on real device snapshots" might be helpful, since the user might think it's a bug otherwise to get a warning about a base class.
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.
aah sure, that makes sense
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 actually realized thanks to that screenshot that the message said "it will be removed Qiskit 1.0", so I updated the removal timeline of all the classes that were missing the "in" in 706de09.
@@ -55,7 +56,9 @@ | |||
class TestTarget(QiskitTestCase): | |||
def setUp(self): | |||
super().setUp() | |||
self.fake_backend = FakeBackendV2() | |||
with warnings.catch_warnings(): |
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.
Should we assert that this warns instead since we're deprecating now?
@@ -681,7 +686,9 @@ def test_non_overlapping_kak_gates_with_backendv2(self, opt_level): | |||
qr = QuantumRegister(2) | |||
circ = QuantumCircuit(qr) | |||
circ.append(random_unitary(4, seed=1), [1, 0]) | |||
backend = FakeBackendV2() | |||
with warnings.catch_warnings(): |
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.
Same comment. Assert warns instead?
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 doing this!
Summary
(1/2) --> This is the first PR of the second step of the
FakeBackends
refactoring epic in the main branch, it deprecates the snapshot-dependent fake backends as well as the fake provider.Details and comments
This PR is blocked by:
qiskit-ibm-runtime
release including Migrate fake provider base classes from Qiskit qiskit-ibm-runtime#1270 -> to avoid deprecation warnings being raised when using the fake backends inqiskit-ibm-runtime
, as this PR directly modifies the fake backend base classes currently shared with runtime. In principle people would useqiskit-ibm-runtime
withqiskit
1.0, so it is unlikely that the warning would be raised, but it's a good safeguard to have.This PR is followed by:
FakeProvider
class and related tools in 1.0 #11376Related PRs:
FakeGeneric
#10918