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

Barebone generic backend options #12747

Merged
merged 7 commits into from
Jul 11, 2024
Merged

Conversation

sbrandhsn
Copy link
Contributor

Summary

Added two options to the constructor of GenericBackendV2 that reduce the memory overhead from ~12GB to 300MB when set and transpiling on a 2000 qubit device.

Details and comments

Code:

be = GenericBackendV2(2000)

qc = QuantumCircuit(4)
qc.h(0)
qc.cx(0, 1)
qc.cx(1, 2)
qc.cx(2, 3)

qc_r = transpile(qc, backend=be)

Old:
image

New (setting include_channels=False and include_errors=False):
image

@sbrandhsn sbrandhsn requested review from jyu00 and a team as code owners July 9, 2024 16:36
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@jakelishman
Copy link
Member

Just to be clear, you're asking for a 2000q device here with all-to-all connectivity, so you're building instruction properties for like 4m+ gates.

@brandhsn
Copy link
Contributor

brandhsn commented Jul 9, 2024

Yes, I'm aware of that. :) The transpiler needs 2GB for a proper 2k qubit circuit (not the one in the example) I don't see why the transpiler infrastructure should need 6x that much in the general case also note that this is 3kb/gate memory overhead on average for the 12GB/4m+ gates - this is simply too much for the default path. Until 2k q circuits become relevant for the default path, I'm happy for the wary user to set the proper Qiskit settings, i. e. disable extended information on the used backend in order to get a feasible output.

@coveralls
Copy link

coveralls commented Jul 9, 2024

Pull Request Test Coverage Report for Build 9889319587

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • 81 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.01%) to 89.882%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 3 92.11%
crates/qasm2/src/parse.rs 6 97.61%
qiskit/circuit/library/basis_change/qft.py 6 91.15%
crates/circuit/src/dag_node.rs 7 85.58%
qiskit/transpiler/passes/basis/basis_translator.py 7 97.7%
qiskit/qpy/binary_io/circuits.py 25 91.75%
qiskit/transpiler/passes/synthesis/high_level_synthesis.py 26 90.41%
Totals Coverage Status
Change from base Build 9855880439: 0.01%
Covered Lines: 65745
Relevant Lines: 73146

💛 - Coveralls

@jakelishman
Copy link
Member

Putting in an option to remove the errors is fine, but that definitely shouldn't be the default path (which this PR doesn't do), because those are pretty critical, and all real-world backends are likely to have that.

The pulse stuff (the channels) is probably the more problematic bit of this, though, and qiskit.pulse's memory usage is not great end-to-end. The data model of the pulse/channel representations is just not very efficient. Having the option to disable that is certainly a good thing.


The point about all-to-all connectivity is that, since this information about a backend with 4 million links that are (in theory) all individually calibrated, it's not hugely unexpected that it'll use a lot of memory to represent. 3kb a link is high, but not 100% insane, considering the pulse model is specifying low-level calibrations of how the pulses on the link will affect it over general time, not just at the gate point. (Though pulse's memory usage is still not great, and tbh, neither is Target's in general.)

A more realistic example of memory usage would be to ask for a 2000q backend that has an average qubit connectivity of, say, 3, rather than an average qubit connectivity of 2000, which is what we're actually going to get from hardware vendors1. We still have problems, of course, but I wouldn't necessarily make API decisions based on a 2000q all-to-all device.

Footnotes

  1. The ion trappers can claim all-to-all connectivity, but in practice I think it's still fine to model ion shuttling as swaps, albeit with a lower error rate - the error rates would likely be linked to physical position in the trap, which is still well represented by a linear chain.

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Besides Jake's point of being careful with all-to-all connectivity blowing up easily (this was a nightmare to catch in some tests when adding the PR), I think it's ok to add ways to construct more lighweight fake backends if that helps running benchmarks. I have been asked about how to build noiseless generic backends before, so include_errors could actually kill two birds in one stone and give users a new way of constructing ideal/noiseless backends. Some missing points I see in the PR are:

  • missing new attribute descriptions in the docstring
  • dealing with calibrate_instructions with include_errors = False, because instructions without properties can't be calibrated (and it will currently fail)
  • maybe thinking of a different name for include_errors. I know no name is perfect, but what we are skipping here are actually the full instruction properties, which is different than setting errors to None (which is what I originally interpreted). I get that include_instruction_props can be too long... maybe include_noise? I use "noise" in different places in this class to refer to the instruction property values in general.

@sbrandhsn
Copy link
Contributor Author

Thanks for the code review! :-)

My previous argument was that if representing an all-to-all connected backend with 2k qubits ever becomes relevant, we ought to find a more efficient default path for constructing such backends. It may not become relevant or it may turn out that we don't require 3kb/gate of detailed information on such large devices. Maybe we should warn the user that we do not support the desired backend with the chosen configuration efficiently (with suggestions on how to rectify that). I'm only suggesting this because backends are quite a user-facing concept that may be exposed to a lot of users new to quantum or programming (I also think this suggestion and this discussion is out of scope for this PR).

BTW https://www.nature.com/articles/s41586-024-07107-7 already expects a degree-6 graph and other technologies might even exceed that. It may make sense to periodically check the memory overhead of backends/targets with reasonable connectivity.

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks! I like the new names. If you update the reno the PR looks good to me :)

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

I just realized that there are no unit tests, do you think you could come up with a couple of quick ones in test/python/providers/fake_provider/test_generic_backend_v2.py? (checking that setting the flag to false doesn't break anything should probably be enough, nothing fancy).

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Whoops, I didn't notice the typo either. Thanks a lot @sbrandhsn, LGTM!

@ElePT ElePT added this pull request to the merge queue Jul 11, 2024
Merged via the queue into Qiskit:main with commit a306cb6 Jul 11, 2024
15 checks passed
@ElePT ElePT added Changelog: New Feature Include in the "Added" section of the changelog mod: fake_provider Related to the fake_provider module and fake backends labels Jul 30, 2024
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
* Update generic_backend_v2.py

* reno

* Update barebone-backend-option-675c86df4382a443.yaml

* ...

* suggestions from code review

* Update barebone-backend-option-675c86df4382a443.yaml

* tests, typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog mod: fake_provider Related to the fake_provider module and fake backends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants