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

Add option to SabreLayout to specify starting layouts #10721

Merged
merged 17 commits into from
Sep 12, 2023

Conversation

mtreinish
Copy link
Member

Summary

The SabreLayout pass typically starts with each layout trial with a random starting layout. However, in some cases starting with a specific starting layout can result in better output quality than starting with a fully random layout. To use this feature an analysis pass can set a new sabre_starting_layout field in the property set before SabreLayout with a list of layout objects that will add additional trials using each layout as a starting layout.

Details and comments

@mtreinish mtreinish added Changelog: New Feature Include in the "Added" section of the changelog Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler labels Aug 28, 2023
@mtreinish mtreinish added this to the 0.45.0 milestone Aug 28, 2023
@mtreinish mtreinish requested a review from a team as a code owner August 28, 2023 16:14
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

@coveralls
Copy link

coveralls commented Aug 28, 2023

Pull Request Test Coverage Report for Build 6160408129

  • 53 of 54 (98.15%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 87.294%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/sabre_layout.rs 34 35 97.14%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
crates/qasm2/src/lex.rs 1 91.16%
Totals Coverage Status
Change from base Build 6160358090: 0.02%
Covered Lines: 74523
Relevant Lines: 85370

💛 - Coveralls

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I think this is a good change, and certainly useful for research into how different initial layouts affect the final layout / routing.

crates/accelerate/src/sabre_layout.rs Outdated Show resolved Hide resolved
Comment on lines 244 to 265
inner_run = self._inner_run
if "sabre_starting_layout" in self.property_set:
inner_run = functools.partial(
self._inner_run, starting_layout=self.property_set["sabre_starting_layout"]
)

Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming the idea of doing this rather than having it be a direct input to the SabreLayout is so a transpiler pass can look at the hardware / circuit and make a choice. If we're starting to go down that route with passes (and we've already walked some of it with ApplyLayout, etc), perhaps we should consider formalising some sort of scope / namespacing in the property set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, typically the layout passes are dependent on the input circuit so using the property set was really the only clean way I could think of integrating it. The other option would be to take an optional callable input on __init__ that is given a DAGCircuit and returns a layout. But the property set approach felt like a better fit for the transpiler. I think we can investigate adding more structure to the property set in a follow up. It's more than just apply layout, this same pattern also exists with the vf2 passes too because they can take in an optional error map from the property set too.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah I agree, the property set is the right way of feeding it in to a pass, if not only because it's what we already do (as you say).

qiskit/transpiler/passes/layout/sabre_layout.py Outdated Show resolved Hide resolved
mtreinish and others added 4 commits August 29, 2023 08:11
The SabreLayout pass typically starts with each layout trial with a
random starting layout. However, in some cases starting with a specific
starting layout can result in better output quality than starting with
a fully random layout. To use this feature an analysis pass can set a
new `sabre_starting_layout` field in the property set before
`SabreLayout` with a list of layout objects that will add additional
trials using each layout as a starting layout.
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
jakelishman
jakelishman previously approved these changes Aug 29, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I think most of my stuff is just nitpicks, so feel free to ignore. I'm fine to merge as is, but I'd want Sasha to approve as well before, since this is intended for work he's doing as well.

crates/accelerate/src/sabre_layout.rs Outdated Show resolved Hide resolved
crates/accelerate/src/sabre_layout.rs Outdated Show resolved Hide resolved
Comment on lines 131 to 152
let physical_qubits = if !starting_layout.is_empty() {
let used_bits: HashSet<usize> = starting_layout
.iter()
.filter_map(|x| x.as_ref())
.copied()
.collect();
let mut free_bits: Vec<usize> = (0..num_physical_qubits)
.filter(|x| !used_bits.contains(x))
.collect();
free_bits.shuffle(&mut rng);
(0..num_physical_qubits)
.map(|x| match starting_layout.get(x) {
Some(phys) => phys.unwrap_or_else(|| free_bits.pop().unwrap()),
None => free_bits.pop().unwrap(),
})
.collect()
} else {
let mut physical_qubits: Vec<usize> = (0..num_physical_qubits).collect();
physical_qubits.shuffle(&mut rng);
physical_qubits
};

Copy link
Member

Choose a reason for hiding this comment

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

Minor, but I don't think we even need the else branch, right? The logic should work even if there's no used_bits.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we don't need it, it would work fine without the if statement. But, I figured it was a bit more efficient to leave it like this. That being said I didn't measure it so I don't actually know how much overhead having a branch here vs doing creating 3 objects (one empty) and doing 2 iterations instead of one has. I can measure it to see if it's worth the extra code or not

qiskit/transpiler/passes/layout/sabre_layout.py Outdated Show resolved Hide resolved
Co-authored-by: Jake Lishman <jake@binhbar.com>
jakelishman
jakelishman previously approved these changes Aug 29, 2023
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

(same comment about waiting for Sasha)

jakelishman
jakelishman previously approved these changes Aug 30, 2023
alexanderivrii
alexanderivrii previously approved these changes Sep 11, 2023
Copy link
Contributor

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks, this is all very nice! I especially like the fact that one can write multiple "guess the initial layout" passes, each pass adding its own layout(s) to property_set["sabre_starting_layout"], and then SabreLayout would examine all of these in addition to the prescribed number of random layouts.

The only super minor question if you think that these initial layouts may eventually make sense also outside of sabre; if so, maybe let's call these "starting_layout" and not "sabre_starting_layout"? Also, should it be "sabre_starting_layouts" to emphasize that it's a list; the same for the new option in SabreLayout's _inner_run. But please feel free to ignore this.

@@ -312,7 +346,7 @@ def run(self, dag):
disjoint_utils.combine_barriers(mapped_dag, retain_uuid=False)
return mapped_dag

def _inner_run(self, dag, coupling_map):
def _inner_run(self, dag, coupling_map, starting_layout=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be slightly clearer to call this starting_layouts instead of starting_layout?

@mtreinish
Copy link
Member Author

The only super minor question if you think that these initial layouts may eventually make sense also outside of sabre; if so, maybe let's call these "starting_layout" and not "sabre_starting_layout"? Also, should it be "sabre_starting_layouts" to emphasize that it's a list; the same for the new option in SabreLayout's _inner_run. But please feel free to ignore this.

I'm not sure there is an application for this outside of sabre layout. Our other in tree layout passes don't really work with a starting layout the way sabre does. I'm having a hard time coming up with another use case where we'd want to feed an unapplied layout as an input to a pass like this does (even VF2PostLayout needs the layout to be applied and routed to work). I think namespacing it to sabre makes sense to clearly indicate that.

As for the variable names yeah you're correct they should probably be plural. I'll update the properties and arguments to be plural starting_layouts instead of singular starting_layout as that is more descriptive as it's a list.

The starting layout is a list and can have more than one entry. To make
this clearer, this commit renames starting_layout -> starting_layouts
and sabre_starting_layout -> sabre_starting_layouts.
Comment on lines 256 to 266
class DensePartialSabreTrial(AnalysisPass):
"""Pass to run dense layout as a sabre trial."""

def __init__(self, cmap):
self.dense_pass = DenseLayout(cmap)
super().__init__()

def run(self, dag):
self.dense_pass.run(dag)
self.property_set["sabre_starting_layout"] = [self.dense_pass.property_set["layout"]]

Copy link
Member

Choose a reason for hiding this comment

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

Given that this test isn't failing while it's still setting the incorrect property-set entry (no terminal "s"), I'm a bit concerned that the test isn't doing its job haha.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, well it was too tricky to come up with an example where using dense layout actually was the selected layout. The full random was almost always a better starting point, so the test just really to exercise the code paths but couldn't enforce they were used easily because we lose it to rust space at the point wed want to assert anything.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes - now that Sasha's said he's happy too I'm good to merge.

@jakelishman jakelishman added this pull request to the merge queue Sep 12, 2023
Merged via the queue into Qiskit:main with commit 578e109 Sep 12, 2023
14 checks passed
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 22, 2024
Building on the work done in Qiskit#10829, Qiskit#10721, and Qiskit#12104 this commit adds
a new trial to all runs of `SabreLayout` that runs the dense layout
pass. In general the sabre layout algorithm starts from a random layout
and then runs a routing algorithm to permute that layout virtually where
swaps would be inserted to select a layout that would result in fewer
swaps. As was discussed in Qiskit#10721 and Qiskit#10829 that random starting point
is often not ideal especially for larger targets where the distance
between qubits can be quite far. Especially when the circuit qubit count
is low relative to the target's qubit count this can result it poor
layouts as the distance between the qubits is too large. In qiskit we
have an existing pass, `DenseLayout`, which tries to find the most
densely connected n qubit subgraph of a connectivity graph. This
algorithm necessarily will select a starting layout where the qubits are
near each other and for those large backends where the random starting
layout doesn't work well this can improve the output quality.

As the overhead of `DenseLayout` is quite low and the core algorithm is
written in rust already this commit adds a default trial that uses
DenseLayout as a starting point on top of the random trials (and any
input starting points). For example if the user specifies to run
SabreLayout with 20 layout trials this will run 20 random trials and
one trial with `DenseLayout` as the starting point. This is all done
directly in the sabre layout rust code for efficiency. The other
difference between the standalone `DenseLayout` pass is that in the
standalone pass a sparse matrix is built and a reverse Cuthill-McKee
permutation is run on the densest subgraph qubits to pick the final
layout. This permutation is skipped because in the case of Sabre's
use of dense layout we're relying on the sabre algorithm to perform
the permutation.

Depends on: Qiskit#12104
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request May 22, 2024
Building on the work done in Qiskit#10829, Qiskit#10721, and Qiskit#12104 this commit adds
a new trial to all runs of `SabreLayout` that runs the dense layout
pass. In general the sabre layout algorithm starts from a random layout
and then runs a routing algorithm to permute that layout virtually where
swaps would be inserted to select a layout that would result in fewer
swaps. As was discussed in Qiskit#10721 and Qiskit#10829 that random starting point
is often not ideal especially for larger targets where the distance
between qubits can be quite far. Especially when the circuit qubit count
is low relative to the target's qubit count this can result it poor
layouts as the distance between the qubits is too large. In qiskit we
have an existing pass, `DenseLayout`, which tries to find the most
densely connected n qubit subgraph of a connectivity graph. This
algorithm necessarily will select a starting layout where the qubits are
near each other and for those large backends where the random starting
layout doesn't work well this can improve the output quality.

As the overhead of `DenseLayout` is quite low and the core algorithm is
written in rust already this commit adds a default trial that uses
DenseLayout as a starting point on top of the random trials (and any
input starting points). For example if the user specifies to run
SabreLayout with 20 layout trials this will run 20 random trials and
one trial with `DenseLayout` as the starting point. This is all done
directly in the sabre layout rust code for efficiency. The other
difference between the standalone `DenseLayout` pass is that in the
standalone pass a sparse matrix is built and a reverse Cuthill-McKee
permutation is run on the densest subgraph qubits to pick the final
layout. This permutation is skipped because in the case of Sabre's
use of dense layout we're relying on the sabre algorithm to perform
the permutation.

Depends on: Qiskit#12104
github-merge-queue bot pushed a commit that referenced this pull request May 22, 2024
Building on the work done in #10829, #10721, and #12104 this commit adds
a new trial to all runs of `SabreLayout` that runs the dense layout
pass. In general the sabre layout algorithm starts from a random layout
and then runs a routing algorithm to permute that layout virtually where
swaps would be inserted to select a layout that would result in fewer
swaps. As was discussed in #10721 and #10829 that random starting point
is often not ideal especially for larger targets where the distance
between qubits can be quite far. Especially when the circuit qubit count
is low relative to the target's qubit count this can result it poor
layouts as the distance between the qubits is too large. In qiskit we
have an existing pass, `DenseLayout`, which tries to find the most
densely connected n qubit subgraph of a connectivity graph. This
algorithm necessarily will select a starting layout where the qubits are
near each other and for those large backends where the random starting
layout doesn't work well this can improve the output quality.

As the overhead of `DenseLayout` is quite low and the core algorithm is
written in rust already this commit adds a default trial that uses
DenseLayout as a starting point on top of the random trials (and any
input starting points). For example if the user specifies to run
SabreLayout with 20 layout trials this will run 20 random trials and
one trial with `DenseLayout` as the starting point. This is all done
directly in the sabre layout rust code for efficiency. The other
difference between the standalone `DenseLayout` pass is that in the
standalone pass a sparse matrix is built and a reverse Cuthill-McKee
permutation is run on the densest subgraph qubits to pick the final
layout. This permutation is skipped because in the case of Sabre's
use of dense layout we're relying on the sabre algorithm to perform
the permutation.

Depends on: #12104
ElePT pushed a commit to ElePT/qiskit that referenced this pull request May 31, 2024
Building on the work done in Qiskit#10829, Qiskit#10721, and Qiskit#12104 this commit adds
a new trial to all runs of `SabreLayout` that runs the dense layout
pass. In general the sabre layout algorithm starts from a random layout
and then runs a routing algorithm to permute that layout virtually where
swaps would be inserted to select a layout that would result in fewer
swaps. As was discussed in Qiskit#10721 and Qiskit#10829 that random starting point
is often not ideal especially for larger targets where the distance
between qubits can be quite far. Especially when the circuit qubit count
is low relative to the target's qubit count this can result it poor
layouts as the distance between the qubits is too large. In qiskit we
have an existing pass, `DenseLayout`, which tries to find the most
densely connected n qubit subgraph of a connectivity graph. This
algorithm necessarily will select a starting layout where the qubits are
near each other and for those large backends where the random starting
layout doesn't work well this can improve the output quality.

As the overhead of `DenseLayout` is quite low and the core algorithm is
written in rust already this commit adds a default trial that uses
DenseLayout as a starting point on top of the random trials (and any
input starting points). For example if the user specifies to run
SabreLayout with 20 layout trials this will run 20 random trials and
one trial with `DenseLayout` as the starting point. This is all done
directly in the sabre layout rust code for efficiency. The other
difference between the standalone `DenseLayout` pass is that in the
standalone pass a sparse matrix is built and a reverse Cuthill-McKee
permutation is run on the densest subgraph qubits to pick the final
layout. This permutation is skipped because in the case of Sabre's
use of dense layout we're relying on the sabre algorithm to perform
the permutation.

Depends on: Qiskit#12104
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: transpiler Issues and PRs related to Transpiler Rust This PR or issue is related to Rust code in the repository
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

5 participants