Make all Layout and Routing passes target aware #9263
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit updates all the layout and routing passes which leverage hardware constraints to have a new optional keyword argument, target, to specify a Target object for modelling the constraints of the target backend. This option will superscede any other specified arguments for backend constraints. For most of these passes the target argument is just internally leveraged to build a CouplingMap (and in 2 cases a BackendProperties) as most of the algorithms don't need more than the global connectivity graph and there is minimal overhead to convert from a target to a CouplingMap. The 2 passes which take backend properties should be refactored to work natively with the target because the conversion adds extra overhead which isn't needed, but in those cases the passes aren't enabled by default (or are slated to be moved out of tree as a plugin, see Qiskit#8662) so the extra overhead isn't of particular concern. This is part of the first step towards making all backend constraints for the transpiler used the Target internally (see Qiskit#9256). Once all passes that take hardware constraints parameters as an input are updated to have a target we can start internally using the Target only for all preset pass manager construction and start the long process of deprecating the legacy interface in these passes. Related to Qiskit#9256
mtreinish
added
Changelog: New Feature
|
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:
|
12 tasks
Pull Request Test Coverage Report for Build 4631214929
💛 - Coveralls |
The pass manager drawer tests were comparing the dot output for statically built pass managers to test that the output visualization is generated correctly. However, the new arguments for the taget are changing the visualization (to show the new option) and caused these tests to fail. This commit updates the reference images to include the new argument.
This commit optimizes the logic of the CheckMap pass. Previously we were unecessarily creating a distance matrix from the coupling graph, and with a target unecessarily creating a CouplingMap object. Instead of doing this instead the pass now creates a list of bidirectional edges for adjacent nodes in the coupling graph and then a set lookup is done for each check instead of checking for a distance of 1 in the distance matrix.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jan 4, 2023
This commit updates all the basis, optimization, and util passes which leverage hardware constraints to have a new optional keyword argument, target, to specify a Target object for modelling the constraints of the target backend. This option will superscede any other specified arguments for backend constraints. With this and Qiskit#9263 all the passes except for the scheduling passes which take in backend constraints are made target aware. Making all the passes target aware is the first step towards making all backend constraints for the transpiler used the Target internally (see Qiskit#9256). Once all passes that take hardware constraints parameters as an input are updated to have a target we can start internally using the Target only for all preset pass manager construction and start the long process of deprecating the legacy interface in these passes.
Closed
mtreinish
commented
Jan 18, 2023
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
ajavadia
reviewed
Apr 4, 2023
In an earlier commit we updated the logic in optimization level 0 to favor target instead of a coupling map. But, that ignored the other optimization levels. This commit corrects this oversight and adds the same logically change to all the preset pass managers.
|
fwiw this is the comment I originally mentioned the I think that when we're trying to support arguments being allowed to be passed either positionally or by name, there always end up being some incompatible calling conventions when trying to rename/remove a positional argument, and nothing's perfect. My suggestion involves having a gross name for the main positional argument (would be less of an issue if we didn't have to support Python 3.7 - we could use positional-only arguments), whereas the current form ends up with the unpleasant thing of needing to pass explicit |
jakelishman
reviewed
Apr 6, 2023
There was a problem hiding this comment.
I don't feel like I read the tests especially thoroughly, but they all looked pretty straightforward to me, and look like they're copied from non-Target versions. My only comments here are just minor inconsistencies from the changes this PR has been through, then I'm happy with it.
releasenotes/notes/target-aware-layout-routing-2b39bd87a9f928e7.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/target-aware-layout-routing-2b39bd87a9f928e7.yaml
Outdated
Show resolved
Hide resolved
jakelishman
approved these changes
Apr 6, 2023
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Apr 7, 2023
In the recently merged 5672c70 the CheckMap transpiler pass had issues when handling some Target inputs (which was added as a supported type in that PR). These issues were actually pointed out in code review during Qiskit#9263 but the follow up slipped through. This commit makes the required fixes and also updates the signature of the CheckMap to match the other changes made in Qiskit#9263.
jakelishman
pushed a commit
that referenced
this pull request
Apr 11, 2023
In the recently merged 5672c70 the CheckMap transpiler pass had issues when handling some Target inputs (which was added as a supported type in that PR). These issues were actually pointed out in code review during #9263 but the follow up slipped through. This commit makes the required fixes and also updates the signature of the CheckMap to match the other changes made in #9263.
giacomoRanieri
pushed a commit
to giacomoRanieri/qiskit-terra
that referenced
this pull request
Apr 16, 2023
* Make all Layout and Routing passes target aware This commit updates all the layout and routing passes which leverage hardware constraints to have a new optional keyword argument, target, to specify a Target object for modelling the constraints of the target backend. This option will superscede any other specified arguments for backend constraints. For most of these passes the target argument is just internally leveraged to build a CouplingMap (and in 2 cases a BackendProperties) as most of the algorithms don't need more than the global connectivity graph and there is minimal overhead to convert from a target to a CouplingMap. The 2 passes which take backend properties should be refactored to work natively with the target because the conversion adds extra overhead which isn't needed, but in those cases the passes aren't enabled by default (or are slated to be moved out of tree as a plugin, see Qiskit#8662) so the extra overhead isn't of particular concern. This is part of the first step towards making all backend constraints for the transpiler used the Target internally (see Qiskit#9256). Once all passes that take hardware constraints parameters as an input are updated to have a target we can start internally using the Target only for all preset pass manager construction and start the long process of deprecating the legacy interface in these passes. Related to Qiskit#9256 * Update pass manager drawer reference dot files The pass manager drawer tests were comparing the dot output for statically built pass managers to test that the output visualization is generated correctly. However, the new arguments for the taget are changing the visualization (to show the new option) and caused these tests to fail. This commit updates the reference images to include the new argument. * Fix lint * Optimize CheckMap This commit optimizes the logic of the CheckMap pass. Previously we were unecessarily creating a distance matrix from the coupling graph, and with a target unecessarily creating a CouplingMap object. Instead of doing this instead the pass now creates a list of bidirectional edges for adjacent nodes in the coupling graph and then a set lookup is done for each check instead of checking for a distance of 1 in the distance matrix. * Fix test failures * Update qiskit/transpiler/passes/layout/full_ancilla_allocation.py * Add test coverage for passes not run in preset pass managers * Fix lint * Fix reference dot for new preset pass managers * Rework arguments to take CouplingMap or Target This commit updates the pass constructors to use a single positional argument for both the coupling map or a target. This replaces the previous behavior where a new keyword argument was added to pass in the target. The exception to this is passes where the target was used differently than the coupling map for extra functionality. * Expand test coverage * Small typo fixes from review * Fix lint and test failure * Update qiskit/transpiler/preset_passmanagers/level0.py Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com> * Update logic to favor target in all optimization levels In an earlier commit we updated the logic in optimization level 0 to favor target instead of a coupling map. But, that ignored the other optimization levels. This commit corrects this oversight and adds the same logically change to all the preset pass managers. * Update release note for api changes in earlier iterations * Update docstrings * Use a single positional argument for embed pm function --------- Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
giacomoRanieri
pushed a commit
to giacomoRanieri/qiskit-terra
that referenced
this pull request
Apr 16, 2023
In the recently merged 5672c70 the CheckMap transpiler pass had issues when handling some Target inputs (which was added as a supported type in that PR). These issues were actually pointed out in code review during Qiskit#9263 but the follow up slipped through. This commit makes the required fixes and also updates the signature of the CheckMap to match the other changes made in Qiskit#9263.
king-p3nguin
pushed a commit
to king-p3nguin/qiskit-terra
that referenced
this pull request
May 22, 2023
* Make all Layout and Routing passes target aware This commit updates all the layout and routing passes which leverage hardware constraints to have a new optional keyword argument, target, to specify a Target object for modelling the constraints of the target backend. This option will superscede any other specified arguments for backend constraints. For most of these passes the target argument is just internally leveraged to build a CouplingMap (and in 2 cases a BackendProperties) as most of the algorithms don't need more than the global connectivity graph and there is minimal overhead to convert from a target to a CouplingMap. The 2 passes which take backend properties should be refactored to work natively with the target because the conversion adds extra overhead which isn't needed, but in those cases the passes aren't enabled by default (or are slated to be moved out of tree as a plugin, see Qiskit#8662) so the extra overhead isn't of particular concern. This is part of the first step towards making all backend constraints for the transpiler used the Target internally (see Qiskit#9256). Once all passes that take hardware constraints parameters as an input are updated to have a target we can start internally using the Target only for all preset pass manager construction and start the long process of deprecating the legacy interface in these passes. Related to Qiskit#9256 * Update pass manager drawer reference dot files The pass manager drawer tests were comparing the dot output for statically built pass managers to test that the output visualization is generated correctly. However, the new arguments for the taget are changing the visualization (to show the new option) and caused these tests to fail. This commit updates the reference images to include the new argument. * Fix lint * Optimize CheckMap This commit optimizes the logic of the CheckMap pass. Previously we were unecessarily creating a distance matrix from the coupling graph, and with a target unecessarily creating a CouplingMap object. Instead of doing this instead the pass now creates a list of bidirectional edges for adjacent nodes in the coupling graph and then a set lookup is done for each check instead of checking for a distance of 1 in the distance matrix. * Fix test failures * Update qiskit/transpiler/passes/layout/full_ancilla_allocation.py * Add test coverage for passes not run in preset pass managers * Fix lint * Fix reference dot for new preset pass managers * Rework arguments to take CouplingMap or Target This commit updates the pass constructors to use a single positional argument for both the coupling map or a target. This replaces the previous behavior where a new keyword argument was added to pass in the target. The exception to this is passes where the target was used differently than the coupling map for extra functionality. * Expand test coverage * Small typo fixes from review * Fix lint and test failure * Update qiskit/transpiler/preset_passmanagers/level0.py Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com> * Update logic to favor target in all optimization levels In an earlier commit we updated the logic in optimization level 0 to favor target instead of a coupling map. But, that ignored the other optimization levels. This commit corrects this oversight and adds the same logically change to all the preset pass managers. * Update release note for api changes in earlier iterations * Update docstrings * Use a single positional argument for embed pm function --------- Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
king-p3nguin
pushed a commit
to king-p3nguin/qiskit-terra
that referenced
this pull request
May 22, 2023
In the recently merged 5672c70 the CheckMap transpiler pass had issues when handling some Target inputs (which was added as a supported type in that PR). These issues were actually pointed out in code review during Qiskit#9263 but the follow up slipped through. This commit makes the required fixes and also updates the signature of the CheckMap to match the other changes made in Qiskit#9263.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This commit updates all the layout and routing passes which leverage hardware constraints to have a new optional keyword argument, target, to specify a Target object for modelling the constraints of the target backend. This option will superscede any other specified arguments for backend constraints. For most of these passes the target argument is just internally leveraged to build a CouplingMap (and in 2 cases a BackendProperties) as most of the algorithms don't need more than the global connectivity graph and there is minimal overhead to convert from a target to a CouplingMap. The 2 passes which take backend properties should be refactored to work natively with the target because the conversion adds extra overhead which isn't needed, but in those cases the passes aren't enabled by default (or are slated to be moved out of tree as a plugin, see #8662) so the extra overhead isn't of particular concern.
This is part of the first step towards making all backend constraints for the transpiler used the Target internally (see #9256). Once all passes that take hardware constraints parameters as an input are updated to have a target we can start internally using the Target only for all preset pass manager construction and start the long process of deprecating the legacy interface in these passes.
Details and comments
Related to #9256