Skip to content

Revert "Port star_preroute to rust"#14381

Merged
jakelishman merged 3 commits into
Qiskit:mainfrom
mtreinish:revert-rust-star-pre-routing
May 16, 2025
Merged

Revert "Port star_preroute to rust"#14381
jakelishman merged 3 commits into
Qiskit:mainfrom
mtreinish:revert-rust-star-pre-routing

Conversation

@mtreinish

Copy link
Copy Markdown
Member

Summary

This commit reverts the move of star prerouting to run in Rust. At the time of this commit it was before the majority of the transpiler infrastructure was ported to rust. At that time the pass was ported we had a few one off passes that were ported to rust and we had to manage the Python rust boundary carefully. The pattern was typically to define custom rust data types that provided an alternate view of the DAGCircuit and the target for compilation that was built out of the Python space dag and target. The star prerouting pass was ported to Rust and leveraged the existing infrastructure used by SabreSwap and SabreLayout as the function of the pass on a DAG was very similar to Sabre (finding where to insert swaps and rebuilding the dag where swaps are needed). At the time this was a pragmatic choice to accelerate the pass and not need to wait for the needed infrastructure to be written in Rust. However in practice the performance gains the Rust porting of the algorithm provided were not as large as was originally hoped and now the code of the pass is tied directly to the current implementation of sabre. As the sabre interface layer is being re-written in #14317 this is proving to be a blocker for that effort. This commit reverts the current rust implementation to unblock the sabre refactoring effort.

It's still definitely worth porting this pass to Rust but we now have sufficient infrastructure in Rust to write full transpiler passes without Python. We should follow up this PR and rewrite the star prerouting pass so that full pass operates completely in rust and directly leverages the native data structures that exist in Rust now instead of the hybrid approach that ties the is tightly coupled to the internals of the sabre pass.

Details and comments

This reverts commit b23c545.

This commit reverts the move of star prerouting to run in Rust. At the
time of this commit it was before the majority of the transpiler
infrastructure was ported to rust. At that time the pass was ported we
had a few one off passes that were ported to rust and we had to manage
the Python rust boundary carefully. The pattern was typically to define
custom rust data types that provided an alternate view of the
DAGCircuit and the target for compilation that was built out of the
Python space dag and target. The star prerouting pass was ported to
Rust and leveraged the existing infrastructure used by SabreSwap and
SabreLayout as the function of the pass on a DAG was very similar to
Sabre (finding where to insert swaps and rebuilding the dag where
swaps are needed). At the time this was a pragmatic choice to
accelerate the pass and not need to wait for the needed infrastructure
to be written in Rust. However in practice the performance gains the
Rust porting of the algorithm provided were not as large as was
originally hoped and now the code of the pass is tied directly to
the current implementation of sabre. As the sabre interface layer is
being re-written in Qiskit#14317 this is proving to be a blocker for that
effort. This commit reverts the current rust implementation to unblock
the sabre refactoring effort.

It's still definitely worth porting this pass to Rust but we now have
sufficient infrastructure in Rust to write full transpiler passes
without Python. We should follow up this PR and rewrite the star
prerouting pass so that full pass operates completely in rust and
directly leverages the native data structures that exist in Rust now
instead of the hybrid approach that ties the is tightly coupled to the
internals of the sabre pass.

This reverts commit b23c545.
@mtreinish mtreinish requested a review from a team as a code owner May 15, 2025 21:23
@mtreinish mtreinish added Changelog: None Do not include in the GitHub Release changelog. mod: transpiler Issues and PRs related to Transpiler labels May 15, 2025
@qiskit-bot

Copy link
Copy Markdown
Collaborator

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

  • @Qiskit/terra-core

@coveralls

coveralls commented May 15, 2025

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 15066436418

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 49 of 50 (98.0%) changed or added relevant lines in 1 file are covered.
  • 104 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.002%) to 88.7%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/routing/star_prerouting.py 49 50 98.0%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 93.48%
crates/qasm2/src/parse.rs 6 97.61%
qiskit/circuit/quantumcircuit.py 94 93.7%
Totals Coverage Status
Change from base Build 15054997170: 0.002%
Covered Lines: 76267
Relevant Lines: 85983

💛 - Coveralls

@mtreinish mtreinish added this to the 2.1.0 milestone May 16, 2025

@jakelishman jakelishman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for taking care that the new test added by the patch still passes as well.

@jakelishman jakelishman added this pull request to the merge queue May 16, 2025
Merged via the queue into Qiskit:main with commit d4df68b May 16, 2025
24 checks passed
@mtreinish mtreinish deleted the revert-rust-star-pre-routing branch May 17, 2025 00:43
rahaman-quantum pushed a commit to rahaman-quantum/qiskit that referenced this pull request Jun 20, 2025
* Revert "Port star_preroute to rust"

This commit reverts the move of star prerouting to run in Rust. At the
time of this commit it was before the majority of the transpiler
infrastructure was ported to rust. At that time the pass was ported we
had a few one off passes that were ported to rust and we had to manage
the Python rust boundary carefully. The pattern was typically to define
custom rust data types that provided an alternate view of the
DAGCircuit and the target for compilation that was built out of the
Python space dag and target. The star prerouting pass was ported to
Rust and leveraged the existing infrastructure used by SabreSwap and
SabreLayout as the function of the pass on a DAG was very similar to
Sabre (finding where to insert swaps and rebuilding the dag where
swaps are needed). At the time this was a pragmatic choice to
accelerate the pass and not need to wait for the needed infrastructure
to be written in Rust. However in practice the performance gains the
Rust porting of the algorithm provided were not as large as was
originally hoped and now the code of the pass is tied directly to
the current implementation of sabre. As the sabre interface layer is
being re-written in Qiskit#14317 this is proving to be a blocker for that
effort. This commit reverts the current rust implementation to unblock
the sabre refactoring effort.

It's still definitely worth porting this pass to Rust but we now have
sufficient infrastructure in Rust to write full transpiler passes
without Python. We should follow up this PR and rewrite the star
prerouting pass so that full pass operates completely in rust and
directly leverages the native data structures that exist in Rust now
instead of the hybrid approach that ties the is tightly coupled to the
internals of the sabre pass.

This reverts commit b23c545.

* Add back reverted alt text for plot in docs
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 the GitHub Release changelog. mod: transpiler Issues and PRs related to Transpiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants