Oxidize QuantumCircuit._data and intern CircuitInstruction args
#10827
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
jakelishman
reviewed
Sep 13, 2023
There was a problem hiding this comment.
I'm really pretty impressed by quite how few changes were needed, even within QuantumCircuit itself! This in principle looks super exciting to me, and I'm looking forwards to having this in properly (though of course I know we've got a way to go yet).
I didn't review in full detail, more just a fairly quick scan through. A common comment that I didn't want to put everywhere was that I think we pretty much should never be taking Vec or returning it at the Python/Rust API boundary; it always necessitates a copy, when we could just act on an incoming PyList/PyTuple/PySequence natively; the cache locality of the objects is the same as Vec, except we don't need to copy onto the Rust heap for an object we mostly just throw away / move somewhere else.
Comment on lines
92
to
96
| |fn_idx_to_bit: &PyObject, args: &Vec<BitType>| -> PyResult<Vec<PyObject>> { | ||
| args.iter() | ||
| .map(|i| fn_idx_to_bit.call1(py, (*i,))) | ||
| .collect() | ||
| }; |
There was a problem hiding this comment.
This is the kind of thing where it would be much more convenient to have fn_idx_to_bit as a known PyList (or PyTuple) - we'd be able to directly index into the memory, rather than needing to indirect via a Python-space call.
We'd also probably be better off here collecting into a PyTuple allocated on the Python heap directly rather than a Rust-heap Vec that will need copying over.
There was a problem hiding this comment.
We'd also probably be better off here collecting into a PyTuple allocated on the Python heap directly rather than a Rust-heap Vec that will need copying over.
That sounds like a good idea to me, but I can't actually figure out a way to do that without first collecting the elements into a Vec. Collecting into a PyResult<&PyTuple> or PyResult<Py<PyTuple>> doesn't seem to work. The difficulty is that we're collecting a sequence of Results into a single Result containing a sequence.
If we were constructing a PyList, I could pre-create an empty list and add these items to it in a for-loop. But since PyTuple is immutable, I don't think that's an option here.
There was a problem hiding this comment.
It would certainly be possible with the raw C API, so this may just be a case where if it does turn out to be performance-important, we might want to drop to using the C-API FFI slightly more directly. I'm fairly sure it can be done in a perfectly safe way (albeit using unsafe blocks to call the FFI functions), so we might even want to implement the collection trait back up to PyO3 if so.
There was a problem hiding this comment.
Yeah, it seems like an obvious gap in PyO3 to not have the collection traits for these. Would certainly be a nice contribution upstream 😄.
InternedInstruction is renamed to PackedInstruction to make it clearer that the code deals in terms of some form of a packed instruction, and that interning of the instruction's qubits and clbits is just an implementation detail of that.
jakelishman
reviewed
Nov 16, 2023
Comment on lines
98
to
104
| /// As a convenience, :attr:`.CircuitData.qubits` and | ||
| /// :attr:`.CircuitData.clbits` are exposed to Python as ``list`` instances, | ||
| /// which are updated inplace as new bits are added via | ||
| /// :meth:`~.CircuitData.add_qubit` and :meth:`~.CircuitData.add_clbit`. | ||
| /// **DO NOT MODIFY THESE LISTS DIRECTLY**, | ||
| /// or they will become out of sync with the internals of this class. | ||
| /// In this way, a :class:`.CircuitData` owns its ``qubits`` and ``clbits``. |
There was a problem hiding this comment.
How much of a convenience / performance necessity is it, over us only offering copies of the list? I'm fine if it's necessary, but if it's not, it feels safer not to offer the footgun, and just make what are currently the qubits and clbits attributes methods instead.
There was a problem hiding this comment.
Turns out that it wasn't necessary for these lists to be updated in place.
I've changed the logic to instead replace the lists each time new bits are added in f2646b4. This way, client code won't be able to depend on the lists being updated.
Technically, this may be a breaking change since previously QuantumCircuit.{qubits, clbits} would be updated in place as new bits and registers were added to this circuit. Is this worth documenting in the release notes?
I left CircuitData.{qubits, clbits} as attributes rather than methods since the underlying values are cached as bits are added. I'm mildly concerned that changing them to methods might convey to clients that the lists are constructed on the fly and thus expensive to compute, which is not true. I could see an argument where they make more sense as methods to convey that the reference cannot be relied upon, but I've documented this in the docstrings.
jakelishman
approved these changes
Nov 17, 2023
There was a problem hiding this comment.
Thanks so much for the huge amount of work that went into this. This looks ready to merge to me. I know you've got a follow-up in the pipeline, and I think the best thing we can do right now is get this merged and start actually using the new code to get it exercised as widely as we can.
|
I'm not pressing "merge" yet because I'll give Matt another chance to look first, since there have been significant changes since his review. |
mtreinish
approved these changes
Nov 20, 2023
Comment on lines
+156
to
+159
| /// A private enumeration type used to extract arguments to pymethods | ||
| /// that may be either an index or a slice. | ||
| #[derive(FromPyObject)] | ||
| pub enum SliceOrInt<'a> { |
FabianBrings
pushed a commit
to FabianBrings/qiskit
that referenced
this pull request
Nov 27, 2023
…iskit#10827) * Initial commit. * Fix bugs with slicing impl. * Fix sort, remove dead code. * Use custom cfg flag for debug. * Run fmt. * Add todo. * Revert utils change, not needed anymore. * Use CircuitData in compose. * Revert test stub. * Remove once_cell. Not needed anymore. * Run format. * Fix lint issues. * Use PyTuple in ElementType. * Use native list and dict types for lookup tables. * Implement __traverse__ and __clear__. * Take iterable for extend. Preallocate. * Fix insertion indexing behavior. * Fix typo. * Avoid possible hash collisions in InternContext. * Use () instead of None for default iterable. * Resolve copy share intern context TODO. * Use () instead of empty list in stubed lists. * Use u32 for IndexType. * Resolve TODO in if_else.py. * Fix Rust lint issues. * Add unit testing for InternContext. * Remove print logging. * Fix bug introduced during print removal. * Add helper methods for getting the context in CircuitData. * Fix issue with BlueprintCircuit. * Workaround for CircuitInstruction operation mutability. Fix lint. * Revert "Workaround for CircuitInstruction operation mutability." This reverts commit 21f3e51. * Add _add_ref to InstructionSet. Allows in-place update of CircuitInstruction.operation within a CircuitData. * Exclude CircuitData::intern_context from GC clear. * Fix lint. * Avoid copy into list. * Override __deepcopy__. * Override __copy__ to avoid pulling CircuitData into a list. * Implement copy for CircuitData. * Port CircuitInstruction to Rust. * Use Rust CircuitInstruction. * Optimize circuit_to_instruction.py * Use freelist for CircuitInstruction class. * Remove use count, fix extend, InternContext internal. Previously CircuitData::extend would construct CircuitInstruction instances in memory for the entire iterable. Now, a GILPool is created for each iteration of the loop to ensure each instance is dropped before the next one is created. At most 3 CircuitInstruction instances are now created during the construction and transpilation of a QuantumVolume circuit in my testing. Use count tracking is now removed from InternContext. InternContext is now no longer exposed through the Python API. * Revert to using old extraction for now until we move bits inside CircuitData. * Fix issue with deletion. * Performance optimization overhaul. - Switched to native types for qubits, clbits, qubit_indices, clbit_indices. - Tweaks to ensure that only 1-3 CircuitInstruction instances are ever alive at the same time (e.g. in CircuitData.extend). - Use py_ext helpers to avoid unnecessary deallocations in PyO3. - Move pickling for CircuitData from QC to CircuitData itself. - Add CircuitData.reserve to preallocate space during copy scenarios. * Fix lint. * Attempt to move docstring for CircuitInstruction. * Add missing py_ext module file. * Use full struct for InternedInstruction. * Remove __copy__ from QuantumCircuit. * Improve bit key wrapper name. * Remove opt TODO comment. Will be done in new PR. * Clean up GC cycle breaking. * Add separate method convert_py_index_clamped. * Implement __eq__ instead of __richcmp__. * Avoid 'use'ing SliceOrInt enum. * Port slice conversion to pure Rust. * Clean up InternContext. * Change import order in quantumcircuitdata.py. * Use .zip method on iter(). * Rename get_or_cache to intern_instruction. * Improve error handling. * Add documentation comments for Rust types. * Move reserve method. * Add tests for bit key error. * Localize BlueprintCircuit workaround. * Slice refactoring, fixes, tests. * Fix setitem slice regression, clean up slice testing. * Use Python docstring form for pymethods. * Don't use underscore in BitAsKey type name. * Add release note. * Add upgrade note. * Add error messages for exceeded qubits and clbits. * Use BitType instead of u32. * Improve code comments for extend. * Improve readability with PackedInstruction. InternedInstruction is renamed to PackedInstruction to make it clearer that the code deals in terms of some form of a packed instruction, and that interning of the instruction's qubits and clbits is just an implementation detail of that. * Fix reserve issue. * Use usize for pointer type. * Use copied instead of cloned on Option. * Use .is() instead of IDs. * Convert tuples to list in legacy format. * Remove redundant parens. * Add crate comment for py_ext. * Make CircuitData::qubits and CircuitData::clbits ephemeral.
9 tasks
mtreinish
added a commit
to kevinhartman/qiskit
that referenced
this pull request
Aug 12, 2024
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>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 23, 2024
* Port DAGCircuit to Rust 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 #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> * Update visual mpl circuit drawer references 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. * Ensure DAGNode.sort_key is always a string 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. * Make Python argument first in Param::eq and Param::is_close 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. * Fix merge conflict with #12943 With the recent merge with main we pulled in #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. * Add release notes and test for invalid args on apply methods 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. * Restore `inplace` argument functionality for substitute_node() 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. * Revert needless dict() cast on metadata in dag_to_circuit() 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. * Add code comment for DAGOpNode.__eq__ parameter checking 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. * Raise a ValueError on DAGNode creation with invalid index 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. * Use macro argument to set python getter/setter name 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. * Remove Ord and PartialOrd derives from interner::Index 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. * Fix missing nodes in matplotlib drawer. 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. * Revert "Update visual mpl circuit drawer references" 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. * Update visual mpl circuit drawer references for control flow circuits 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. * Fix edge cases in DAGOpNode.__eq__ 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. * Remove Param::add() for global phase addition 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. * More complete fix for matplotlib drawer. * Revert "Update visual mpl circuit drawer references for control flow circuits" This reverts commit 9a6f953. * Unify rayon versions in workspace * Remove unused _GLOBAL_NID. * Use global monotonic ID counter for ids in drawer 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). * Re-add missing documentation * Remove unused BitData iterator stuff. * Make types, dag, and bit count methods public 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. * Make Wire pickle serialization explicit 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. * Make py token usage explicit in _VarIndexMap The _VarIndexMap type was designed to look like an IndexMap but is actually an inner python dictionary. This is because `Var` types are still defined in python and we need to use a dictionary if we have `Var` objects as keys in the mapping. In the interest of looking like an IndexMap all the methods (except for 1) used `with_gil` internally to work with the dictionary. This could add unecessary overhead and to make it explicit that there is python involvement with this struct's methods this commit adds a py: Python argument to all the methods and removes the `with_gil` usage. * Make all pub(crate) visibility pub * Remove unused method * Reorganize code structure around PyVariableMapper and BitLocations * Add missing var wires to .get_wires() method In the porting of the get_wires() method to Rust the handling of Var wires was missed in the output of the method. This commit corrects the oversight and adds them to the output. * Raise TypeError not ValueError for invalid input to set_global_phase * De-duplicate check logic for op node adding methods The methods for checking the input was valid on apply_operation_back, apply_operation_front, and _apply_op_node_back were all identical. This combines them into a single method to deduplicate the code. * Improve collect_1q_runs() filter function The filter function for collect_1q_runs() was needlessly building a matrix for all the standard gates when all we need to know in that case is whether the standard gate is parameterized or not. If it's not then we're guaranteed to have a matrix available. This commit updates the filter logic to account for this and improve it's throughput on standard gates. * Use swap_remove instead of shift_remove * Combine input and output maps into single mapping This commit combines the `DAGCircuit` `qubit_input_map` and `qubit_output_map` fields into a single `IndexMap` `qubit_io_map` (and the same for `clbit_input_map` and `clbit_output_map` going to `clbit_io_map`). That stores the input and output as 2 element array where the first element is the input node index and the second element is the output node index. This reduces the number of lookups we need to do in practice and also reduces the memory overhead of `DAGCircuit`. * Ensure we account for clbits in depth() short circuit check * Also account for Vars in DAGCircuit.width() The number of vars should be included in the return from the width() method. This was previously missing in the new implementation of this method. * Remove duplicated _get_node() method The `_get_node()` method was duplicated with the already public `node()` method. This commit removes the duplicate and updates it's only usage in the code base. * Handle Var wires in classical_predecessors This method was missing the handling for var wires, this commit corrects the oversight. * Remove stray comment * Use Operation::control_flow() instead of isinstance checking * Use &str for increment_op and decrement_op This commit reworks the interface for the increment_op and decrement_op methods to work by reference instead of passing owned String objects to the methods. Using owned String objects was resulting in unecessary allocations and extra overhead that could be avoided. There are still a few places we needed to copy strings to ensure we're not mutating things while we have references to nodes in the dag, typically only in the decrement/removal case. But this commit reduces the number of String copies we need to make in the DAGCircuit. * Also include vars in depth short circuit * Fix typing for controlflow name lookup in count_ops * Fix .properties() method to include operations field The .properties() method should have included the output of .count_ops() in its dictionary return but this was commented out temporarily while other pieces of this PR were fixed. This commit circles back to it and adds the missing field from the output. As an aside we should probably deprecate the .properties() method for removal in 2.0 it doesn't seem to be the most useful method in practice. * Add missing Var wire handling to py_nodes_on_wire * Add back optimization to avoid isinstance in op_nodes This commit adds back an optimization to the op_nodes dag method to avoid doing a python space op comparison when we're filtering on non-standard gates and evaluating a standard gate. In these cases we know that the filter will not match purely from rust without needing a python space op object creation or an isinstance call so we can avoid the overhead of doing that. * Simplify/deduplicate __eq__ method This commit reworks the logic in the DAGCircuit.__eq__ method implementation to simplify the code a bit and make it less verbose and duplicated. * Invalidate cached py op when needed in substitute_node_with_dag This commit fixes a potential issue in substitute_node_with_dag() when the propagate_condition flag was set we were not invalidating cached py ops when adding a new condition based on a propagated condition. This could potentially cause the incorrect object to be returned to Python after calling this method. This fixes the issues by clearing the cached node so that when returning the op to python we are regenerating the python object. * Copy-editing suggestions for release notes Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> * Fix and simplify separable_circuits() 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. * Add clbit removal test * Move to using a Vec<[NodeIndex; 2]> for io maps 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. * Make add_clbits() signature the same as add_qubits() 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. * Add attribution comment to num_tensor_factors() method * Add py argument to add_declared_var() * Remove unnecessarily Python-space check * Correct typo in `to_pickle` method --------- 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>
raynelfss
added a commit
to raynelfss/qiskit
that referenced
this pull request
Aug 24, 2024
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>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 4, 2024
* init * up * lint * . * up * before cache * with cache * correct * cleaned up * lint reno * Update Cargo.lock * . * up * . * revert op * . * . * . * . * Delete Cargo.lock * . * corrected string comparison * removed Operator class from operation.rs * . * Apply suggestions from code review Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> * comments from code review * Port DAGCircuit to Rust 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 #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> * Update visual mpl circuit drawer references 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. * Ensure DAGNode.sort_key is always a string 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. * Make Python argument first in Param::eq and Param::is_close 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. * Fix merge conflict with #12943 With the recent merge with main we pulled in #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. * Add release notes and test for invalid args on apply methods 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. * Restore `inplace` argument functionality for substitute_node() 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. * Revert needless dict() cast on metadata in dag_to_circuit() 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. * Add code comment for DAGOpNode.__eq__ parameter checking 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. * Raise a ValueError on DAGNode creation with invalid index 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. * Use macro argument to set python getter/setter name 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. * Remove Ord and PartialOrd derives from interner::Index 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. * Fix missing nodes in matplotlib drawer. 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. * Revert "Update visual mpl circuit drawer references" 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. * Update visual mpl circuit drawer references for control flow circuits 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. * Apply suggestions from code review Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> * code review * Fix edge cases in DAGOpNode.__eq__ 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. * Remove Param::add() for global phase addition 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. * More complete fix for matplotlib drawer. * Revert "Update visual mpl circuit drawer references for control flow circuits" This reverts commit 9a6f953. * Unify rayon versions in workspace * Remove unused _GLOBAL_NID. * Use global monotonic ID counter for ids in drawer 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). * Remove unused BitData iterator stuff. * Fully port Optimize1qGatesDecomposition to Rust This commit builds off of #12550 and the other data model in Rust infrastructure and migrates the Optimize1qGatesDecomposition pass to operate fully in Rust. The full path of the transpiler pass now never leaves rust until it has finished modifying the DAGCircuit. There is still some python interaction necessary to handle parts of the data model that are still in Python, mainly calibrations and parameter expressions (for global phase). But otherwise the entirety of the pass operates in rust now. This is just a first pass at the migration here, it moves the pass to be a single for loop in rust. The next steps here are to look at operating the pass in parallel. There is no data dependency between the optimizations being done by the pass so we should be able to the throughput of the pass by leveraging multithreading to handle each run in parallel. This commit does not attempt this though, because of the Python dependency and also the data structures around gates and the dag aren't really setup for multithreading yet and there likely will need to be some work to support that (this pass is a good candidate to work through the bugs on that). Part of #12208 * remove with_gil in favor of passing python tokens as params * Apply suggestions from code review Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> * fmt * python serialization * deprecation * Update commutation_checker.py * heh * init * let Pytuple collect * lint * First set of comments - use Qubit/Clbit - more info on unsafe - update reno - use LazySet less - use OperationRef, avoid CircuitInstruction creation * Second part - clippy - no BigInt - more comments * Matrix speed & fix string sort -- could not use op.name() directly since sorted differently than Python, hence it's back to BigInt * have the Python implementation use Rust * lint & tools * remove unsafe blocks * One more try to avoid segfaulty windows -- if that doesn't work maybe revert the change the the Py CommChecker uses Rust * Original version Co-authored-by: Sebastian Brandhofer <148463728+sbrandhsn@users.noreply.github.com> * Sync with updated CommutationChecker todo: shouldn't make the qubits interner public * Debug: disable cache trying to figure out why the windows CI fails (after being unable to locally reproduce we're using CI with a reduced set of tests) * ... second try * Update crates/accelerate/src/commutation_checker.rs Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> * Restore azure config * Remove unused import * Revert "Debug: disable cache" This reverts commit c564b80. * Don't overallocate cache We were allocating a the cache hashmap with a capacity for max cache size entries every time we instantiated a new CommutationChecker. The max cache size is 1 million. This meant we were allocating 162MB everytime CommutationChecker.__new__ was called, which includes each time we instantiate it manually (which happens once on import), the CommutationAnalysis pass gets instantiated (twice per preset pass manager created with level 2 or 3), or a commutation checker instance is pickle deserialized. This ends up causing a fairly large memory regression and is the source of the CI failures on windows. Co-authored-by: Jake Lishman <jake.lishman@ibm.com> * Cleanup parameter key type to handle edge conditions better This commit cleans up the ParameterKey type and usage to make it handle edge conditions better. The first is that the type just doesn't do the right thing for NaN, -0, or the infinities. Canonicalization is added for hash on -0 and the only constructor of the newtype adds a runtime guard against NaN and inifinity (positive or negative) to avoid that issue. The approach only makes sense as the cache is really there to guard us against unnecessary re-computing when we reiterate over the circuit > 1 time and nothing has changed for gates. Otherwise comparing floats like done in this PR does would not be a sound or an effective approach. * Remove unnecessary cache hit rate tracking * Undo test assertion changes * Undo unrelated test changes * Undo pending deprecation and unify commutation classes This commit removes the pending deprecation decorator from the python class definition as the Python class just internally is using the rust implementation now. This also removes directly using the rust implementation for the standard commutation library global as using the python class is exactly the same now. We can revisit if there is anything we want to deprecate and remove in 2.0 in a follow up PR. Personally, I think the cache management methods are all we really want to remove as the cache should be an internal implementation detail and not part of the public interface. * Undo gha config changes * Make serialization explicit This commit makes the pickling of cache entries explicit. Previously it was relying on conversion traits which hid some of the complexity but this uses a pair of conversion functions instead. * Remove stray SAFETY comment * Remove ddt usage from the tests Now that the python commutation checker and the rust commutation checker are the same thing the ddt parameterization of the commutation checker tests was unecessary duplication. This commit removes the ddt usage to restore having a single run of all the tests. * Update release note * Fix CommutationChecker class import * Remove invalid test assertion for no longer public attribute * Ray's review comments Co-authored-by: Raynel Sanchez <raynelfss@hotmail.com> * Handle ``atol/rtol``, more error propagation * update to latest changes in commchecker * fix merge conflict remnants * re-use expensive quantities such as the relative placement and the parameter hash * add missing header * gentler error handling * review comments & more docs * Use vec over IndexSet + clippy - vec<vec> is slightly faster than vec<indexset> - add custom types to satisfies clippy's complex type complaint - don't handle Clbit/Var * Simplify python class construction Since this PR was first written the split between the python side and rust side of the CommutationChecker class has changed so that there are no longer separate classes anymore. The implementations are unified and the python space class just wraps an inner rust object. However, the construction of the CommutationAnalysis pass was still written assuming there was the possibility to get either a rust or Python object. This commit fixes this and the type change on the `comm_checker` attribute by removing the unnecessary logic. --------- Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: Matthew Treinish <mtreinish@kortar.org> 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> Co-authored-by: Julien Gacon <jules.gacon@googlemail.com> Co-authored-by: Raynel Sanchez <raynelfss@hotmail.com>
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
Introduces a new list-like Rust data type
CircuitData, used to more efficiently store aQuantumCircuit's instruction listing in memory. This is used along with another new Rust typeInternContext, which serves as a cache for argument lists that use the same sequence of numeric bit identifiers.The basic idea is that when a
CircuitInstructioninstance is appended toQuantumCircuit, the underlyingCircuitDatastores just a reference to itsoperationon the Python heap and twou32IDs (handles into theInternContext) which correspond to itsqubitsandclbitsargument lists. WhenCircuitDataelements are accessed (e.g. viaQuantumCircuit._data), they get converted back toCircuitInstructioninstances.Details and comments
These changes leave the
QuantumCircuitDatawrapper entirely in place, with a few internal modifications to side-step implementing things like__rmul__for the Rust-sideCircuitData.The
InternContextdeals in terms of bit indices (not Python references), so it's naturally able to share argument lists across circuits, and it doesn't distinguish between qubit and clbit arg lists. Its only limitation is really the number of unique argument sequences it can store, which right now is2^32since we're using a slotted approach withu32IDs.At present, new
QuantumCircuitinstance manage their ownInternContext,and copies of a circuit share the context of the original(EDIT: for this PR, no sharing is done since concurrent implications need to be addressed.) In theory, we could use another mechanism to share the sameInternContextacross (all | some) new circuits (e.g. using a Python singleton), but that's probably only worth exploring if we discover that memory pressure across circuits is a big bottleneck for users down the line.Intern context implementation
When an argument list is interned (via
InternContext::intern(arg_list: Vec<BitType>)-> IndexType), the provided argument list is looked up in a mapping of arg list to slot ID, au32. If the mapping already exists, the returned slot ID identifies a slot which already contains the argument list corresponding to arg list.The number of uses (also a field of the slot) is incremented.If the mapping does not exist, the next available slot is claimed, the arg list is moved into it,and the new slot's use count is set to 1(EDIT: use count tracking has been cut from this PR).An interned argument list is retrieved via its slot ID (
InternContext::lookup(slot: IndexType) -> &Vec<BitType>).When a client ofInternContextknows that it no longer needs a particular use of a slot, it can callInternContext::drop_use(slot: IndexType)to decrement the use count, unallocating the slot if it is no longer in use.The (likely) only client of(EDIT: Use count tracking cut from this PR)InternContextisCircuitData, which has been written with care to drop intern uses asCircuitInstructions are removed from it, and on its own destruction.Memory profiling
I profiled circuit construction with
memrayof a circuit with about 5 million parameter-less gates and found that peak memory use was reduced by about400 MiB(from1.7 GiBto1.3 GiB). With #10314 applied, the same circuit comes down to just192 MiBpeak memory use (most of our memory pressure comes from operation instances, with argument lists being the second worst offender).Todo
QuantumCircuit._datainto Python lists in various places.InternContext.Handle case of intern context full.=> likely not necessary with 2^32 slots.