Update transpile() to convert BackendV1 inputs to BackendV2 with BackendV2Converter#11996
Conversation
Pull Request Test Coverage Report for Build 8347542982Details
💛 - Coveralls |
|
One or more of the the following people are requested to review this:
|
ElePT
force-pushed
the
convert-v2-transpile
branch
from
2a6f289 to
6e56f98
Compare
ElePT
force-pushed
the
convert-v2-transpile
branch
from
6e56f98 to
e28a4d1
Compare
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
I only have one question (which is just something trivial) about the way some tests are written. Other than that LGTM!
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
This LGTM, I have some very very mild concern in the tests that we might be dropping some coverage of the v1 path now. But I don't think it's a big deal if we are.
The only question I have before enqueuing for merge is do you think this warrants a release note? Or were you viewing this as an internal implementation detail only? I think either is fine, as either position seems equally valid, but I wanted to check first.
| } | ||
|
|
||
| in_data = {"num_qubits": configuration.n_qubits} | ||
| in_data = {"num_qubits": configuration.num_qubits} |
There was a problem hiding this comment.
Fwiw n_qubits is still valid, it's part of the backend configuration schema:
and it's why we never removed the n_qubits property. But using num_qubits here for consistency makes sense.
There was a problem hiding this comment.
Oh, I just realized that the num_qubits attribute docs call out that you shouldn't use n_qubits, but we could never actually remove it because of the legacy heritage around matching the schema. I'll be very happy once we deprecate the V1 interface and the model objects for removal in 2.0.
There was a problem hiding this comment.
yes, I based this change off that comment, I was not aware of n_qubits being used in the schema so good to know.
| backend_name="fake_1q", | ||
| backend_version="0.0.0", | ||
| n_qubits=1, | ||
| num_qubits=1, |
There was a problem hiding this comment.
I actually don't think either field is required in BackendProperties, this just gets added the payload as an extra field but afaict nothing actually will use this because it's not part of the schema.
|
Hmm we might be dropping some coverage but I would say that most of the V1 path was actually tested with loose constraints, so it would be a very small percentage (as you said). In this sense, #12185 is riskier. I thought of this change as a non user-facing one, but I guess a reno would also work, I could then extend it in #12185. |
sbrandhsn
added
the
Changelog: Added
7 participants
Summary
This PR is a step forward in unifying the transpiler internals to use the target-base model, as specified in #9256.
I had to fix the behavior of
transpilewith custom scheduling constraints when givenBackendV2inputs, and extracted this into a separate PR, which should be merged before this one: #12042 [edit: merged].Adding the on hold label again for this reason.Details and comments
The conversion to the target-based model is hidden to the user but this PR includes a couple of small user-facing changes required for the pipeline to work:
convert_to_targetthat now allows to process calibrations for instructions without registeredInstructionProperties, instead of skipping these instructions (I believe that this was not the expected behavior)BackendV2Converterthat now looks fornum_qubitsinstead of the legacyn_qubitsin the backend configurationBackendV2ConverterNote that BackendV2 does not have any internal tracking whether the backend is a simulator, so any "simulator-based test" now uses
BasicSimulatordirectly (test/python/transpiler/test_1q.py).In addition to this, some unit tests were using "loopholes" in the
BackendV1model that wouldn't (and shouldn't) translate to a target properly, such as giving a coupling map that didn't match the number of qubits of the backend (test/python/transpiler/test_preset_passmanagers.py) or an instruction schedule map that contained non registered basis gates (test/python/transpiler/test_pulse_gate_pass.py). These tests have been adapted to be coherent with the new model.