[WIP] Add method to add instructions to a DAGCircuit from an iterator of PackedInstruction
#13010
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 migrates the entirety of the `DAGCircuit` class to Rust. It fully replaces the Python version of the class. The primary advantage of this migration is moving from a Python space rustworkx directed graph representation to a Rust space petgraph (the upstream library for rustworkx) directed graph. Moving the graph data structure to rust enables us to directly interact with the DAG directly from transpiler passes in Rust in the future. This will enable a significant speed-up in those transpiler passes. Additionally, this should also improve the memory footprint as the DAGCircuit no longer stores `DAGNode` instances, and instead stores a lighter enum NodeType, which simply contains a `PackedInstruction` or the wire objects directly. Internally, the new Rust-based `DAGCircuit` uses a `petgraph::StableGraph` with node weights of type `NodeType` and edge weights of type `Wire`. The NodeType enum contains variants for `QubitIn`, `QubitOut`, `ClbitIn`, `ClbitOut`, and `Operation`, which should save us from all of the `isinstance` checking previously needed when working with `DAGNode` Python instances. The `Wire` enum contains variants `Qubit`, `Clbit`, and `Var`. As the full Qiskit data model is not rust-native at this point while all the class code in the `DAGCircuit` exists in Rust now, there are still sections that rely on Python or actively run Python code via Rust to function. These typically involve anything that uses `condition`, control flow, classical vars, calibrations, bit/register manipulation, etc. In the future as we either migrate this functionality to Rust or deprecate and remove it this can be updated in place to avoid the use of Python. API access from Python-space remains in terms of `DAGNode` instances to maintain API compatibility with the Python implementation. However, internally, we convert to and deal in terms of NodeType. When the user requests a particular node via lookup or iteration, we inflate an ephemeral `DAGNode` based on the internal `NodeType` and give them that. This is very similar to what was done in Qiskit#10827 when porting CircuitData to Rust. As part of this porting there are a few small differences to keep in mind with the new Rust implementation of DAGCircuit. The first is that the topological ordering is slightly different with the new DAGCircuit. Previously, the Python version of `DAGCircuit` using a lexicographical topological sort key which was basically `"0,1,0,2"` where the first `0,1` are qargs on qubit indices `0,1` for nodes and `0,2` are cargs on clbit indices `0,2`. However, the sort key has now changed to be `(&[Qubit(0), Qubit(1)], &[Clbit(0), Clbit(2)])` in rust in this case which for the most part should behave identically, but there are some edge cases that will appear where the sort order is different. It will always be a valid topological ordering as the lexicographical key is used as a tie breaker when generating a topological sort. But if you're relaying on the exact same sort order there will be differences after this PR. The second is that a lot of undocumented functionality in the DAGCircuit which previously worked because of Python's implicit support for interacting with data structures is no longer functional. For example, previously the `DAGCircuit.qubits` list could be set directly (as the circuit visualizers previously did), but this was never documented as supported (and would corrupt the DAGCircuit). Any functionality like this we'd have to explicit include in the Rust implementation and as they were not included in the documented public API this PR opted to remove the vast majority of this type of functionality. The last related thing might require future work to mitigate is that this PR breaks the linkage between `DAGNode` and the underlying `DAGCirucit` object. In the Python implementation the `DAGNode` objects were stored directly in the `DAGCircuit` and when an API method returned a `DAGNode` from the DAG it was a shared reference to the underlying object in the `DAGCircuit`. This meant if you mutated the `DAGNode` it would be reflected in the `DAGCircuit`. This was not always a sound usage of the API as the `DAGCircuit` was implicitly caching many attributes of the DAG and you should always be using the `DAGCircuit` API to mutate any nodes to prevent any corruption of the `DAGCircuit`. However, now as the underlying data store for nodes in the DAG are no longer the python space objects returned by `DAGCircuit` methods mutating a `DAGNode` will not make any change in the underlying `DAGCircuit`. This can come as quite the surprise at first, especially if you were relying on this side effect, even if it was unsound. It's also worth noting that 2 large pieces of functionality from rustworkx are included in this PR. These are the new files `rustworkx_core_vnext` and `dot_utils` which are rustworkx's VF2 implementation and its dot file generation. As there was not a rust interface exposed for this functionality from rustworkx-core there was no way to use these functions in rustworkx. Until these interfaces added to rustworkx-core in future releases we'll have to keep these local copies. The vf2 implementation is in progress in Qiskit/rustworkx#1235, but `dot_utils` might make sense to keep around longer term as it is slightly modified from the upstream rustworkx implementation to directly interface with `DAGCircuit` instead of a generic graph. Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com> Co-authored-by: Alexander Ivrii <alexi@il.ibm.com> Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com> Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Right now there is a bug in the matplotlib circuit visualizer likely caused by the new `__eq__` implementation for `DAGOpNode` that didn't exist before were some gates are missing from the visualization. In the interest of unblocking this PR this commit updates the references for these cases temporarily until this issue is fixed.
Previously the sort_key attribute of the Python space DAGCircuit was incorrectly being set to `None` for rust generated node objects. This was done as for the default path the sort key is determined from the rust domain's representation of qubits and there is no analogous data in the Python object. However, this was indavertandly a breaking API change as sort_key is expected to always be a string. This commit adds a default string to use for all node types so that we always have a reasonable value that matches the typing of the class. A future step is likely to add back the `dag` kwarg to the node types and generate the string on the fly from the rust space data.
The standard function signature convention for functions that take a `py: Python` argument is to make the Python argument the first (or second after `&self`). The `Param::eq` and `Param::is_close` methods were not following this convention and had `py` as a later argument in the signature. This commit corrects the oversight.
With the recent merge with main we pulled in Qiskit#12943 which conflicted with the rust space API changes made in this PR branch. This commit updates the usage to conform with the new interface introduced in this PR.
This commit adds several release notes to document this change. This includes a feature note to describe the high level change and the user facing benefit (mainly reduced memory consumption for DAGCircuits), two upgrade notes to document the differences with shared references caused by the new data structure, and a fix note documenting the fix for how qargs and cargs are handled on `.apply_operation_back()` and `.apply_operation_front()`. Along with the fix note a new unit test is added to serve as a regression test so that we don't accidentally allow adding cargs as qargs and vice versa in the future.
This commit restores the functionality of the `inplace` argument for `substitute_node()` and restores the tests validating the object identity when using the flag. This flag was originally excluded from the implementation because the Rust representation of the dag is not a shared reference with Python space and the flag doesn't really mean the same thing as there is always a second copy of the data for Python space now. The implementation here is cheating slighty as we're passed in the DAG node by reference it relies on that reference to update the input node at the same time we update the dag. Unlike the previous Python implementation where we were updating the node in place and the `inplace` argument was slightly faster because everything was done by reference. The rust space data is still a compressed copy of the data we return to Python so the `inplace` flag will be slightly more inefficient as we need to copy to update the Python space representation in addition to the rust version.
This commit removes an unecessary `dict()` cast on the `dag.metadata` when setting it on `QuantumCircuit.metadata` in `qiskit.converters.dag_to_circuit()`. This slipped in at some point during the development of this PR and it's not clear why, but it isn't needed so this removes it.
This commit adds a small inline code comment to make it clear why we skip parameter comparisons in DAGOpNode.__eq__ for python ops. It might not be clear why the value is hard coded to `true` in this case, as this check is done via Python so we don't need to duplicate it in rust space.
This commit adds error checking to the DAGNode constructor to raise a PyValueError if the input index is not valid (any index < -1). Previously this would have panicked instead of raising a user catchable error.
This commit updates the function names for `get__node_id` and `set__node_id` method to use a name that clippy is happy with and leverage the pyo3 macros to set the python space name correctly instead of using the implicit naming rules.
The Ord and PartialOrd traits were originally added to the Index struct so they could be used for the sort key in lexicographical topological sorting. However, that approach was abandonded during the development of this PR and instead the expanded Qubit and Clbit indices were used instead. This left the ordering traits as unnecessary on Index and potentially misleading. This commit just opts to remove them as they're not needed anymore.
Previously, the change in equality for DAGNodes was causing nodes to clobber eachother in the matplotlib drawer's tracking data structures when used as keys to maps. To fix this, we ensure that all nodes have a unique ID across layers before constructing the matplotlib drawer. They actually of course _do_ in the original DAG, but we don't really care what the original IDs are, so we just make them up. Writing to _node_id on a DAGNode may seem odd, but it exists in the old Python API (prior to being ported to Rust) and doesn't actually mutate the DAG at all since DAGNodes are ephemeral.
With the previous commit the bug in the matplotlib drawer causing the images to diverge should be fixed. This commit reverts the change to the reference images as there should be no difference now. This reverts commit 1e4e6f3.
The earlier commit that "fixed" the drawers corrected the visualization to match expectations in most cases. However after restoring the references to what's on main several comparison tests with control flow in the circuit were still failing. The failure mode looks similar to the other cases, but across control flow blocks instead of at the circuit level. This commit temporarily updates the references of these to the state of what is generated currently to unblock CI. If/when we have a fix this commit can be reverted.
This commit fixes a couple of edge cases in DAGOpNode.__eq__ method around the python interaction for the method. The first is that in the case where we had python object parameter types for the gates we weren't comparing them at all. This is fixed so we use python object equality for the params in this case. Then we were dropping the error handling in the case of using python for equality, this fixes it to return the error to users if the equality check fails. Finally a comment is added to explain the expected use case for `DAGOpNode.__eq__` and why parameter checking is more strict than elsewhere.
This commit removes the Param::add() method and instead adds a local private function to the `dag_circuit` module for doing global phase addition. Previously the `Param::add()` method was used solely for adding global phase in `DAGCircuit` and it took some shortcuts knowing that context. This made the method implementation ill suited as a general implementation.
…circuits" This reverts commit 9a6f953.
The fundamental issue with matplotlib visualizations of control flow is that locally in the control flow block the nodes look the same but are stored in an outer circuit dictionary. If the gates are the same and on the same qubits and happen to have the same node id inside the different control flow blocks the drawer would think it's already drawn the node and skip it incorrectly. The previous fix for this didn't go far enough because it wasn't accounting for the recursive execution of the drawer for inner blocks (it also didn't account for LayerSpoolers of the same length).
This commit makes some attributes of the dag circuit public as they will need to be accessible from the accelerate crate to realistically start using the DAGCircuit for rust transpiler passes.
This commit pivots away from using the PyO3 crate's conversion traits for specialized pickle serialization output of Wire objects. The output of the previous traits wasn't really intended for representing a Wire in Python but only for pickle serialization. This commit migrates these to custom methods, without a trait, to make it clear they're only for pickle.
This commit fixes and simplifies the separable_circuits() method. At it's core the method is building a subgraph of the original dag for each weakly connected component in the dag with a little bit of extra tracking to make sure the graph is a valid DAGCircuit. Instead of trying to do this manually this commit updates the method implementation to leverage the tools petgraph gives us for filtering graphs. This both fixes a bug identified in review but also simplifies the code.
raynelfss
added
performance
Rust
raynelfss
changed the title
DAGCircuit from an iterator of PackedInstructionDAGCircuit from an iterator of PackedInstruction
raynelfss
force-pushed
the
rusty-dag-from-iter
branch
from
0695db2
to
2600f5c
Compare
This commit migrates the qubit_io_map and clbit_io_map to go from a type of `IndexMap<Qubit, [NodeIndex; 2], RandomState>` to `Vec<[NodeIndex; 2]>`. Our qubit indices (represented by the `Qubit` type) must be a contiguous set for the circuit to be valid, and using an `IndexMap` for these mappings of bit to input and output nodes only really improved performance in the removal case, but at the cost of additional runtime overhead for accessing the data. Since removals are rare and also inefficient because it needs to reindex the entire dag already we should instead optimize for the accessing the data. Since we have contiguous indices using a Vec is a natural structure to represent this mapping.
At some point during the development of this PR the function signatures between `add_qubits()` and `add_clbits()` diverged between taking a `Vec<Bound<PyAny>>` and `&Bound<PySequence>`. In general they're are comprable but since we are going to be working with a `Vec<>` in the function body this is a better choice to let PyO3 worry about the conversion for us. Additionally, this is a more natural signature for rust consumption. This commit just updates `add_clbits()` to use a Vec too.
- Implement `DAGCircuit` with `with_capacity` to create an initially allocated instance, for rust only. - Implement `with_capacity` for `BitData` and `IndexInterner` that creates an initally allocated instance of each type. - Other small tweaks and fixes.
- If known, use `num_edges` argument to create a `DAGCircuit` with the exact number of edges. - Leverage usage of new method in `copy_empty_like`. - Use `..Default::default()` for `op_names` and `calibrations` fields in `DAGCircuit::with_capacity()` - Other small tweaks and fixes.
- Adapt to rebase
- Introduce a method that adds a chain of `PackedInstruction` continuously avoiding the re-linking of each bit's output-node until the very end of the iterator. - TODO: Add handling of vars - Add header for a `from_iter` function that will create a `DAGCircuit` based on a chain of `PackedInstruction`.
- Fix incorrect re-insertion of last_node.
- Make `from_iter` public.
- Add `::from_iter()` to generate a `DAGCircuit` instance based on an iterator of `PackedInstructions` and the interner instances from the source. - The generated `DAGCircuit` is ensured to have the exact capacity required to store the instructions provided by doing pre-parsing before-hand. After pre-parsing a new `DAGCircuit` is generated by calling `DAGCircuit::with_capacity()` to provide a pre-allocated instance.
- io-maps are Vecs now, this commit fixes their behavior.
raynelfss
force-pushed
the
rusty-dag-from-iter
branch
from
36ce038
to
432397e
Compare
raynelfss
changed the title
DAGCircuit from an iterator of PackedInstructionDAGCircuit from an iterator of PackedInstruction
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.
Solves #13003
Summary
Tracked by #13001 and preceeded by #12975
The following commits aim to add a
DAGCircuit::add_from_iter()to add a chain ofDAGOpNodesto the DAGCircuit based on a sequence ofPackedInstruction.Details and comments
These commits add the following two methods:
DAGCircuit::add_from_iter()to add a sequence ofPackedInstructioncontinously avoiding having to re-add each bit/var's output node each time an instruction is added. The addition of the output node is only performed once all instructions have been added correctly to the DAGCircuit.Known issues:
PyObjectwhich are not hashable.One possible solution would be to use a.PyDicthowever the structures that are being stored within this mapping are rust-native (NodeIndex,Wire)Another solution would be to somehow internalize these structures, which I'm still thinking on how to do.Blockers
DAGCircuitin Rust. #12975 (will rebase once merged)DAGCircuitto Rust #12550