Use toposort instead of topological_op_nodes for DAG reconstruction#15987
Use toposort instead of topological_op_nodes for DAG reconstruction#15987mtreinish wants to merge 7 commits into
Conversation
This commit switches the topological sort function we use in transpiler passes when reconstructing a dag from scratch. In several passes where we typically replace or remove a large numbers of gates we iterate over the input dag in topological order and construct a copy of it making the alterations as we go. Right now when we do this we rely on `DAGCircuit::topological_op_nodes` which makes sense because it's or built-in method for iterating over a dag's op node in topological ordering. Internally this uses rustworkx's lexicographical topological sort function with our custom sort function that maintains our desired tie breaker using the bits of a node. However, since Qiskit#14762 where we're asserting structural equality in passes we don't need to use that sort anymore for these reconstruction cases. We just need a consistent topological sort. In optimizing Qiskit#13419 one thing that showed in profiles was that for very large circuits the overhead of the lexicographical topological sort for the iteration. The toposort function in petgraph is lower overhead because it doesn't need to work about the lexicographical tie breaker. By switching to use this instead we can reduce the overhead of the final sort in all these passes. In asv benchmarking this commit speeds up transpiler benchmarks are 2-5% faster (although without asv flagging it as significant).
mtreinish
added
Changelog: None
|
One or more of the following people are relevant to this code:
|
jakelishman
left a comment
jakelishman
left a comment
There was a problem hiding this comment.
Got to say I don't love this in the way it's been done. I don't like all these extra explicit calls to unwrap just to call what should be an infallible function, and I'm not a fan of having a specific DAGCircuit::topological_op_nodes function that you shouldn't use.
Is there any way we can fix topological_op_nodes in place?
This does not replace all the uses of That being said I can refactor this into a rebuilding method instead. Where you give it a callback that is passed every node and it will return a new dag instance based on that. Since we do this enough times having a streamlined api makes sense anyway. |
This commit adds a new method to DAGCircuit that is designed to replace the explicit rebuilding of a dag that is done in several passes. The previous commit updated the common pattern of rebuilding the dag to use toposort instead of topological_sort, but this wasn't the cleanest interface and it was resulting in some ugliness between the API boundaries. To facilitate making the more performant path easier to use correctly this commit opts to just extract the common pattern into a method that enables performant rebuilding of a dag.
Cryoris
left a comment
Cryoris
left a comment
There was a problem hiding this comment.
I'm curious: do you think a rebuild interface is better than just providing an iterator? The iterator seems more general since you might just analyze the DAG in topo order without rebuilding it. You could ofc misuse the rebuild interface for that and just return an empty dag too.
| .copy_empty_like_with_same_capacity(VarsMode::Alike, BlocksMode::Keep) | ||
| .unwrap(); |
There was a problem hiding this comment.
Does copy_empty_like actually need to be fallible? It looks like it's calling a lot of add_wire & similar methods which are fallible in general, but shouldn't actually be when copying a valid DAG.
There was a problem hiding this comment.
There shouldn't be a reason it needs to fail, or at least not that I can think of. I think it is mostly a legacy of when there were more python things in the data model and we were potentially calling out to Python. But I would save any changes to that for a different PR.
There was a problem hiding this comment.
Yeah definitely not for this PR but it would be nice to resolve that so we don't have unwraps flying around that look dangerous but aren't 😄
There was a problem hiding this comment.
Even so, please don't use unwrap: at least use expect with the reason for why it can't panic, so if we're ever wrong we've got the debug information of the original assumption.
There was a problem hiding this comment.
The copy_empty_like methods currently don't document how they might fail, so any assumption we write here isn't supported by our internal documentation - it'd be better pathing to either make this method fallible while copy_empty_like is fallible and remove it later, or fix copy_empty_like first so we can use it in this PR.
I would like us to get out of the habit of using unwrap/expect on long-range assumptions when the true fix is to fix the base interface, even for things that are expected to be ephemeral.
There was a problem hiding this comment.
I dug into this and made the copy methods infallible as they actually are in: #16316 . A DAGCircuit can't have duplicate wires and the error condition being propagated wasn't possible from the copy methods.
Coverage Report for CI Build 26657665730Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.01%) to 87.553%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions6 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
I'm wondering if there's a way to make developers be more aware of the choice of how to iterate over the dag. Is there a reason to expose the builder instead of just having something like a |
It's a fairly nuanced decision. I think what we have with topological_op_nodes using the lexicographical topological sort makes sense and exposing it as an option is more likely to go wrong than not. I'd rather just wrap the usage of |
5 participants
Summary
This commit switches the topological sort function we use in transpiler passes when reconstructing a dag from scratch. In several passes where we typically replace or remove a large numbers of gates we iterate over the input dag in topological order and construct a copy of it making the alterations as we go. Right now when we do this we rely on
DAGCircuit::topological_op_nodeswhich makes sense because it's or built-in method for iterating over a dag's op node in topological ordering. Internally this uses rustworkx's lexicographical topological sort function with our custom sort function that maintains our desired tie breaker using the bits of a node. However, since #14762 where we're asserting structural equality in passes we don't need to use that sort anymore for these reconstruction cases. We just need a consistent topological sort. In optimizing #13419 one thing that showed in profiles was that for very large circuits the overhead of the lexicographical topological sort for the iteration. The toposort function in petgraph is lower overhead because it doesn't need to work about the lexicographical tie breaker. By switching to use this instead we can reduce the overhead of the final sort in all these passes. In asv "utility scale" transpile benchmarking this commit speeds up runtime 2-5% (although without asv flagging it as significant).Details and comments