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 option to run MLAE circuits separately #7428

Closed
wants to merge 4 commits into from
Closed

Conversation

jplewa
Copy link

@jplewa jplewa commented Dec 17, 2021

Summary

Certain backends, such as IonQBackend in qiskit-ionq, don't support circuit lists in the run method (see here). In this case, lists that contain a single element are allowed, but not lists with multiple elements. With MLAE it shouldn't matter if the independent circuits are submitted as one job or as several separate job jobs. The latter will probably take more time in most cases, so I kept the current behavior as the default and added an optional argument that splits the circuits into separate jobs.

Details and comments

run_circuits_as_one_job is a bit of an awkward name, but I couldn't think of anything better.

@jplewa jplewa requested review from manoelmarques, woodsp-ibm and a team as code owners December 17, 2021 19:42
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty sensible to me, though the algorithms code is far outside my domain. Please could you also add a release note briefly explaining the addition?

(By the way, you don't need to update the PR when there are changes on main - it'll get updated automatically during the merge process.)

Comment on lines +70 to +72
run_circuits_as_one_job: If set to True, the necessary circuits will be submitted as
one job. Otherwise, each circuit will be run separately. This is useful for
backends that don't support multi-circuit experiments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use_separate_jobs=False? (I have very little preference here - just suggesting alternatives.)

A minor wording nitpick: "If set to True" implies to me that that's not the default. It'd perhaps be best to use the words "If set to ..." to talk about the non-default setting.

Comment on lines +106 to +107
self._run_circuits_as_one_job = run_circuits_as_one_job

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that backend can be changed after object instantiation, you may want to make this a public attribute so it can be set if the value of the switch isn't known til later.

Comment on lines +568 to +575
mlae = MaximumLikelihoodAmplitudeEstimation(
evaluation_schedule=5,
quantum_instance=quantum_instance,
run_circuits_as_one_job=False,
)

# run the algo
result = mlae.estimate(problem)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to use a mock object to wrap your quantum_instance's execute method, and ensure that it's called the number of times you expect - as it stands, this test only ensures that the option is valid, not that it actually separates the circuits.

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Dec 20, 2021

The QuantumInstance class is supposed to take care of any backend specific requirements in this regard and should be splitting what the algorithm emits in order to meet the backend requirements. I recall there was an earlier issue around this but I thought it was resolved to allow the algorithms to work correctly.

Update - just searched for it #6391- I see that refers to the old V1 backend path but I thought the V2 path was ok. Maybe @mtreinish has some thought here?

@jplewa
Copy link
Author

jplewa commented Dec 20, 2021

(By the way, you don't need to update the PR when there are changes on main - it'll get updated automatically during the merge process.)

Right, I wasn't sure if this repo had up-to-date merge checks ;) Thanks for letting me know!

The QuantumInstance class is supposed to take care of any backend specific requirements in this regard and should be splitting what the algorithm emits in order to meet the backend requirements. I recall there was an earlier issue around this but I thought it was resolved to allow the algorithms to work correctly.

Huh, it definitely doesn't work with the IonQBackend implementation from azure-quantum at the moment. I wasn't able to run QuantumAmplitudeEstimation either, even though the circuit list contains just one element in this case. I have PR in that repo to at least allow circuit lists of lenght one for now (microsoft/azure-quantum-python#201), because currently I can't use any of the Qiskit amplitude estimation classes. If that PR gets merged, I'd expect QAE, IAE, and FAE to work, but not MLAE, since it typically runs several circuits at once - I even used the debugger to verify that the IonQBackend#run method does indeed receive several circuits at once in the case of MLAE. Looking at the code of IonQBackend#run there's no chance that could work right now (this is the line that's currently throwing an exception, but some other calls like circuit.name definitely also won't work if circuit is a list).

I know the story is pretty much the same in qiskit-ionq. A while ago they added the option to handle single-element circuit lists (qiskit-community/qiskit-ionq#71). If run receives a list with several elements, they explicitly throw a runtime exception.

@woodsp-ibm
Copy link
Member

Huh, it definitely doesn't work with the IonQBackend implementation from azure-quantum at the moment.

Ok, understood. But I think the right approach is to figure where/why the mismatch is occuring. The QI code uses a run_circuits function which looks for max_experiments property from the backend - maybe that is not being set correctly https://github.com/Qiskit/qiskit-terra/blob/0d85bfe3f4765a64f8e018514daa3898d9cf32af/qiskit/utils/run_circuits.py#L478-L484 It is, as you see, also influenced by an environment variable as retrieved here https://github.com/Qiskit/qiskit-terra/blob/0d85bfe3f4765a64f8e018514daa3898d9cf32af/qiskit/utils/run_circuits.py#L38

Maybe try setting the env var to 1 and see if things work. Then perhaps we cab figure what needs changing to make things work. I would rather not have algorithm specific setting for this aspect since others, like VQE etc etc will produce lists of circuits and all should work with any backend where the QuantumInstance and its use of run_circuits correctly fragments this list into one or more jobs according to any backend limits.

@jplewa
Copy link
Author

jplewa commented Dec 20, 2021

Maybe try setting the env var to 1 and see if things work.

I was just looking at this bit of code :) MLAE does work if I set it to 1 (and I use the version of azure-quantum from my PR). Is having to set the variable manually on one's machine the intended approach?

@woodsp-ibm
Copy link
Member

Is having to set the variable manually on one's machine the intended approach?

No, that was intended as a manual override that could be used. Ideally of course things should "just work". In that regard the run_circuits was designed to be able to configure itself via information about the backend. This is the other path in the if test to set max circuits per job. If its a local simulator, as per its determination, then there is no limit. Otherwise it sets from max_experiments - what is that for your backend - not sure how much of the configuration is standard/expected? I assume its not 1 - maybe it returns None - but a cursory look at the code I think that ought to fail later when it looks at splitting the list into chunks if that were the case.

@jplewa
Copy link
Author

jplewa commented Dec 20, 2021

Ideally of course things should "just work".

My bad, it looks like it does work even without setting this variable! Apologies, I was focusing on the fact that the IonQBackend#run methods in both azure-quantum and qiskit-ionq were throwing these explicit errors when faced with a multi-element circuit list. I was sure I had debugged MLAE to make sure this method was actually receiving multi-element circuit lists, but it looks like I hadn't (I probably just looked at run_circuits and called it a day without analyzing the code that's executed between the two methods). Everything should be fine with QAE, IAE, FAE, and MLAE, once the azure-quantum PR gets merged and the backend stops freaking out when it sees a circuit list with one element in it. Sorry again, debugging things across three libraries gets tricky ;)

Note: I only verified this for azure-quantum. I don't think I have the IonQ token that would be necessary to use the qiskit-ionq provider.

@jplewa jplewa closed this Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants