Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Rust representation of EquivalenceLibrary #12585

Merged
merged 83 commits into from
Sep 25, 2024

Conversation

raynelfss
Copy link
Contributor

@raynelfss raynelfss commented Jun 14, 2024

Fixes #12278

Summary

As we continue our efforts to make more of the transpiler and circuit's functionality into rust. After #12459 added an infrastructure for Gates and Operations in Rust, it will be important for us to think of how to keep track of equivalent implementations of said gates that might not work in different Backends, which is why the EquivalenceLibrary exists.

The following commits add a rust representation of the EquivalenceLibrary structure.

Details and comments

Implementation:

These modifications consist of two parts:

  • A rust PyO3 class named: EquivalenceLibrary that implements most of the functionality of the python counterpart while keeping equivalent functionality in rust.
  • A python extension of the PyO3 class which is only used for the draw() method.

Modifications:

  • A couple of modifications were made to some of the defined structures under the qiskit._accelerate.circuit crate.

Possible issues:

  • There is currently no way of creating a PyDiGraph natively from rust. However, rustworkx-core allows the creation of a DiGraph that has the same functionality. If an instance of PyDiGraph is needed from Python, an alternative method called graph will assemble and store a PyDiGraph instance equivalent to the one present in the EquivalenceLibrary instance. This being only a representation should not modify the internal data of the graph stored in rust and will be discarded every time an equivalency is added or modified.
  • This implementation uses PyO3's subclass property to allow the rust PyClass to be extended from Python. However, this might lead to unexpected behavior if one were to extend the python class, as these need to be extended using the __new__ method instead of __init__. It is highly unlikely that this class would be extended further, but it is important to note this change in behavior.

Blocks

Other comments:

  • Feel free to leave any reviews or comments.

@raynelfss raynelfss requested a review from a team as a code owner June 14, 2024 22:44
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

@raynelfss raynelfss added priority: high Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library experimental Experimental feature without API stability guarantee labels Jun 14, 2024
@raynelfss raynelfss changed the title Add Rust representation of EquivalenceLibrary Add Rust representation of EquivalenceLibrary Jun 14, 2024
@raynelfss raynelfss added this to the 1.2.0 milestone Jun 14, 2024
@raynelfss raynelfss added enhancement and removed Changelog: None Do not include in changelog labels Jun 14, 2024
@Cryoris
Copy link
Contributor

Cryoris commented Jun 17, 2024

Nice! Did you already get to benchmark the basis translator with the oxidized equivalence library? 🙂

@raynelfss
Copy link
Contributor Author

Nice! Did you already get to benchmark the basis translator with the oxidized equivalence library? 🙂

Not yet @Cryoris, I am having some issues with the BasisTranslator, once I look at those will get to benchmarking it.

@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
@Qiskit Qiskit deleted a comment from coveralls Sep 24, 2024
- Using large int as the key's number of qubits breaks compatibility with qpy, use a random string instead.
Copy link
Contributor

@jlapeyre jlapeyre left a comment

Choose a reason for hiding this comment

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

Looks good.

@mtreinish mtreinish added performance Changelog: None Do not include in changelog and removed experimental Experimental feature without API stability guarantee labels Sep 24, 2024
@jlapeyre jlapeyre added this pull request to the merge queue Sep 25, 2024
Merged via the queue into Qiskit:main with commit 6a041ee Sep 25, 2024
15 checks passed
raynelfss added a commit to raynelfss/qiskit that referenced this pull request Sep 27, 2024
- Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after Qiskit#12585 merged.
github-merge-queue bot pushed a commit that referenced this pull request Sep 27, 2024
)

* Initial: Add `compose_transforms` and `get_example_gates` to Rust.

* Add: `compose_transform` logic in rust

* Fix: Correct the behavior of `compose_transforms`.
-  Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation.
- Remove panic statement when checking for example gates.
- Use qreg when adding an instruction to the mock_dag in `compose_transforms`.
- Add missing comparison check in for loop to compare the mapped instructions.
- Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract.
- Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`.

* Fix: Leverage rust-based `circuit_to_dag` converters.
- Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances.

* Format: Fix indentation of import lines in `compose_transforms.rs`

* Formatting: Separate complicated type aliases

* Fix: Adapt to new `DAGCircuit` limitations

* Format: Remove unused imports in BasisTranslator

* Fix: Adapt to #13143

* Fix: Code review comments
- Remove unused `circuit_to_dag` and `OperationRef` imports.
- Streamline complex types into simpler aliases.
- Rename `CircuitRep` to `CircuitFromPython`.
- Reshape `get_example_gates` to work with `CircuitData` instances during its recursion.
- Remove unnecessary clone of `gate_name` in `compose_transform`.
- Use `mapped_instructions` values when iterating in `compose_transforms`.
- Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust.
- Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple.

* Fix: More commments from code review
- Remove stale comment related to #3947
- Rename tuple instances to `GateIdentifier`.
- Rename `doomed_nodes` to `nodes_to_replace`.
- Add docstring for `get_example_gates`.
- Rename `get_example_gates` to `get_gate_num_params`.

Co-authored-by: Kevin Hartman <[email protected]>

* Refactor: Rename `example_gates` to `gate_param_counts`
- Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after #12585 merged.

---------

Co-authored-by: Kevin Hartman <[email protected]>
raynelfss added a commit to raynelfss/qiskit that referenced this pull request Sep 27, 2024
Fixes Port `BasisTranslator` to Rust Qiskit#12246

This is the final act of the efforts to move the `BasisTranslator` transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.)

Methodology:
The way this works is by keeping the original `BasisTranslator` python class, and have it store the rust-space counterpart (now called `CoreBasisTranslator`) which will perform all of the operations leveraging the existent Rust API's available for the `Target` (Qiskit#12292), `EquivalenceLibrary`(Qiskit#12585) and the `BasisTranslator` methods `basis_search` (Qiskit#12811) and `compose_transforms`(Qiskit#13137).

All of the inner methods will have private visibility and will not be accessible to `Python` as they're intended to be internal by design.

By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made.

Changes:

- Add the pyo3 class/struct `BasisTranslator` that will contain allof the main data used by the transpiler pass to perform its operation.
- Convert the `target_basis` into a set manually from python before sending it into the Rust space.
- Remove the exposure of `basis_search` and `compose_transforms` to python.
- Change `basis_search` so that it accepts references to `HashSet` instances instead of accepting a `HashSet<&str>` instance.
- Change inner method's visibility for `basis_search` and `compose_transform` modules in rust.
- Expose the exception imports from `Target` to the `accelerate` crate.
- Expose `DAGCircuit::copy_empty_like` to the rest of the crates.
- Remove all of the unused imports in the Python-side `BasisTranslator`.

Blockers:
- [ ] Qiskit#12811
emilkovacev pushed a commit to emilkovacev/qiskit that referenced this pull request Oct 1, 2024
…kit#13137)

* Initial: Add `compose_transforms` and `get_example_gates` to Rust.

* Add: `compose_transform` logic in rust

* Fix: Correct the behavior of `compose_transforms`.
-  Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation.
- Remove panic statement when checking for example gates.
- Use qreg when adding an instruction to the mock_dag in `compose_transforms`.
- Add missing comparison check in for loop to compare the mapped instructions.
- Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract.
- Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`.

* Fix: Leverage rust-based `circuit_to_dag` converters.
- Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances.

* Format: Fix indentation of import lines in `compose_transforms.rs`

* Formatting: Separate complicated type aliases

* Fix: Adapt to new `DAGCircuit` limitations

* Format: Remove unused imports in BasisTranslator

* Fix: Adapt to Qiskit#13143

* Fix: Code review comments
- Remove unused `circuit_to_dag` and `OperationRef` imports.
- Streamline complex types into simpler aliases.
- Rename `CircuitRep` to `CircuitFromPython`.
- Reshape `get_example_gates` to work with `CircuitData` instances during its recursion.
- Remove unnecessary clone of `gate_name` in `compose_transform`.
- Use `mapped_instructions` values when iterating in `compose_transforms`.
- Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust.
- Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple.

* Fix: More commments from code review
- Remove stale comment related to Qiskit#3947
- Rename tuple instances to `GateIdentifier`.
- Rename `doomed_nodes` to `nodes_to_replace`.
- Add docstring for `get_example_gates`.
- Rename `get_example_gates` to `get_gate_num_params`.

Co-authored-by: Kevin Hartman <[email protected]>

* Refactor: Rename `example_gates` to `gate_param_counts`
- Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after Qiskit#12585 merged.

---------

Co-authored-by: Kevin Hartman <[email protected]>
raynelfss added a commit to raynelfss/qiskit that referenced this pull request Oct 8, 2024
Fixes Port `BasisTranslator` to Rust Qiskit#12246

This is the final act of the efforts to move the `BasisTranslator` transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.)

Methodology:
The way this works is by keeping the original `BasisTranslator` python class, and have it store the rust-space counterpart (now called `CoreBasisTranslator`) which will perform all of the operations leveraging the existent Rust API's available for the `Target` (Qiskit#12292), `EquivalenceLibrary`(Qiskit#12585) and the `BasisTranslator` methods `basis_search` (Qiskit#12811) and `compose_transforms`(Qiskit#13137).

All of the inner methods will have private visibility and will not be accessible to `Python` as they're intended to be internal by design.

By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made.

Changes:

- Add the pyo3 class/struct `BasisTranslator` that will contain allof the main data used by the transpiler pass to perform its operation.
- Convert the `target_basis` into a set manually from python before sending it into the Rust space.
- Remove the exposure of `basis_search` and `compose_transforms` to python.
- Change `basis_search` so that it accepts references to `HashSet` instances instead of accepting a `HashSet<&str>` instance.
- Change inner method's visibility for `basis_search` and `compose_transform` modules in rust.
- Expose the exception imports from `Target` to the `accelerate` crate.
- Expose `DAGCircuit::copy_empty_like` to the rest of the crates.
- Remove all of the unused imports in the Python-side `BasisTranslator`.

Blockers:
- [ ] Qiskit#12811
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Oct 9, 2024
* Initial: Add equivalence to `qiskit._accelerate.circuit`

* Add: `build_basis_graph` method

* Add: `EquivalencyLibrary` to `qiskit._accelerate.circuit`
- Add `get_entry` method to obtain an entry from binding to a `QuantumCircuit`.
- Add `rebind_equiv` to bind parameters to `QuantumCircuit`

* Add: PyDiGraph converter for `equivalence.rs`

* Add: Extend original equivalence with rust representation

* Fix: Correct circuit parameter extraction

* Add: Stable infrastructure for EquivalenceLibrary
- TODO: Make elements pickleable.

* Add: Default methods to equivalence data structures.

* Fix: Adapt to new Gate Structure

* Fix: Erroneous display of `Parameters`

* Format: Fix lint test

* Fix: Use EdgeReferences instead of edge_indices.
- Remove stray comment.
- Use match instead of matches!.

* Fix: Use StableDiGraph for more stable indexing.
- Remove required py argument for get_entry.
- Reformat `to_pygraph` to use `add_nodes_from` and `add_edges_from`.
- Other small fixes.

* Fix: Use `clone` instead of `to_owned`
- Use `clone_ref` for the PyObject Graph instance.

* Fix: Use `OperationTypeConstruct` instead of `CircuitInstruction`
- Use `convert_py_to_operation_type` to correctly extract Gate instances into rust operational datatypes.
- Add function `get_sources_from_circuit_rep` to not extract circuit data directly but only the necessary data.
- Modify failing test due to different mapping. (!!)
- Other tweaks and fixes.

* Fix: Elide implicit lifetime of PyRef

* Fix: Make `CircuitRep` attributes OneCell-like.
- Attributes from CircuitRep are only written once, reducing the overhead.
- Modify `__setstate__` to avoid extra conversion.
- Remove `get_sources_from_circuit_rep`.

* Fix: Incorrect pickle attribute extraction

* Remove: Default initialization methods from custom datatypes.
- Use `__getnewargs__ instead.

* Remove: `__getstate__`, `__setstate__`, use `__getnewargs__` instead.

* Fix: Further improvements to pickling
- Use python structures to avoid extra conversions.
- Add rust native `EquivalenceLibrary.keys()` and have the python method use it.

* Fix: Use `PyList` and iterators when possible to skip extra conversion.
- Use a `py` token instead of `Python::with_gil()` for `rebind_params`.
- Other tweaks and fixes.

* Fix: incorrect list operation in `__getstate__`

* Fix: improvements on rust native methods
- Accept `Operations` and `[Param]` instead of the custom `GateOper` when calling from rust.
- Build custom `GateOper` inside of class.

* Remove: `add_equiv`, `set_entry` from rust-native methods.
- Add `node_index` Rust native method.
- Use python set comparison for `Param` check.

* Remove: Undo changes to Param
- Fix comparison methods for `Key`, `Equivalence`, `EdgeData` and `NodeData` to account for the removal of `PartialEq` for `Param`.

* Fix: Leverage usage of `CircuitData` for accessing the `QuantumCircuit` intructions in rust.
- Change implementation of `CircuitRef, to leverage the use of `CircuitData`.

* Add: `data()` method to avoid extracting `CircuitData`
- Add `py_clone` to perform shallow clones of a `CircuitRef` object by cloning the references to the `QuantumCircuit` object.
- Extract `num_qubits` and `num_clbits` for CircuitRep.
- Add wrapper over `add_equivalence` to be able to accept references and avoid unnecessary cloning of `GateRep` objects in `set_entry`.
- Remove stray mutability of `entry` in `set_entry`.

* Fix: Make `graph` attribute public.

* Fix: Make `NoteData` attributes public.

* Fix: Revert reference to `CircuitData`, extract instead.

* Add: Make `EquivalenceLibrary` graph weights optional.

* Fix: Adapt to Qiskit#12730

* Fix: Use `IndexSet` and `IndexMap`

* Fix: Revert changes from previously failing test

* Fix: Adapt to Qiskit#12974

* Fix: Use `EquivalenceLibrary.keys()` instead of `._key_to_node_index`

* Chore: update dependencies

* Refactor: Move `EquivalenceLibrary` to `_accelerate`.

* Fix: Erroneous `pymodule` function for `equivalence`.

* Fix: Update `EquivalenceLibrary` to store `CircuitData`.
- The equivalence library will now only store `CircuitData` instances as it does not need to call python directly to re-assign parameters.
- An `IntoPy<PyObject>` trait was adapted so that it can be automatically converted to a `QuantumCircuit` instance using `_from_circuit_data`.
- Updated all tests to use register-less qubits in circuit comparison.
- Remove `num_qubits` and `num_clbits` from `CircuitRep`.

* Fix: Make inner `CircuitData` instance public.

* Fix: Review comments and ownership issues.
- Add `from_operation` constructor for `Key`.
- Made `py_has_entry()` private, but kept its main method public.
- Made `set_entry` more rust friendly.
- Modify `add_equivalence` to accept a slice of `Param` and use `Into` to convert it into a `SmallVec` instance.

* Fix: Use maximum possible integer value for Key in basis_translator.
- Add method to immutably borrow the `EquivalenceLibrary`'s graph.

* Fix: Use generated string, instead of large int
- Using large int as the key's number of qubits breaks compatibility with qpy, use a random string instead.

---------

Co-authored-by: John Lapeyre <[email protected]>
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Oct 9, 2024
…kit#13137)

* Initial: Add `compose_transforms` and `get_example_gates` to Rust.

* Add: `compose_transform` logic in rust

* Fix: Correct the behavior of `compose_transforms`.
-  Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation.
- Remove panic statement when checking for example gates.
- Use qreg when adding an instruction to the mock_dag in `compose_transforms`.
- Add missing comparison check in for loop to compare the mapped instructions.
- Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract.
- Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`.

* Fix: Leverage rust-based `circuit_to_dag` converters.
- Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances.

* Format: Fix indentation of import lines in `compose_transforms.rs`

* Formatting: Separate complicated type aliases

* Fix: Adapt to new `DAGCircuit` limitations

* Format: Remove unused imports in BasisTranslator

* Fix: Adapt to Qiskit#13143

* Fix: Code review comments
- Remove unused `circuit_to_dag` and `OperationRef` imports.
- Streamline complex types into simpler aliases.
- Rename `CircuitRep` to `CircuitFromPython`.
- Reshape `get_example_gates` to work with `CircuitData` instances during its recursion.
- Remove unnecessary clone of `gate_name` in `compose_transform`.
- Use `mapped_instructions` values when iterating in `compose_transforms`.
- Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust.
- Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple.

* Fix: More commments from code review
- Remove stale comment related to Qiskit#3947
- Rename tuple instances to `GateIdentifier`.
- Rename `doomed_nodes` to `nodes_to_replace`.
- Add docstring for `get_example_gates`.
- Rename `get_example_gates` to `get_gate_num_params`.

Co-authored-by: Kevin Hartman <[email protected]>

* Refactor: Rename `example_gates` to `gate_param_counts`
- Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after Qiskit#12585 merged.

---------

Co-authored-by: Kevin Hartman <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Oct 31, 2024
…Rust. (#13237)

* Initial: Move the rest of the `BasisTranslator` to Rust.

Fixes Port `BasisTranslator` to Rust #12246

This is the final act of the efforts to move the `BasisTranslator` transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.)

Methodology:
The way this works is by keeping the original `BasisTranslator` python class, and have it store the rust-space counterpart (now called `CoreBasisTranslator`) which will perform all of the operations leveraging the existent Rust API's available for the `Target` (#12292), `EquivalenceLibrary`(#12585) and the `BasisTranslator` methods `basis_search` (#12811) and `compose_transforms`(#13137).

All of the inner methods will have private visibility and will not be accessible to `Python` as they're intended to be internal by design.

By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made.

Changes:

- Add the pyo3 class/struct `BasisTranslator` that will contain allof the main data used by the transpiler pass to perform its operation.
- Convert the `target_basis` into a set manually from python before sending it into the Rust space.
- Remove the exposure of `basis_search` and `compose_transforms` to python.
- Change `basis_search` so that it accepts references to `HashSet` instances instead of accepting a `HashSet<&str>` instance.
- Change inner method's visibility for `basis_search` and `compose_transform` modules in rust.
- Expose the exception imports from `Target` to the `accelerate` crate.
- Expose `DAGCircuit::copy_empty_like` to the rest of the crates.
- Remove all of the unused imports in the Python-side `BasisTranslator`.

Blockers:
- [ ] #12811

* Fix: Redundancies with serialization methods
- Remove extra copies of `target`, `target_basis`, `equiv_lib`, and `min_qubits`.
- Remove unnecessary mutability in `apply_transforms` and `replace_node`.

* Refactor: Remove `BasisTranslator` struct, use pymethod.
- Using this method avoids the creation of a datastructure in rust and the overhead of deserializing rust structures which can be overly slow due to multiple cloning. With this update, since the `BasisTranslator` never mutates, it is better to not store anything in Rust.

* Lint: Ignore too_many_args flag from clippy on `basis_translator::run()`

* Fix: Remove redundant clone

* Fix: Leverage using `unwrap_operation` when taking op_nodes from the dag.
- Add function signature in `base_run`.

* Update crates/accelerate/src/basis/basis_translator/mod.rs

Co-authored-by: Elena Peña Tapia <[email protected]>

* Adapt to #13164
- Use `QuantumCircuit._has_calibration_for()` when trying to obtain calibrations from a `QuantumCircuit` due to the deprecation of the `Pulse` package.

---------

Co-authored-by: Elena Peña Tapia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library performance priority: high Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port EquivalenceLibrary to Rust
6 participants