Add unitary synthesis plugin interface #6124

mtreinish · 2021-03-31T11:53:12Z

Summary

This commit adds the initial steps for a unitary synthesis plugin
interface. It enables external packages to ship plugin packages that
then integrate cleanly into qiskit's transpiler without any need for
extra imports or qiskit changes. The user can then just specify the
'unitary_synthesis_method' kwarg on the transpile() call and use the
name of the external plugin and the UnitarySynthesis pass will leverage
that plugin for synthesizing the unitary.

Details and comments

TODO:

Add detailed documentation on writing plugins (and how to package them)
Add method to list all available synthesis methods (and expose it in API)

This commit adds the initial steps for a unitary synthesis plugin interface. It enables external packages to ship plugin packages that then integrate cleanly into qiskit's transpiler without any need for extra imports or qiskit changes. The user can then just specify the 'unitary_synthesis_method' kwarg on the transpile() call and use the name of the external plugin and the UnitarySynthesis pass will leverage that plugin for synthesizing the unitary.

1ucian0 · 2021-04-01T09:24:29Z

qiskit/transpiler/passes/synthesis/unitary_synthesis.py

@@ -60,7 +62,9 @@ class UnitarySynthesis(TransformationPass):

    def __init__(self,
                 basis_gates: List[str],
-                 approximation_degree: float = 1):
+                 approximation_degree: float = 1,
+                 coupling_map = None,


Would it be better to pass a PassManagerConfig to the plugin to avoid extending parameters further and further (for example, in case of a potential noise-aware synthesis)? Maybe approximation_degree should be part of PassManagerConfig?

The issue with using a PassManagerConfig is that it is only used by transpile and the preset pass managers? If a user wants to create a custom pass manager or directly call this pass they'll have to go an instantiate this config object to pass to the pass constructor which nothing else requires. For the preset pass manager case I think it makes things simpler but I think we have to consider the standalone case too.

The one thing I thought about doing was making the api free form and just have the abstract run() signature take all kwargs and then the plugins can use them opportunistically so that gives us a bit more flexibility in the api (as we can add parameters over time). But I decided on being explicit like this so we can make the api a bit more well defined.

…into synthesis-plugins

The usefulness of the plugin for unitaries >2q in the default pass manager pipelines was limited by the fact that previously the pass managers were all setup to run unroll3q before the unitary synthesis pass is ever run. The unroll3q pass just calls the circuit.data to decompose gates >=3q which for unitary gate objects just calls an inlined equivalent of the 'default' plugin. The purpose of this is to ensure later passes only ever need to deal with 2q gates. However with plugins now if a plugin only works on > 2q we'll never actually be passing an >= 3q unitaries to the plugin in the default pipelines. To fix this issue, this commit calls unitary synthesis before unroll 3q to use the synthesis pass to unroll any unitary gates instead of relying on the gate class's internal decomposition method which will always use the qiskit decomposition techniques.

This commit adds the UnitarySynthesis pass to the preset pass manager anywhere unrolling of custom or wide gates was done. Previously, we only ever called the UnitarySynthesis pass in the preset pass managers if the basis translation method was set to 'synthesis' or we were using level3 and then it was part of the optimization pass (as part of 2q peephole optimization). This was an issue in that we're implicitly calling the _define() method of the class whenever we're unrolling gates >= 3q or gates with a custom definition. The _define() method is basically identical to the UnitarySynthesis pass except it doesn't expose the options to set a basis gate or approximation degree, which would result in the output gates from unitary gates in the unroll steps are always in the U3 basis. This meant we would be converting unitary gates to u3 (and cx) which would result in a conversion to the target basis later, even if we could just go to the target basis directly. This is also future proofing for Qiskit#6124 where a plugin interface is added to the UnitarySynthesis pass and can potentially be used for arbitrary sized unitaries. At the same time this change caught an issue qith the SingleQubitUnitary gate where the name was duplicated with the UnitaryGate which would result in errors when UnitarySynthesis was called because the UnitarySynthesis pass looks for gate named 'unitary' to run on. This is fixed and the SingleQubitUnitary gate's name is changed to 'squ' to differentiate it from the UnitaryGate.

This commit adds a new PassManager subclass, FullPassManager. This class is used to have a PassManager with a defined structure and stages for the normal transpile workflow. The preset pass managers are then updated to be FullPassManager objects they conform to the fixed structure. Having a class with defined phases gives us flexibility in the future for making the transpiler pluggable with external plugins (similar to what's done in PR Qiskit#6124) and also have backend hook points before or after different phases of the transpile. Fixes Qiskit#5978

adekusar-drl · 2021-05-28T11:37:52Z

Hi, I'm looking forward to see this plugin merged. I have a suggestion on the plugin interface. Here, https://github.com/Qiskit/qiskit-terra/pull/6124/files#diff-8e9b7b00856cfb221b326a03419bb4640f8c17f9da7612841224f2028602a9d4R163, there's a unitary matrix as an input in the interface. Matrix representation may be a limitation if we want to extend this plugin to a larger unitaries, say, 10 or more qubits. So, could it be better to pass a DAG or something else as a parameter?

jakelishman

Almost all minor documentation comments (mostly with click-able suggestions). The two non-trivial points are

should supported_basis be the plural supported_bases?
would it be better to have the keyword coupling_map be a 2-tuple of the map and what is currently the keyword qubits?

qiskit/transpiler/passes/synthesis/plugin.py

qiskit/transpiler/passes/synthesis/unitary_synthesis.py

Co-authored-by: Jake Lishman <jake@binhbar.com>

This commit updates several aspects of the documentation to make it more clear how to use the plugin interface. It also updates the naming of supported_basis to be supported_bases as it's actually plural. The last change in this commit is making the default kwarg value for the unitary synthesis method default to 'default' (for the default built-in method) instead of None. This simplifies the logic in the unitary synthesis pass.

jakelishman

Very close to approval from me. I pushed a commit fixing a couple of straggler basis -> bases typos. It looks like there's a minor bug in _choose_bases, and probably one bit of extra documentation/behaviour definition we need about that, but other than those minor points, I'm ready to merge.

qiskit/transpiler/passes/synthesis/plugin.py

qiskit/transpiler/passes/synthesis/unitary_synthesis.py

Co-authored-by: Jake Lishman <jake@binhbar.com>

jakelishman

Looks good to me now.

Conflicts: constraints.txt Merged by keeping both new constraints at the bottom of the file.

jakelishman

Fixed constraints.txt merge conflict.

In Qiskit#6124 which recently merged the UnitarySynthesis pass was added to preset passmanager to enable synthesis plugins that operate on > 2q from having an impact (otherwise they wouldn't be called). However that PR didn't have the option to avoid synthesizing 2q unitaries too early. This commit updates the base branch to the latest state of main which includes Qiskit#6124. This means this PR just adds a min_qubits option to UnitarySynthesis and leverages it for the unroll3q stage in the preset pass managers.

* Rely on UnitarySynthesis pass for unrolling UnitaryGates This commit adds the UnitarySynthesis pass to the preset pass manager anywhere unrolling of custom or wide gates was done. Previously, we only ever called the UnitarySynthesis pass in the preset pass managers if the basis translation method was set to 'synthesis' or we were using level3 and then it was part of the optimization pass (as part of 2q peephole optimization). This was an issue in that we're implicitly calling the _define() method of the class whenever we're unrolling gates >= 3q or gates with a custom definition. The _define() method is basically identical to the UnitarySynthesis pass except it doesn't expose the options to set a basis gate or approximation degree, which would result in the output gates from unitary gates in the unroll steps are always in the U3 basis. This meant we would be converting unitary gates to u3 (and cx) which would result in a conversion to the target basis later, even if we could just go to the target basis directly. This is also future proofing for #6124 where a plugin interface is added to the UnitarySynthesis pass and can potentially be used for arbitrary sized unitaries. At the same time this change caught an issue qith the SingleQubitUnitary gate where the name was duplicated with the UnitaryGate which would result in errors when UnitarySynthesis was called because the UnitarySynthesis pass looks for gate named 'unitary' to run on. This is fixed and the SingleQubitUnitary gate's name is changed to 'squ' to differentiate it from the UnitaryGate. * Add option to set a minimum size to unitarysynthesis pass This commit adds a new option to the UnitarySynthesis pass constructor, min_qubits, which is used to specify a minimimum size unitary to synthesize. If the unitary is smaller than that it will be skipped. This is then used by the UnitarySynthesis instance in the unroll3q phase so we don't decompose 1 or 2q unitaries before routing. * Fix rebase error * Correct oversights from rebase Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

This commit tweaks the preset passmanager definitions to not include the UnitarySynthesis pass as part of basis translation if the 'unitary' gate is in the basis gates. Previously in Qiskit#6124 and Qiskit#6349 the preset passmanagers were updated to leverage the user specified unitary synthesis method instead of implicitly using the default as part of the unroll 3q and unroll custom definition passes. This however had the unintended side effect of always synthesizing unitary gates even if it was in the basis (which is the case on Aer). This commit fixes this by only running unitary synthesis as part of the basis translation step if it's not in the basis. A potential follow-on here is to make the unroll 3q transpiler pass basis aware (since right now it will implicitly run unitary synthesis internally) and add a similar logic around the use of the UnitarySynthesis pass for unrolling 3q or larger gates. The unroll 3q transpiler stage suffers from this same issue. However, this wasn't done here because the way 3q unrolling is used is to reduce the gates to be less than 3 qubits so the layout and routing phase can deal work with the gates in the circuit would likely cause issues if a unitary gate larger than 2 qubits was in the circuit being transpiled.

@jakelishman

As was pointed out by @jakelishman in code review. Prior to Qiskit#6124 the unitary synthesis pass was checking the basis for the presence of unitary (and swap gates if pulse_optimize=True) in the basis set and skipping the gates in the pass directly instead of in the pass manager. This logic was lost when we added the plugin interface in Qiskit#6124 (probably as part of a rebase as it was a long lived branch). Since we used to have the logic in the pass itself this commit changes the approach to not adjust the preset passmanager usage and just make the pass detect if unitary is in the basis and not synthesize. This simplifies the logic and makes it less error prone. At the same time the test coverage is expanded to ensure we preserve this behavior moving forward (as aer's testing requires it).

@jakelishman

) * Ensure UnitaryGate's are preserved by transpile if in basis gates This commit tweaks the preset passmanager definitions to not include the UnitarySynthesis pass as part of basis translation if the 'unitary' gate is in the basis gates. Previously in #6124 and #6349 the preset passmanagers were updated to leverage the user specified unitary synthesis method instead of implicitly using the default as part of the unroll 3q and unroll custom definition passes. This however had the unintended side effect of always synthesizing unitary gates even if it was in the basis (which is the case on Aer). This commit fixes this by only running unitary synthesis as part of the basis translation step if it's not in the basis. A potential follow-on here is to make the unroll 3q transpiler pass basis aware (since right now it will implicitly run unitary synthesis internally) and add a similar logic around the use of the UnitarySynthesis pass for unrolling 3q or larger gates. The unroll 3q transpiler stage suffers from this same issue. However, this wasn't done here because the way 3q unrolling is used is to reduce the gates to be less than 3 qubits so the layout and routing phase can deal work with the gates in the circuit would likely cause issues if a unitary gate larger than 2 qubits was in the circuit being transpiled. * Skip level 3 in tests The level 3 preset passmanager by default runs 2q block collection which is then synthesized into a unitary. As long as we're running that in the pass manager it's not really possible right now to preserve input unitaries through the transpilation. A potential follow on PR could potential skip the synthesis pass if 'unitary' is in the basis so the optimization pass would just collect 2q blocks into a unitary and pass that to the backend directly. However, that's not in scope for this PR (and it's not clear if we want that or not). * Revert to main preset passmanager logic As was pointed out by @jakelishman in code review. Prior to #6124 the unitary synthesis pass was checking the basis for the presence of unitary (and swap gates if pulse_optimize=True) in the basis set and skipping the gates in the pass directly instead of in the pass manager. This logic was lost when we added the plugin interface in #6124 (probably as part of a rebase as it was a long lived branch). Since we used to have the logic in the pass itself this commit changes the approach to not adjust the preset passmanager usage and just make the pass detect if unitary is in the basis and not synthesize. This simplifies the logic and makes it less error prone. At the same time the test coverage is expanded to ensure we preserve this behavior moving forward (as aer's testing requires it). * Add level 3 back to tests * Improve efficieny if gates to synthesize are in basis

* Add FullPassManager class for pass manager with defined stages This commit adds a new PassManager subclass, FullPassManager. This class is used to have a PassManager with a defined structure and stages for the normal transpile workflow. The preset pass managers are then updated to be FullPassManager objects they conform to the fixed structure. Having a class with defined phases gives us flexibility in the future for making the transpiler pluggable with external plugins (similar to what's done in PR #6124) and also have backend hook points before or after different phases of the transpile. Fixes #5978 * Add docs * Deduplicate preset passmanager construction * Update docs * Add dedicated scheduling stage to FullPassManager * Add missing new UnitarySynthesis kwargs after rebase * Use basis_translator as default method instead of basis Co-authored-by: Kevin Krsulich <kevin@krsulich.net> * Rename FullPassManager StructuredPassManager * Rename generate_scheduling_post_opt() generate_scheduling() * Fix missing and incorrect arguments * Fix more rebase issues * Fix even more rebase issues * Only run unroll3q on level 0-2 if coupling map is set To preserve the behavior prior to this reorganization this commit makes the Unroll3qorMore pass run if we have a coupling map set. This is because this pass is only needed to be run for these optimization levels so that the layout and routing passes can function (as they only work with 2q gates). If we're not running these passes we shouldn't be unrolling gates. This will fix the last 4 QAOA test failures post the recent rebase as that was failing because we were trying to unroll an unbound 4q hamiltonian gate when weren't before. * Rework StructuredPassManager as a more dynamic StagedPassManager This commit redefines the previous StructuredPassManager class into a more dynamic StagedPassmanager. The StagedPassManager doesn't have fixed hard coded stages anymore but instead lets users define their own stages. It also adds implicit 'pre_' and 'post_' hook points for each listed stage. This lets users dynamically define the stages based on a particular use case. * Fix docs * Update internal pass set on each access This commit updates the api for the StagedPassManager to refresh the internal pass list on each access. This adds a little overhead but with the tradeoff of making in place modifications to a stage's pass manager reflected without needing to manually call an update method. * Rename phases attribute to stages * Fix lint * Explicitly set name in qpy compat tests The rework of the preset passmanager construction changes the import order slightly and the number of circuits constructed in a session prior to the qpy compat deserialization side generating equivalent circuits for comparision has changed. This is causing all the deserialization side numbers to change and the tests are now failing because the circuit names are not equivalent. Since the exact name is a side effect of the import order (based on the number of unnamed circuits created in the session priort) it's not part of what the qpy tests are validating. We're trying to assert that the naem is preserved loading a qpy file across a version boundary. To fix this issues this commit adds an explicit name to the generation for all the circuits to ensure that we have a deterministic name for each circuit. * Apply suggestions from code review Co-authored-by: Luciano Bello <bel@zurich.ibm.com> * Run black * Update type hint * Remove out of date docstring note * Update copyright header date in qiskit/transpiler/preset_passmanagers/common.py Co-authored-by: Luciano Bello <bel@zurich.ibm.com> * Add check for invalid stage names * Add backwards compatibility note * Add docs on using StagedPassManager features with preset passmanagers Co-authored-by: Kevin Krsulich <kevin@krsulich.net> Co-authored-by: Luciano Bello <bel@zurich.ibm.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

mtreinish added the on hold Can not fix yet label Mar 31, 2021

mtreinish added this to the 0.18 milestone Mar 31, 2021

mtreinish requested a review from a team as a code owner March 31, 2021 11:53

mtreinish force-pushed the synthesis-plugins branch from 4e06c2d to ad04aaa Compare March 31, 2021 15:14

1ucian0 reviewed Apr 1, 2021

View reviewed changes

mtreinish added 6 commits April 13, 2021 10:25

Also add qubits in payload with coupling map

1fb017b

Fix lint

1da2d87

Fix logic

0532a1e

Add documentation about the plugin interface

19bb0c8

Export get_unitary_synthesis_plugin_names from qiskit.transpiler.passes

fece875

Merge branch 'master' into synthesis-plugins

d9eda3b

mtreinish changed the title ~~[WIP] Add unitary synthesis plugin interface~~ Add unitary synthesis plugin interface Apr 14, 2021

mtreinish removed the on hold Can not fix yet label Apr 14, 2021

mtreinish added 5 commits April 15, 2021 07:05

Fix lint

b7678cf

Tweak doc organization slightly

5622895

Merge branch 'synthesis-plugins' of github.com:mtreinish/qiskit-core …

566ebcf

…into synthesis-plugins

Merge branch 'master' into synthesis-plugins

0bb935e

mtreinish mentioned this pull request May 4, 2021

Add min_qubits kwarg to UnitarySynthesis pass #6349

Merged

mtreinish mentioned this pull request May 12, 2021

Add StagedPassManager class for pass manager with defined stages #6403

Merged

mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label May 12, 2021

mtreinish added 2 commits May 28, 2021 08:10

Merge remote-tracking branch 'origin/main' into synthesis-plugins

7808223

Run black

dd9211f

jakelishman reviewed Sep 29, 2021

View reviewed changes

mtreinish and others added 7 commits September 29, 2021 13:30

Apply suggestions from code review

0e06b7d

Co-authored-by: Jake Lishman <jake@binhbar.com>

Update qiskit/transpiler/passes/synthesis/unitary_synthesis.py

d9f6884

Co-authored-by: Jake Lishman <jake@binhbar.com>

Merge branch 'main' into synthesis-plugins

0e5c637

Make coupling_map run() kwarg a tuple

aa2af0f

Add comment on why we don't use a shared global instance

ba6a4a0

Fix straggler basis->bases typos

66bb763

jakelishman reviewed Sep 30, 2021

View reviewed changes

qiskit/transpiler/passes/synthesis/plugin.py Outdated Show resolved Hide resolved

qiskit/transpiler/passes/synthesis/unitary_synthesis.py Outdated Show resolved Hide resolved

qiskit/transpiler/passes/synthesis/unitary_synthesis.py Outdated Show resolved Hide resolved

mtreinish and others added 2 commits September 30, 2021 12:40

Apply suggestions from code review

7240065

Co-authored-by: Jake Lishman <jake@binhbar.com>

Document supported_bases handling of no matching basis

9ca591a

jakelishman previously approved these changes Sep 30, 2021

View reviewed changes

Merge remote-tracking branch 'ibm/main' into pr/6124

9c12540

Conflicts: constraints.txt Merged by keeping both new constraints at the bottom of the file.

jakelishman dismissed their stale review via 9c12540 September 30, 2021 17:57

jakelishman approved these changes Sep 30, 2021

View reviewed changes

mtreinish added the automerge label Sep 30, 2021

mergify bot merged commit 4eda799 into Qiskit:main Sep 30, 2021

mtreinish deleted the synthesis-plugins branch October 1, 2021 13:22

mtreinish mentioned this pull request Oct 5, 2021

Ensure UnitaryGate's are preserved by transpile if in basis gates #7104

Merged

ajavadia mentioned this pull request Oct 7, 2021

error from unitary plugin interface #7108

Closed

mtreinish mentioned this pull request Oct 27, 2021

Out-of-date BQSKit mtreinish/bqskit-qiskit-synthesis-plugin#1

Open

1ucian0 mentioned this pull request Jul 1, 2022

plugin interface for Clifford synthesis #8284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unitary synthesis plugin interface #6124

Add unitary synthesis plugin interface #6124

mtreinish commented Mar 31, 2021 •

edited

Loading

1ucian0 Apr 1, 2021

mtreinish Apr 1, 2021

adekusar-drl commented May 28, 2021

jakelishman left a comment

jakelishman left a comment

jakelishman left a comment

jakelishman left a comment

Add unitary synthesis plugin interface #6124

Add unitary synthesis plugin interface #6124

Conversation

mtreinish commented Mar 31, 2021 • edited Loading

Summary

Details and comments

1ucian0 Apr 1, 2021

Choose a reason for hiding this comment

mtreinish Apr 1, 2021

Choose a reason for hiding this comment

adekusar-drl commented May 28, 2021

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

mtreinish commented Mar 31, 2021 •

edited

Loading