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

Fix default batching in variational algorithms #9038

Merged
merged 6 commits into from
Nov 1, 2022

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Oct 31, 2022

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... 😄

@qiskit-bot
Copy link
Collaborator

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:

@Cryoris Cryoris added this to the 0.22.1 milestone Oct 31, 2022
@Cryoris Cryoris added Changelog: Bugfix Include in the "Fixed" section of the changelog mod: algorithms Related to the Algorithms module labels Oct 31, 2022
@woodsp-ibm
Copy link
Member

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.

@Cryoris
Copy link
Contributor Author

Cryoris commented Nov 1, 2022

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 👍🏻

@coveralls
Copy link

coveralls commented Nov 1, 2022

Pull Request Test Coverage Report for Build 3371383702

  • 24 of 26 (92.31%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 84.499%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/algorithms/eigensolvers/vqd.py 3 4 75.0%
qiskit/algorithms/minimum_eigensolvers/sampling_vqe.py 3 4 75.0%
Totals Coverage Status
Change from base Build 3371383153: 0.009%
Covered Lines: 62464
Relevant Lines: 73923

💛 - Coveralls

@Cryoris Cryoris added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Nov 1, 2022
mtreinish
mtreinish previously approved these changes Nov 1, 2022
Copy link
Member

@mtreinish mtreinish 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 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

qiskit/algorithms/eigensolvers/vqd.py Outdated Show resolved Hide resolved
woodsp-ibm
woodsp-ibm previously approved these changes Nov 1, 2022
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.

Looks ok to me!

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@Cryoris Cryoris dismissed stale reviews from woodsp-ibm and mtreinish via 7f6ae23 November 1, 2022 14:43
@mergify mergify bot merged commit e4a7c40 into Qiskit:main Nov 1, 2022
mergify bot pushed a commit that referenced this pull request Nov 1, 2022
* 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)
mergify bot added a commit that referenced this pull request Nov 1, 2022
* 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>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
* 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>
ElePT pushed a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: algorithms Related to the Algorithms module stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VQE should batch per default
5 participants