-
Notifications
You must be signed in to change notification settings - Fork 24
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
Provide support for Amplitude Estimation algorithms and similar #69
Conversation
…k-compatible with qobj, working around mcx issue
This should not be needed, if something is passing a qobj to the BackendV1 that's a bug in terra (and a pretty bad one, but honestly not unexpected from the algorithms/quantum instance code which is a mess of abstractions that ignore interfaces). We really don't want to paper over bugs like this here, especially as you point out disassembling a qobj is a best effort thing and not guaranteed to reproduce the circuit correctly (and clearly doesn't cover every case). Can you open a bug against terra with the details of what is failing with the stack trace and we can prioritize a fix, since this will be a larger problem than just the ionq provider. |
noted, terra issue is here: Qiskit/qiskit#6280 That said, @mtreinish, unless you think the terra fix will be out very soon (like, EOW soon) I still think we should merge this — I realize it feels like papering over a bug to you, but I still want to be responsive to user needs as it gets addressed upstream. I understand this stuff is always a bit of a moving target with OSS, but I want to address it ASAP as it's causing real, right-now issues with folks trying to use the provider. Additionally, it's a forward-compatible change that will silently pass once the upstream issue is fixed, so it doesn't feel particularly dangerous or poor form to me. |
Thanks Coleman for the very quick fix!
|
@Francesco-Benfenati can you go ahead and pull down the latest commit on this branch and give it one more go? This error should be resolved! |
@amilstead Thanks Alex,
Always coming from this obsolete qobj |
🤦 Sorry @Francesco-Benfenati, I'm going to go back to the drawing board here and make sure we can successfully run your example code before asking you to rerun. Thanks for your patience! |
No worries, thank you! |
@Francesco-Benfenati so, this is not really a permanent fix, and you may eventually run into additional edge cases due to the the issue mentioned above (this one), but I was able to run your example to completion and get a result with this diff:
Again-- please note: this is not a permanent fix. This is a hack, and will likely be irrelevant when the upstream terra issues are resolved. That said, it will unblock you for now. Apologies again for the back and forth here. |
Thanks Alex, adding those lines manually I got it working indeed! |
n.b. we are ultimately not going to merge this as the upstream fixes in Terra are now in PR, but will leave up for visibility until then in case anyone else is running into this problem |
@ColemanCollins @Francesco-Benfenati After this qiskit-terra bug fix makes it into a published package version, We will also plan to release an updated version pin in our provider to that release and later to mitigate this issue on new installations. I'll circle back and close this out once these two things have happened. |
Resolved by #72 + upstream fixes in Terra 0.17.4 |
Summary
It seems like some Terra methods still pass Qobj/don't respect versioned backend classes. I know this is an ongoing epic but until it's fully complete, it seems best to support at least a rudimentary qobj case (single QASM experiment in the qobj) in our conversion and serialization helpers so as to allow a broader chunk of Qiskit's prebuilt algorithms to be used. Also intend to open an issue in Terra addressing this, but want to get an interim fix in to address this user need which we know from private conversations is fairly urgent/critical.
Also removes backend "support" for mcx gates until Qiskit/qiskit#6271 is resolved — we need to decompose the Qobj, and we can't do that if mcx gates are present right now. Given we're just transpiling these on the API side right now anyway (i.e. it's not a native gate or somehow magically better decomposition) it's a pretty meaningless change from a UX perspective.
fixes #65