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 BackendV2 version of FakeOpenPulse2Q #8712

Open
wshanks opened this issue Sep 8, 2022 · 6 comments
Open

Add a BackendV2 version of FakeOpenPulse2Q #8712

wshanks opened this issue Sep 8, 2022 · 6 comments
Labels
type: feature request New feature or request

Comments

@wshanks
Copy link
Contributor

wshanks commented Sep 8, 2022

What should we add?

Would it be reasonable to add a BackendV2 version of FakeOpenPulse2Q? What would need to be added besides the class? There are not really tests for the V1 version, but it is used in several tests.

I made a hacked V2 version in qiskit-community/qiskit-experiments#900 for reference. Another option might be to wait for #8611. I am not sure about timing and priorities.

Also, a question that came up in qiskit-community/qiskit-experiments#900 and Qiskit/qiskit-ibm-provider#387 -- could the V2 version of FakeOpenPulse2Q have a defaults() method? Could the other fake V2 backends have it as well (see fake_pulse_backends.py in qiskit-community/qiskit-experiments#900).

I could work on this but I wanted to see if it would be acceptable or if this is not moving in the same direction as BackendV2 and should just be hacked around outside of terra (like in qiskit-community/qiskit-experiments#900) until it is not needed.

@wshanks wshanks added the type: feature request New feature or request label Sep 8, 2022
@jakelishman
Copy link
Member

jakelishman commented Sep 8, 2022

It seems useful to me, but I don't have the full details. FakeOpenPulse2Q probably doesn't have tests because most of the fake backends were originally just testing mocks themselves (for old IBMQ backends). We just promoted them recently to be a full part of the Terra interface, since they got such wide use in practice.

In that vein, having a V2 version of FakeOpenPulse2Q (or however-many-q-you-want-it-to-be) seems like a sensible addition to me. I don't have the right knowledge to fully say what's required beyond just quoting the docs to you, though.

I would be against adding methods to any prospective BackendV2 subclass in Terra that's not in the BackendV2 interface in order to allow better duck-typing / mocking of IBM Quantum backends. BackendV2 has _default_options, which I believe is the method you want to implement (though I'm very not sure). I think adding the v1-ish (or IBMQ-ish) defaults() would blur the lines, and make the fake backends a poorer reference implementation for others, or risk eroding Terra's backend agnosticism.

@wshanks
Copy link
Contributor Author

wshanks commented Sep 9, 2022

BackendV2 has _default_options, which I believe is the method you want to implement (though I'm very not sure).

No, defaults() is a method that returns a PulseDefaults object, like here:

https://github.com/Qiskit/qiskit-terra/blob/e8001f5448fa9de980858e490d045ca3535a8498/qiskit/providers/fake_provider/fake_pulse_backend.py#L24-L33

defaults() was not in V1 or V2 but was in the V1 pulse fake backends (and is in the real V1 and V2 IBM backends), so I was not sure if it was a V1 feature or a pulse feature. The main attribute that is accessed in it is instruction_schedule_map and the equivalent of that is BackendV2.target. It is mainly just the pulse channel frequencies that are useful in defaults() now and not available elsewhere. So it seems reasonable to me to find another place for those and drop usage of defaults() going forward with V2. I can work around that in the meantime. For what it's worth though, the fake backends like FakeBelemV2 do not have a defaults() method while the "real" V2 belem backend from qiskit-ibm-provider does.

@jakelishman
Copy link
Member

Let me leave this for @mtreinish when he's back next week - I'll probably just get more things mixed up and not help anything.

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Sep 12, 2022

Here the real IBM V2 backend reports .defaults() object. Indeed, this is a bug/missing feature in terra's fake provider because these backends are basically the mirror of IBM backends, not a generic V2 backend -- I'm fine with not reporting .defaults in provider agnostic V2 object.

To me what @wshanks is proposing for IBM fake provider seems reasonable upgrade. Perhaps, it would make more sense to have PulseBackendV2 subclass that allows pulse-level control. Although this is a good guide for third-party provider to define own pulse backend (and type hint as @jakelishman mentioned), it might be very tough to accommodate both architecture agnostic and technology-specific configuration in the same representation. This makes me think we would need FixedFrequencySuperconductingPulseBackendV2 which looks really ugly and this no longer reasonable abstract class that Terra can provide.

What I'm not really clear about V1 and V2 model is the future migration plan of configuration, properties, and defaults. The one of the philosophies of V2 is to define a flat data structure so that community provider can easily define their backend. In this sense, these members don't fit in with this principle and should be deprecated in future. So this is not really good idea to develop software stack such as Qiskit Experiments based on these properties.

@mtreinish
Copy link
Member

Right, .defaults() is explicitly not part of the v2 api and we shouldn't be adding it to v2 backends. The reason the IBM provider implementation has it is primarily for backwards compatibility, to provide user code a migration path from v1 to v2, from the ibm provider perspective a user was using their backend objects with v1 in an older release and migrating to v2 if those methods didn't exist their users would break. It also arguably only makes sense for the IBM provider because the payload is literally just an object representation of the API response JSON payload for default pulse calibrations of a backend. From the v2 abstract interface definition defaults does not exist by design. For v1 .defaults() was part of the specification because v1 and the previous unversioned interface basically just mirrored IBM quantum's rest api (although it was explicitly decoupled from the actual api format) as an optional method that a backend could, but wasn't required to, implement. In my ideal end state of finishing the v1->v2 migration (which is probably at least a year off) the qiskit.providers.models subpackage (along with qiskit.qobj and qiskit.assembler) would no longer be in terra because their really belong in the ibm provider only.

From the qiskit-terra and general qiskit user perspective a BackendV2 implementation the defaults() method is a custom attribute and can not be relied upon always being there. For the fake provider in terra we're explicitly not adding defaults (or configuration() and properties()) because it's not part of the standard interface and we're trying to keep the interface limited to what we offer via the required and optional interfaces to ensure we're providing a sufficient interface for our internal needs by dogfooding it. The fake providers in terra are not meant to be a local mock replacement for the ibm provider, it's to provide terra with a battery of different test backends which mirrors real hardware's properties so we can do local testing. If the BackendV2 interface isn't sufficient for some reason then we should improve it to fix that.

@nkanazawa1989
Copy link
Contributor

The fake providers in terra are not meant to be a local mock replacement for the ibm provider, it's to provide terra with a battery of different test backends which mirrors real hardware's properties so we can do local testing.

This is what I misunderstood and this approach is sensible. One the other hand, in Qiskit Experiments, we need to write unittests with these fake provider's backends, and we must expect the same code works for the real backends from the IBM provider. So the change we will make locally should immediately propagate to IBM backends, and the updated model should be still architecture agnostic (note that we are talking about the low-level hardware configuration).

The important point, which is currently missing in the V2, is the field to describe the hardware configuration (e.g. channel frequency, source power, amplifier gain, etc... -- and available configuration depends on provider/access level of the client), and this should be a separate field from calibrations since such a hardware configuration is not gate dependent. I think Target.metadata, which might be a free-form dict, would provide good place to describe them and keep sufficient flexibility to describe various architectures.

For example,

drive_freqs = backend.target.metadata["drive_freqs"]
meas_freqs = backend.target.metadata["meas_freqs"]

Any thoughts? @wshanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants