Setting noise_info to false in GenericBackendV2 creates issues later in the transpiler pipeline #12769

sbrandhsn · 2024-07-12T12:40:32Z

Summary

Fixes bugs introduced in #12747

Details and comments

I added some tests to check whether transpiling with noise-less and/or pulse-channel-less backends still works and noticed test failures. I fixed these bugs and added the corresponding test cases. I don't think this warrants a new release note but let me know if you disagree. :-)

qiskit-bot · 2024-07-12T12:40:36Z

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

@Qiskit/terra-core

coveralls · 2024-07-12T13:04:53Z

Pull Request Test Coverage Report for Build 10109907875

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
899 unchanged lines in 55 files lost coverage.
Overall coverage decreased (-0.2%) to 89.72%

Files with Coverage Reduction	New Missed Lines	%
qiskit/qobj/common.py	1	95.83%
qiskit/circuit/library/standard_gates/swap.py	1	98.18%
qiskit/primitives/backend_sampler.py	1	98.86%
qiskit/circuit/duration.py	1	70.27%
crates/accelerate/src/synthesis/permutation/utils.rs	1	99.02%
qiskit/primitives/base/base_estimator.py	1	97.3%
qiskit/transpiler/passes/layout/sabre_pre_layout.py	1	98.88%
qiskit/circuit/random/utils.py	1	94.87%
qiskit/circuit/library/standard_gates/x.py	1	98.25%
qiskit/transpiler/passes/routing/commuting_2q_gate_routing/commuting_2q_gate_router.py	1	99.12%

Totals
Change from base Build 9895663732:	-0.2%
Covered Lines:	66679
Relevant Lines:	74319

💛 - Coveralls

ElePT

Thanks @sbrandhsn for looking into this. I am guessing that the root of the issue was related to the instructions being global (i.e. properties=None), but what was exactly failing, was it the pm construction or specific passes? Because as far as I understand it global instructions should be allowed too in the pipeline (so maybe the bug is down the line, not here).

sbrandhsn · 2024-07-15T15:55:28Z

Sure, they might be another issue down the line. However, if the user specifies a backend with a coupling map but without noise info, I guess we should not return a target with global instructions. :-) This is the current behavior of the code I pushed last week.

ElePT · 2024-07-16T13:51:39Z

Both options should be valid (target with global instructions and with local instructions but properties=None) as long as they are properly documented, I guess I was asking whether this was just unexpected behavior for you or whether you saw any failure caused by global instructions that motivated this PR. That being said, I am not against the "unexpected behavior" argument.

ElePT · 2024-07-16T13:56:04Z

qiskit/providers/fake_provider/generic_backend_v2.py

+                if is_fully_connected:
+                    self._target.add_instruction(gate, properties=None, name=name)
+                else:
+                    qarg_set = self._coupling_map if gate.num_qubits > 1 else range(self.num_qubits)
+                    props = {(qarg,) if isinstance(qarg, int) else qarg: None for qarg in qarg_set}
+                    self._target.add_instruction(gate, properties=props, name=name)


hmmm what makes the fully connected cmap special in this case? Is this purely an efficiency matter?

Yep, it is much more efficient to set `properties=None' here in that case, otherwise we create a dictionary with entries for each edge of the fully connected coupling map that all point to None...

I see, but I am not sure I like this approach. This would make the behavior differ "arbitrarily" depending on the connectivity of the coupling map used, right? Some times the instructions would be global, some times they would be local with no properties... I think that the behavior should be consistent independently of the connectivity (plus, if having properties=None does introduce issues in the pipeline, then the issues aren't being fixed for the case of fully-connected cmaps, right?).

I see - the issue here is that the memory overhead is quite large when representing an all-to-all connected by local instructions without properties. It can be up to ~2 GB for large backends whereas a backend with global instructions and no properties needs a couple of MB. Granted, one should likely not create a large all-to-all connected backend but if a user does, he will be surprised by that memory overhead.

However, I agree that a discrete change based on that fully-connected flag is similarly confusing - a backend missing only one edge to be all-to-all connected would exhibit a different memory behaviour than a fully-connected backend. Should I remove the optimizations based on is_fully_connected? I think the pre-PR behaviour of noiseless BackendV2 is definitely incorrect...

I think that it's safer to remove the optimizations for is_fully_connected. It's the default behavior of the class and I can see how users would get confused if specifying a custom cmap suddenly affects the gate properties.

sbrandhsn · 2024-07-16T14:08:04Z

Both options should be valid (target with global instructions and with local instructions but properties=None) as long as they are properly documented, I guess I was asking whether this was just unexpected behavior for you or whether you saw any failure caused by global instructions that motivated this PR. That being said, I am not against the "unexpected behavior" argument.

Sorry for not being clear in my first response. :-/ I was getting an intermittent error that I can not replicate easily right now. I think the gist was that build_coupling_map was called by something in the transpiler which returned None because self.qargs is None. Then some code expected None to be a valid coupling map and called size on it. Interestingly this did not occur for all instance of CouplingMap.from_heavy_hex that I was running. Maybe something in main fixes that behaviour right now.

ElePT

@sbrandhsn I have added some suggestions not to separate out the behavior of the fully connected cmap. Let me know if you agree with the approach.

qiskit/providers/fake_provider/generic_backend_v2.py

Co-authored-by: Elena Peña Tapia <[email protected]>

ElePT

LGTM

…in the transpiler pipeline (#12769) * up * fix * Apply suggestions from code review Co-authored-by: Elena Peña Tapia <[email protected]> * lint --------- Co-authored-by: Elena Peña Tapia <[email protected]> (cherry picked from commit edf0b53)

…in the transpiler pipeline (#12769) (#12823) * up * fix * Apply suggestions from code review Co-authored-by: Elena Peña Tapia <[email protected]> * lint --------- Co-authored-by: Elena Peña Tapia <[email protected]> (cherry picked from commit edf0b53) Co-authored-by: Sebastian Brandhofer <[email protected]>

…in the transpiler pipeline (Qiskit#12769) * up * fix * Apply suggestions from code review Co-authored-by: Elena Peña Tapia <[email protected]> * lint --------- Co-authored-by: Elena Peña Tapia <[email protected]>

up

28cc99e

sbrandhsn requested review from jyu00 and a team as code owners July 12, 2024 12:40

ElePT reviewed Jul 15, 2024

View reviewed changes

fix

d7354fd

ElePT reviewed Jul 16, 2024

View reviewed changes

sbrandhsn added this to the 1.2.0 milestone Jul 23, 2024

ElePT self-assigned this Jul 23, 2024

ElePT reviewed Jul 26, 2024

View reviewed changes

qiskit/providers/fake_provider/generic_backend_v2.py Outdated Show resolved Hide resolved

qiskit/providers/fake_provider/generic_backend_v2.py Outdated Show resolved Hide resolved

sbrandhsn and others added 2 commits July 26, 2024 12:34

Apply suggestions from code review

0bb08aa

Co-authored-by: Elena Peña Tapia <[email protected]>

lint

88d1264

ElePT approved these changes Jul 26, 2024

View reviewed changes

ElePT added Changelog: None Do not include in changelog mod: fake_provider Related to the fake_provider module and fake backends stable backport potential The bug might be minimal and/or import enough to be port to stable labels Jul 26, 2024

ElePT added this pull request to the merge queue Jul 26, 2024

Merged via the queue into Qiskit:main with commit edf0b53 Jul 26, 2024
15 checks passed

mergify bot mentioned this pull request Jul 26, 2024

Setting noise_info to false in GenericBackendV2 creates issues later in the transpiler pipeline (backport #12769) #12823

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting noise_info to false in GenericBackendV2 creates issues later in the transpiler pipeline #12769

Setting noise_info to false in GenericBackendV2 creates issues later in the transpiler pipeline #12769

sbrandhsn commented Jul 12, 2024

qiskit-bot commented Jul 12, 2024

coveralls commented Jul 12, 2024 •

edited

Loading

ElePT left a comment •

edited

Loading

sbrandhsn commented Jul 15, 2024

ElePT commented Jul 16, 2024

ElePT Jul 16, 2024

sbrandhsn Jul 16, 2024

ElePT Jul 23, 2024 •

edited

Loading

sbrandhsn Jul 23, 2024

ElePT Jul 23, 2024

sbrandhsn commented Jul 16, 2024

ElePT left a comment

ElePT left a comment

Setting noise_info to false in GenericBackendV2 creates issues later in the transpiler pipeline #12769

Setting noise_info to false in GenericBackendV2 creates issues later in the transpiler pipeline #12769

Conversation

sbrandhsn commented Jul 12, 2024

Summary

Details and comments

qiskit-bot commented Jul 12, 2024

coveralls commented Jul 12, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10109907875

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ElePT left a comment • edited Loading

Choose a reason for hiding this comment

sbrandhsn commented Jul 15, 2024

ElePT commented Jul 16, 2024

ElePT Jul 16, 2024

Choose a reason for hiding this comment

sbrandhsn Jul 16, 2024

Choose a reason for hiding this comment

ElePT Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

sbrandhsn Jul 23, 2024

Choose a reason for hiding this comment

ElePT Jul 23, 2024

Choose a reason for hiding this comment

sbrandhsn commented Jul 16, 2024

ElePT left a comment

Choose a reason for hiding this comment

ElePT left a comment

Choose a reason for hiding this comment

coveralls commented Jul 12, 2024 •

edited

Loading

ElePT left a comment •

edited

Loading

ElePT Jul 23, 2024 •

edited

Loading