-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix default batching in variational algorithms #9038
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
When we first put such function in Aqua it was a simple boolean to turn on batching or not - the optimizer could send as many as it wanted when enabled. People wanted better performance and we were looking to leverage Aer parallel capability. What transpired was that the performance consideration was in regard of running bigger systems - larger operator and more pauli terms/groups. What happened was it would run out of memory with the circuits very easily. Hence we dropped to max_evals_grouped as a more controlled setting and defaulted it off - here the number of points (evals) could be defined, with it defaulting to 1 which is normal non-batched behavior. Could something be more specifically done around SPSA that is being used for real hardware - detect that and set the batch level to 2 for your default case. I guess that does not cover the calibration where we can batch all those points at once. If that's needed then enough to cater to that too. Hopefully whatever we do then with some remote device, whether via the runtime or via say these backend wrappers we do not hit memory issues. I guess if that would happen then setting the max grouped evals explicitly on SPSA to some smaller number for a user would be a solution should this be a problem. This way hopefully it performs by default well, compared to the original runtime VQE etc, and there is some fallback, which the code in this PR already caters too in terms of an explicit setting. Since the runtime devices are all real/qasm sim (not statevector) maybe limiting this to SPSA might be ok - though I believe you had a concern over the gradient descent too but maybe that can be included too so its more selective about the optimizer as to when its enabled and maybe the values set can be chosen accordingly. |
After talking with @jyu00 and running a test with 1000 circuits on the primitives (which failed) maybe reducing the number of grouped evals specific to SPSA would be the best approach for now. Until the primitives handle chopping the jobs into sizes that the memory on the devices can handle, we cannot just "turn on" the batching as we still have the same problem @woodsp-ibm describes above. I'll adjust it 👍🏻 |
Pull Request Test Coverage Report for Build 3371383702
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me, from a backwards compat PoV this should be fine I think (also to backport).
Since if the user explicitly doesn't set a batch size batching from vqe and friends setting it internally and not mutating the optimizer object outside it's current defined behavior. It's a bit hacky but I think that's ok given the constraints we're working with here. We can look at doing this in a more clean manner and explicitly breaking perhaps in 0.23.0. I left one super tiny nit inline but feel free to ignore it. I'll wait for @woodsp-ibm to give it a 👍 before tagging it as automerge though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok to me!
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
* Fix default batching in variational algorithms * fix test * reduce batching to only SPSA * fix tests * Apply suggestions from code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit e4a7c40)
* Fix default batching in variational algorithms * fix test * reduce batching to only SPSA * fix tests * Apply suggestions from code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit e4a7c40) Co-authored-by: Julien Gacon <gaconju@gmail.com>
* Fix default batching in variational algorithms * fix test * reduce batching to only SPSA * fix tests * Apply suggestions from code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Fix default batching in variational algorithms * fix test * reduce batching to only SPSA * fix tests * Apply suggestions from code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
Fixes #8926.
Details and comments
As we motivate users to use the runtime primitives over runtime programs, it is important that the code remains as efficient as possible. With the variational algorithms released in 0.22.0 however, there's a significant slowdown if hardware is used (also with Sessions), as we didn't include batching of energy evaluations. This PR fixes that performance issue by adding default batching in case the user didn't specify any, which is the same behavior users previously had with the runtime programs.
For the 0.23 release we're planning to update how batching works and then these hardcoded limits can be removed, but we probably don't want to wait 3 months to fix this slowdown of a factor of ~2 🙂
I'm not sure if it's ok to change the default of the internal
_max_evals_grouped
(@mtreinish @jakelishman what do you think?) but I couldn't find another way to check if the user set the_max_evals_grouped
manually. If that change blocks inlcuding this as a bugfix, we could also not fix this behavior and add a very big textbox in the docs that says users really have to enable batching themselves manually. Until they discover that note, however, they likely already complained about slow runtimes... 😄