-
Notifications
You must be signed in to change notification settings - Fork 126
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
Backend v2 basic compatibility #843
Backend v2 basic compatibility #843
Conversation
…rations module not correctly handling `qubit_freq_est`
# Conflicts: # qiskit_experiments/database_service/db_experiment_data.py # test/test_readout_error.py
…uency for now as it's not available
… usages to `BackendData`
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.
Thanks @gadial this is almost good. I have a design suggestion on new class. Apart from this, you should write a release note about new data object and base class change, so that a user (developer) doesn't access backend data through self._backend
.
|
||
def __init__(self, backend): | ||
"""Inits the backend and verifies version""" | ||
self._backend = 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 think backend can be discarded after parsing all necessary data. I like _parse_additional_data
logic. Can you do the same for other fields and remove self._v1
and self._v2
? Having these member may indicate this object is "sensitive" to backend version, but the aim of this object is to return the same data regardless of backend version.
You can write a dispatcher to do it cleanly. Actually functools provides a method dispatcher but available only > 3.8. Using this pattern makes this object robust to future update of backend, i.e. V3. You can write backend parser with dispatcher and save parsed values as protected members. Then you can provide property methods to indicate these are kind of static values.
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 like the idea of pulling out all the data in __init__
so that the other methods can be common to V1 and V2. One question though -- would we need to use this compatibility class for any property of the backend that is allowed to be modified? Nothing in the class currently should be modified. max_circuits
is an edge case which we have previously decided can be changed to help with job splitting: #638 (comment). However, modifying max_experiments
like that comment describes is a hack according to this comment: Qiskit/qiskit-ibm-provider#361 (comment) (but it's the only way in qiskit-experiments to get a job split on a smaller number of circuits with BackendV1; with V2 the max_circuits_per_job
run option can be used -- actually the provider supports it but I think BaseExperiment needs to be updated to support it as well).
Besides max_circuits
/max_experiments
, is there anything else?
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 very hesitant to accept the parsing solution. The backend comes from an outside source, and I don't know if and when it might be changed. I think of BackendData
as a mediator class, which hides the differences between v1 and v2, not as something that somehow enables us to forget about the backend.
I considered using dispatcher but for now I actually prefer to keep the code as explicit as possible - I don't think it's large enough to drive us into using more abstract code.
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.
Fair enough, but I don't understand why
I don't know if and when it might be changed
this becomes a problem for parsing. Actually backend is bit weird object because it is basically "configuration" + "interface for job submission". So current code allows us to write
my_expr._backend_data._backend.run(...)
though it is not expected to call protected members (I meant, this is no longer "data", but this is not strong objection).
@@ -95,6 +95,7 @@ def enable_restless( | |||
""" | |||
try: | |||
if not rep_delay: | |||
# BackendV1 only; BackendV2 does not support this |
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.
Actually you can assume "BackendV2" have rep delay in options. The basic idea of V2 is to minimize provider-specific configurations as a provider agnostic framework. In this sense, backend data object can provide this value. If you cannot find it, you can just return None.
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 a little confused. Since rep_delay_range
is not part of BackendV2
interface, I don't know where I should look for it. Should I check the existence of a rep_delay_range
field in the BackendV2
object? Can I be sure it won't have another name, such as rep_delay
? Is there a documentation discussing the standard for BackendV2
fields that might be missing?
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 guess such value is provided through backend.options
. One of the important concepts of the backend V2 is the flattened configuration fields (everything is under options), and less backend-specific configuration. So IBM-ish configurations are now removed from the base class. However, you can still assume such configuration exists for a particular experiment relying on it. If the option is not reported, it simply means you cannot run the experiment on that backend. In this sense, it would be better to define dedicated error such as MissingBackendOptionError
to make this context clearer.
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.
@mtreinish please correct me if I'm wrong.
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.
Or maybe rep_delay_range
is directly attached to an IBM backend object as an instance property, because this is not configurable value and Options
doesn't support static (read-only) field, i.e. getattr(backend, "rep_delay_range")
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.
What confuses me here is that in BackendV1
the rep_delay_range
value is in the configuration, not the Options
, so why would it be there in BackendV2
? Other fields from the configuration are now present directly as properties of BackendV2
, sometimes with different name, e.g.
backend.configuration().max_experiments
->backend.max_circuits
.
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
…acquire_channel`
…n not using fake backends
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.
Thanks, now this looks good to me. As a reminder, we need to fix the implementation of FakeBackendV2
in terra, and then remove temp code to generate _pulse_conf
which is currently necessary to provide channel mapping with FakeBackendV2
.
* Basic fixes enabling running of randomized benchmarking experiment * Beginning a switch to V2 backends in tests * Added failing test due to handling of control channels in calibrations * Control channel and coupling map working for backendv2 (requires Terra update) * Tests for existance of `drive_freq` and `meas_freq` * Test ramsey xy converted to fake v2 backend; known problem with calibrations module not correctly handling `qubit_freq_est` * Readout error tests converted to V2; raises a bug in composite experiments * Fix to parallel experiment to correctly get coupling map for v2 backends * Finishing the merge with the new service PR * Fixes to experiment_data to handle backend v2 * Switch RB tests to backend v2 * Switched fine drag experiment to v2 - no problems * Switched fine frequency experiment to v2 - should not work yet * Getting qubit frequency from backend v2 but dropping measurement frequency for now as it's not available * Linting * Cross resonance tests are now working with v2; starting to switch all usages to `BackendData` * Switching to `BackendData` usage * Switching to `BackendData` usage * Moving more experiments to v2 * Linting * Further fixes in calibrations * Fixes in test_ramsey_xy.py * Fix in test_fine_frequency.py * Linting * Fixing noisy delay backendv1 reference and added a failing test for t1 backendv1 reference * Non-fixable usage in restless mixin * Added test_circuit_with_backend to t1 tests and added the relevant fixes * Removed granularity setting and fixed the tests accordingly * Default granularity is 1 * dt default value 1 * Added getters for other target fields * refactored max_experiments to max_circuits * drive_freq and meas_freq name refactor * Refactored BackendData to be a standard class * Hack to enable working with BackendV2 until Terra gives native access to control channels * Linting * Switch to is_simulator for other experiments * Fix additional usages of backendv1 * Linting * Returning default dt leads to failing tests, so disable for now * resonator_spectroscopy.py fixes * default() usage fix in qubit_spectroscopy.py * drive_freq fix * Catch NotImplementedError * Fixed pulse data parsing * backend name access fix * Lint * Update qiskit_experiments/framework/backend_data.py Co-authored-by: Naoki Kanazawa <[email protected]> * Update qiskit_experiments/framework/backend_data.py Co-authored-by: Naoki Kanazawa <[email protected]> * Typo fix * Release note * Update qiskit-ibm-experiment version * Extended note about the problem with `is_simulator` in `BackendV2` * Update terra version * Remove `control_channels` from `BackendData` to avoid exposing inner data structure * Added additional support for `drive_channel`, `measure_channel` and `acquire_channel` * Linting * control_channel now relies on the backends control_channel method when not using fake backends * More robust code for now Co-authored-by: Naoki Kanazawa <[email protected]>
### Summary This change continues the work started in #843 to make qiskit-experiments compatible with BackendV2. It updates the spectrscopy, fine frequency and amplitude, and T1 tests to use `BackendV2` rather than `BackendV1`. Additionally it adds support for querying `BackendV2` for measurement frequencies. ### Details and comments The main changes are: * Use `backend.defaults()` to look up `measure_freq_est` for `BackendV2` as well as `BackendV1`. * Improve the error message when `measure_freq_est` can not be accessed for `ResonatorSpectroscopy` * Add T1 to `BackendData` because it is needed by `RestlessMixin`. * Switch `MockRestlessBackend` and `MockIQBackend` to `BackendV2`. This conversion was done by making a `FakeOpenPulse2QV2` `BackendV2` shim for the `BackendV1` `FakeOpenPulse2Q` in terra. * Replace modification of `basis_gates` with modification of `backend.target` in relevant tests. * Remove some unnecessary code (assignments to `timing_constraints` and a test class with no tests). There are a few of inelegant changes in this PR: * Usage of `backend.defaults()` was restored for accessing the measurement frequencies. This is an IBM specific method not in the `BackendV1` or `BackendV2` base classes. It is only used for querying meas_freq_est. `backend.defaults()` is what we had been using before #843, so this is just putting things back the way we were. We might decide to get this data a different way that does not use `defaults()` but at the moment that is the only way to get the measurement frequencies, so we would need to the IBM backends to provide these values in the properties, for instance. * `defaults()` was hacked into the fake `BackendV2` backends for the `ResonatorSpectroscopy` tests. The fake `BackendV1` backends in terra do provide it but not the `BackendV2` variants. One nice thing about moving away from `defaults()` (previous point) is that we could remove this hack. * There is no `BackendV2` version of `FakeOpenPulse2Q`, so one was hacked out of the `BackendV1` version. Perhaps we could get terra to include a `BackendV2` version of `FakeOpenPulse2Q` in the future. Closes #1099 --------- Co-authored-by: Naoki Kanazawa <[email protected]>
Summary
The goal of this PR is ensuring
qiskit-experiments
works with theBackendV2
class.Details and comments
Addresses #736