Fix determinism in ConsolidateBlocks#16237
Conversation
alexanderivrii
added
the
Changelog: Fixed
|
One or more of the following people are relevant to this code:
|
Coverage Report for CI Build 26275374846Coverage decreased (-0.005%) to 87.489%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions13 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
You might be able to get away with writing a test that would randomly decompose between two options if the test is flaky, but reliably select a particular one if it's fixed. It wouldn't fail 100% of the time in a single test-suite run, but it'd have enough chance of failure it'd almost certainly be caught by any CI run.
That said, this isn't the end of the world if we can't write a test for it: it's fairly unlikely to accidentally reintroduce the bug.
| Fixed a problem in :class:`.ConsolidateBlocks` where basis gate selection was not always | ||
| deterministic, which could lead to different transpiled circuits across runs. |
There was a problem hiding this comment.
Can you add a quick mention of the configuration needed to make it non-deterministic too? I think most "common" use of the pass was always deterministic.
| kak_gates = KAK_GATE_NAMES.keys() & (basis_gates or []) | ||
| kak_param_gates = KAK_GATE_PARAM_NAMES.keys() & (basis_gates or []) | ||
| if kak_param_gates: | ||
| kak_gate = next((gate for gate in KAK_GATE_NAMES if gate in basis_gates), None) | ||
| kak_param_gate = next( | ||
| (gate for gate in KAK_GATE_PARAM_NAMES if gate in basis_gates), None | ||
| ) |
There was a problem hiding this comment.
Incredibly minor, but since KAK_GATE_NAMES and KAK_GATE_PARAM_NAMES are reliably dict, but basis_gates is a list, you might want to drive the iteration from basis_gates and lookup into KAK_GATE_NAMES instead, especially if you're in the habit of passing huge basis_gates lists.
4 participants
Summary
While experimenting with the Clifford+T benchmarks (in an ongoing experiment with @jan-an and @Cryoris), I noticed that some runs were not deterministic (for the same value of transpiler seed) and managed to pinpoint the problem to which basis gate is selected inside the
ConsolidateBlock.__init__method).The problem occurs when there are multiple parametric or multiple non-parametric 2-qubit gates to choose from. While the dict
KAK_GATE_NAMES = {"cx": CXGate(), "cz": CZGate(), "iswap": iSwapGate(), "ecr": ECRGate()}has a deterministic iteration order, the computationKAK_GATE_NAMES.keys() & (basis_gates or [])returns a set, and its first elementnext(iter(kak_param_gates))might be different across runs.In Clifford+T transpilation, we often pass all 2-qubit Clifford gates as basis, leading to a selection from CZ, CX, ECR and iSWAP, however this problem could be also present for continuous basis sets where both CX and CZ can be present (thanks @mtreinish for the information).
Details
The current (very simple) approach chooses the first gate from
KAK_GATE_NAMESthat is also in thebasis_gates(and same for parametric gates).I did not change the order of gates in the dict, so for Clifford+T transpilation with all Clifford basis gates this ends up always using the CX-gate. However, on FT benchmarks using CZ seems consistently better than using CX (at least with the rest of our pipeline intact):
I am wondering if I should add an extra argument to ConsolidateBlocks to make it "Clifford+T -transpilation aware"?
I am not sure what's the best test to add here as non-deterministic behavior occurs across different Qiskit runs (see #16139 for a similar problem).
AI/LLM disclosure