Fix potential non-determinism in DAGOpNode sort key #9569
Conversation
This commit fixes potential source of non-determinism when topologically sorting a DAGCircuit object. The lexicographical_topological_sort() function we're using from rustworkx takes a sort key callback that returns a string which is used for tie breaking in the topological sort. The string used for op nodes was previously `str(qargs)`, but this is potentially problematic for standalone Qubit objects. The repr for a qubit without a register set will include the memory address of the bit object. This could result in the topological sort differing between executions of the same program which could lead to unexpected results. This commit fixes this by changing the sort key to be a string of the qubit indices in the dag instead of the qubit objects in qargs. This commit will likely introduce a small performance regression because we're adding overhead on every DAGOpNode creation. This regression can be addressed when Qiskit#9389 is implemented because we can just pass the data structure mapping bit objects to indices to the constructor instead of having to build that mapping on each op node. Additionally, some test cases were fixed as part of this commit, because several DAGCircuit test cases were incorrectly calling apply_operation_back() and setting clbits as qargs which will now fail because of the index lookup on qubits.
mtreinish
added
Changelog: None
|
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:
|
There was a problem hiding this comment.
Deeply unfortunately, SabreSwap and LookaheadSwap both manually construct some DAGOpNodes - I think we need to thread the sort-key generation through them as well (and stop them from doing this, at some point). Looking at the SabreSwap code now, it also just for fun mutates the qargs after the node is created, so its produced sort key won't even be correct 👍.
Pull Request Test Coverage Report for Build 5752236784
💛 - Coveralls |
Looking through the code, |
|
Ah, you're right - it'll just continue to work. But given there's a |
There was a problem hiding this comment.
I'm kind of concerned that the performance regression here will be too large, until we've got #9389 in place. Maybe it would be best to wait? The potential bug we identified for this is probably super arcane right now, because most topological iteration through DAGs happens in the transpiler, and layout/routing passes canonicalise to a single register, so no loose bits.
There was a problem hiding this comment.
Might be time to revisit this, now that #10128 is merged. Minor comment: when we're generating the sort key, we might want to zero-pad out the elements of the key so the lexicographical sort works like a numeric sort? It won't affect correctness, but might make iteration slightly less confusing.
I also suspect that this might have some performance improvements for topological sorting; the key comparisons will be comparing much shorted strings.
|
I updated the PR to use |
This commit updates the sort key construction to do two things, first it updates the integer indices used in the sort key to be 0 padded so that the sort order is more intuitive. At the same time the sort key is not including the cargs for an operation to ensure that's being factored into the sort order. Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
There was a problem hiding this comment.
Looks fine to me now. Preliminary benchmarking from Matt's machine suggests that there's no significant performance regressions or improvements here. There's potentially an issue with topological iteration between two circuits constructed in slightly different orders that contain operations on (e.g.) q0, q1 and separately c0, c1, but we think that the impact will be incredibly low, if it exists at all, and we're likely to change how cargs are handled anyway.
In a typical circuit there are only certain numbers of qargs that are used regularly. The best examples are 1q gates on each qubit in the target, or 2q gates on each edge in the connectivity graph. As the qargs will be repeated many times in a typical circuit then for most operations the sort key attribute is also not-unique (as it's just composed of the positional indices of each qarg and carg). In very large circuit compilation we spend a non-insignificant amount of time constructing the sort key strings on nodes during the lifecycle of the DAGCircuit as it gets transformed. This commit caches the sort keys so that they're not rebuilt for every new DAGNode. This does add some extra overhead to the construction the first time a DAGNode is created because it has to do an additional qarg tuple construction and a dict lookup. But this is worth it because in most circutis the cache hit rate will be very high and we'll avoid the more expensive string construction for most gates in a circuit. There is also a potential shared state issue here if any qubit or clbit objects are present in >1 DAGCircuit and the relative positions of those bits differ between the circuits the sort key will not be the same as before because the earlier qubit combinations. This is because the cache is stored as a class attribute on the DAGOpNode class and will be reused between circuits. In practice this is unlikely to come up very frequently. But also it's impact is mitigated by these keys just being used for sorting and the sorting hasn't always been 100% deterministic anyway as long as it's consistent for the life of the DAGCircuit being exactly the same between multiple DAGCircuit objects has never been a guarantee (although we tried to improve it in Qiskit#9569).
* Cache DAGOpNode sort keys In a typical circuit there are only certain numbers of qargs that are used regularly. The best examples are 1q gates on each qubit in the target, or 2q gates on each edge in the connectivity graph. As the qargs will be repeated many times in a typical circuit then for most operations the sort key attribute is also not-unique (as it's just composed of the positional indices of each qarg and carg). In very large circuit compilation we spend a non-insignificant amount of time constructing the sort key strings on nodes during the lifecycle of the DAGCircuit as it gets transformed. This commit caches the sort keys so that they're not rebuilt for every new DAGNode. This does add some extra overhead to the construction the first time a DAGNode is created because it has to do an additional qarg tuple construction and a dict lookup. But this is worth it because in most circutis the cache hit rate will be very high and we'll avoid the more expensive string construction for most gates in a circuit. There is also a potential shared state issue here if any qubit or clbit objects are present in >1 DAGCircuit and the relative positions of those bits differ between the circuits the sort key will not be the same as before because the earlier qubit combinations. This is because the cache is stored as a class attribute on the DAGOpNode class and will be reused between circuits. In practice this is unlikely to come up very frequently. But also it's impact is mitigated by these keys just being used for sorting and the sorting hasn't always been 100% deterministic anyway as long as it's consistent for the life of the DAGCircuit being exactly the same between multiple DAGCircuit objects has never been a guarantee (although we tried to improve it in #9569). * Move sort key cache to be DAG instance scoped This commit reworks the storage location for the cached dag sort keys. In the previous commit the sort keys were stored as a class attribute on the DAGOpNode. As was mentioned in that commit message this had potential shared state issues. To address that potential issue this commit moves the cache to be scoped to an instance of DAG as that is really what the sort key is scoped to. At the same time the lookup key for the cache is changed to be (qargs, cargs) instead of the flattened tuple. * Use a more accurate not overloaded variable name for the cache lookup key
* Fix potential non-determinism in DAGOpNode sort key This commit fixes potential source of non-determinism when topologically sorting a DAGCircuit object. The lexicographical_topological_sort() function we're using from rustworkx takes a sort key callback that returns a string which is used for tie breaking in the topological sort. The string used for op nodes was previously `str(qargs)`, but this is potentially problematic for standalone Qubit objects. The repr for a qubit without a register set will include the memory address of the bit object. This could result in the topological sort differing between executions of the same program which could lead to unexpected results. This commit fixes this by changing the sort key to be a string of the qubit indices in the dag instead of the qubit objects in qargs. This commit will likely introduce a small performance regression because we're adding overhead on every DAGOpNode creation. This regression can be addressed when Qiskit#9389 is implemented because we can just pass the data structure mapping bit objects to indices to the constructor instead of having to build that mapping on each op node. Additionally, some test cases were fixed as part of this commit, because several DAGCircuit test cases were incorrectly calling apply_operation_back() and setting clbits as qargs which will now fail because of the index lookup on qubits. * Update sort_key creation to use DAGCircuit.find_bit * 0 pad integer indices and include cargs in sort key This commit updates the sort key construction to do two things, first it updates the integer indices used in the sort key to be 0 padded so that the sort order is more intuitive. At the same time the sort key is not including the cargs for an operation to ensure that's being factored into the sort order. Co-authored-by: Jake Lishman <jake.lishman@ibm.com> --------- Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
* Cache DAGOpNode sort keys In a typical circuit there are only certain numbers of qargs that are used regularly. The best examples are 1q gates on each qubit in the target, or 2q gates on each edge in the connectivity graph. As the qargs will be repeated many times in a typical circuit then for most operations the sort key attribute is also not-unique (as it's just composed of the positional indices of each qarg and carg). In very large circuit compilation we spend a non-insignificant amount of time constructing the sort key strings on nodes during the lifecycle of the DAGCircuit as it gets transformed. This commit caches the sort keys so that they're not rebuilt for every new DAGNode. This does add some extra overhead to the construction the first time a DAGNode is created because it has to do an additional qarg tuple construction and a dict lookup. But this is worth it because in most circutis the cache hit rate will be very high and we'll avoid the more expensive string construction for most gates in a circuit. There is also a potential shared state issue here if any qubit or clbit objects are present in >1 DAGCircuit and the relative positions of those bits differ between the circuits the sort key will not be the same as before because the earlier qubit combinations. This is because the cache is stored as a class attribute on the DAGOpNode class and will be reused between circuits. In practice this is unlikely to come up very frequently. But also it's impact is mitigated by these keys just being used for sorting and the sorting hasn't always been 100% deterministic anyway as long as it's consistent for the life of the DAGCircuit being exactly the same between multiple DAGCircuit objects has never been a guarantee (although we tried to improve it in Qiskit#9569). * Move sort key cache to be DAG instance scoped This commit reworks the storage location for the cached dag sort keys. In the previous commit the sort keys were stored as a class attribute on the DAGOpNode. As was mentioned in that commit message this had potential shared state issues. To address that potential issue this commit moves the cache to be scoped to an instance of DAG as that is really what the sort key is scoped to. At the same time the lookup key for the cache is changed to be (qargs, cargs) instead of the flattened tuple. * Use a more accurate not overloaded variable name for the cache lookup key
Summary
This commit fixes potential source of non-determinism when topologically sorting a DAGCircuit object. The lexicographical_topological_sort() function we're using from rustworkx takes a sort key callback that returns a string which is used for tie breaking in the topological sort. The string used for op nodes was previously
str(qargs), but this is potentially problematic for standalone Qubit objects. The repr for a qubit without a register set will include the memory address of the bit object. This could result in the topological sort differing between executions of the same program which could lead to unexpected results. This commit fixes this by changing the sort key to be a string of the qubit indices in the dag instead of the qubit objects in qargs.This commit will likely introduce a small performance regression because we're adding overhead on every DAGOpNode creation. This regression can be addressed when #9389 is implemented because we can just pass the data structure mapping bit objects to indices to the constructor instead of having to build that mapping on each op node.
Additionally, some test cases were fixed as part of this commit, because several DAGCircuit test cases were incorrectly calling apply_operation_back() and setting clbits as qargs which will now fail because of the index lookup on qubits.
Details and comments