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

Batching of circuits to overcome memory issues when using statevector simulator #209

Merged
merged 18 commits into from
Oct 11, 2021

Conversation

rsln-s
Copy link
Contributor

@rsln-s rsln-s commented Sep 9, 2021

Summary

Currently batch_size parameter is ignored if the statevector simulator is used. This leads to unreasonable memory use due to the size of the transpiled circuits, up to 1TB of RAM for 800 by 800 kernel matrix and 20 qubits (see qiskit-terra, issue #6991). This pull request fixes this by transpiling and simulating circuits in batches, never storing the entire 800 circuits. The modification uses batch_size parameter that is already used in non-statevector case.

Details and comments

I had success by setting batch_size=50 (memory footprint down to <20 GB).

@CLAassistant
Copy link

CLAassistant commented Sep 9, 2021

CLA assistant check
All committers have signed the CLA.

@adekusar-drl
Copy link
Collaborator

@rsln-s Thanks a lot for your contribution. Could you please add a reno file with a bug fix description? Also, if possible add a test to cover this fix.

@attp could you please take a look at the changes?

@rsln-s
Copy link
Contributor Author

rsln-s commented Sep 10, 2021

Added reno file. The correctness of the output of the code is checked by tests already in place; testing the memory usage may be complicated within constraints of gh-actions.

….yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
@woodsp-ibm
Copy link
Member

woodsp-ibm commented Sep 14, 2021

The spell checker also fails on this transpiled word from the release note. If you add it to .pylintdict file in the root, i.e. to our custom dictionary, since its correctly spelt, then that should get it to pass CI. (You will see the words are in lowercase in alphabetic order so just add it appropriately)

@rsln-s
Copy link
Contributor Author

rsln-s commented Sep 14, 2021

Updated the .pylintdict dictionary

adekusar-drl
adekusar-drl previously approved these changes Sep 15, 2021
Copy link
Collaborator

@adekusar-drl adekusar-drl left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot!

@adekusar-drl
Copy link
Collaborator

@woodsp-ibm Do you have any comments?

@woodsp-ibm
Copy link
Member

Can I ask you to make a minor change to the docstring for batch_size in the constructor. It says

batch_size: Number of circuits to batch together for computation. Default 1000.

specifically default 1000 yet the code has

batch_size: int = 900,

So it should state the default is 900. I think it may have been 1000 at one point but was changed, if I recall correctly, to 900 to fit more with the limits around the provider.

In terms of testing you say its covered by the current unit tests. From what I can see there is nothing explicitly testing that aspect. Of course batch_size defaults to 900 and is in main path. I am not sure what we do in test is ever affected by the batch size since from what I can see the tests are pretty small. Having said that if we had a test that dropped the batch size down I am not sure how to test it given its behavior is internal to evaluate - the only way comes to mind is hooking the quantum instance execute and checking the number of circuits is as expected along the way in addition the the final result being as expected.

@attp
Copy link
Contributor

attp commented Sep 15, 2021

Looks good to me.

Regarding the batch_size docstring, you are correct @woodsp-ibm the default was originally 1000, and we changed it to 900 to match the backend limits and reduce the number of jobs sent. I must have missed updating the docstring in the PR.

@rsln-s
Copy link
Contributor Author

rsln-s commented Sep 17, 2021

Updated the docstring as requested by @woodsp-ibm.

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Sep 21, 2021

Just a note. Locally I changed the QuantumInstance execute method to print the number of circuits it was passed and ran the kernel unit tests. Its quite a small number so nowhere near the 900 limit - in fact its not even in double digits. Anyway I changed default back size in the kernel to a much small number and things worked but not all numbers were limited - presumably the ones via statevector usage as I was doing this from the main branch. Cloning your fork and doing the same from there then all the counts were limited - in fact I set it to 1 as a test and it printed all 1's and passed. So it seems its working ok. It would be nice if the unit testing did somehow test out the batch size but maybe that could/should be raised as a separate issue. @adekusar-drl any thoughts here - you commented early on about a test around the fix.

@adekusar-drl
Copy link
Collaborator

@woodsp-ibm When I mentioned unit tests I did not have anything special on my mind. In general, your idea of setting batch size to 1 and then running a test on the statevector simulator make sense.

@rsln-s What do you think?

@woodsp-ibm
Copy link
Member

In general, your idea of setting batch size to 1 and then running a test on the statevector simulator make sense.
I had done it so it applied to the qasm mode as well - since it appeared not tested in general. While the final outcome can be checked as currently, what is more complicated to do is to check that indeed the batch size is limiting the number of internal computations (circuits). My only thought on perhaps how to do such a check. as I mentioned in an earlier comment was about hooking the QuantumInstance execute method on the instance used with the backend such that the number of circuits given to the method could be fairly easily intercepted and checked - i.e hook it and do whatever was needed to check then call over to the original method that was hooked so that the circuit results from execute can be returned.

@adekusar-drl
Copy link
Collaborator

@woodsp-ibm I approve, merge this PR and open an issue to improve tests for QuantumKernel. Any thoughts?

Copy link
Member

@woodsp-ibm woodsp-ibm left a comment

Choose a reason for hiding this comment

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

merge this PR and open an issue to improve tests for QuantumKernel. Any thoughts?

@adekusar-drl that seems fine by me.

@adekusar-drl
Copy link
Collaborator

Closed and #242 is opened.

gentinettagian pushed a commit to gentinettagian/qiskit-machine-learning that referenced this pull request Dec 14, 2021
… simulator (qiskit-community#209)

Currently batch_size parameter is ignored if the statevector simulator is used. This leads to unreasonable memory use due to the size of the transpiled circuits, up to 1TB of RAM for 800 by 800 kernel matrix and 20 qubits (see qiskit-terra, issue #6991). This pull request fixes this by transpiling and simulating circuits in batches, never storing the entire 800 circuits. The modification uses batch_size parameter that is already used in non-statevector case.

* initial attempt that passes the tests

* further improvement

* fix formatting

* added reno file

* Update releasenotes/notes/batch-circuits-statevector-522a842c6f68d954.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* added the word `transpiled` to the dictionary for spellchecker

* updated docstring to accurately reflect default batch size

Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
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.

6 participants