Support direct Target generation and lazy Pulse Qobj conversion#413
Conversation
fbe7c76 to
224712a
Compare
Pull Request Test Coverage Report for Build 4026788784
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4726014865
💛 - Coveralls |
There was a problem hiding this comment.
Mostly looks good to me, looks like lots of integration tests are failing with the following errors:
test_ibm_integration, test_ibm_qasm_simulator
File "/Users/kt/opt/anaconda3/envs/dev/lib/python3.9/site-packages/qiskit/circuit/gate.py", line 237, in validate_parameter
raise CircuitError(f"Invalid param type {type(parameter)} for gate {self.name}.")
qiskit.circuit.exceptions.CircuitError: "Invalid param type <class 'str'> for gate kraus."test_ibm_job, test_ibm_job_attributes
File "/Users/kt/opt/anaconda3/envs/dev/lib/python3.9/site-packages/qiskit/circuit/gate.py", line 237, in validate_parameter
raise CircuitError(f"Invalid param type {type(parameter)} for gate {self.name}.")
qiskit.circuit.exceptions.CircuitError: "Invalid param type <class 'str'> for gate mcu3."I'm using terra 0.23.1
a0e61cd to
a9ba0f6
Compare
mtreinish
left a comment
There was a problem hiding this comment.
Overall this LGTM, I like the new structure for the target generation, it also makes a lot more sense in the json utils because that's what it's doing (the old file was called backend_converter.py because that was the filename of my local test script for going v1->v2 that it was based on). I just left two small inline comments about some edge case handling in the target generation.
The other thing is I think this needs a rebase, it conflicts with some recent changes made to the backends to do more lazy loading in general.
| new_instruction = Gate( | ||
| name=name, | ||
| num_qubits=len(qubits), | ||
| params=[], | ||
| ) |
There was a problem hiding this comment.
Shouldn't we look this up in qiskit_inst_mapping first instead of assigning it an opaque gate?
There was a problem hiding this comment.
I decided to ignore this in a67315e because this must be an edge case of misconfiguration, i.e. gate property is provided for gates which is not included in basis gates.
ef1869b to
c5999a4
Compare
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
c5999a4 to
a67315e
Compare
|
Thanks Matthew for reviewing. I added logger to the decoder module and add more description for missing entries (I hope this will help deployment work). The criteria of error is bit unclear, as you suggested to raise in this comment. This case could be just a info log since missing calibration (especially for default gates) is not significant matter. On the other hand, missing Qiskit gate representation for given name is much more problematic and we could raise an error. I completed integration tests locally and encountered following errors, but likely these are not relevant to this PR. I didn't complete |
|
My thinking on that comment at the time was if the defaults payload contained a gate not mentioned in the configuration or properties it was malformed and that should be an error. But, the two things I hadn't thought of at the time but on reflection are important to discuss now are:
So given that I think assuming this isn't an edge case that could come up during calibrations I think you're correct we should just log this and not raise an exception. Since you're correct a user would have no control over the payload being malformed as it would require changes in the backend to fix it. So I think your suggestion to just emit a log message (maybe at warning level instead of info so it's seen by default) so users can report it's happening and just ignore the out of spec gate. |
|
Thanks Matthew. That's correct. There is nothing actionable for end-users. I chose the info level since IBM backend still reports U gate calibrations and it generates very long lines of warning message which is quite annoying. I turned the error into log message and set warning level only to missing Qiskit gate 939ca42. I think now this PR is ready for the final review/approval. |
mtreinish
left a comment
There was a problem hiding this comment.
This LGTM, thanks for making this change
Summary
This PR has dependency on Qiskit/qiskit#8885
This PR allows V2 backend to bypass generation of
PulseDefaultsandBackendPropertiesto generate target, in addition to support of lazy Pulse Qobj conversion mechanism for speedup. Roughly x100 speed up in loading the target of ibm_washington.Details and comments
1. Benchmarking this branch with Qiskit/qiskit#8885
To avoid taking network latency into account, target generation speed is measured by preloaded defaults and property dictionary.
2. Benchmarking main branch
In the same way target "conversion" speed is measured.