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

Remove faulty qubits and gates sections from transpile() #9900

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

mtreinish
Copy link
Member

Summary

This commit removes the "faulty qubits" support from the transpile(). Previously the transpile() function would attempt to filter out any faulty qubits or gates reported in a BackendV1's BackendProperties object and remap the qubits based on that. However, this code was never actually functioning as the first BackendV1 based backend to attempt to set these flags is causing an internal panic in rustworkx trying to created the remapped coupling map. This code path was mildly controversial at the time of introduction (and was already reverted once in the past) and since it provably isn't valid anymore (and I wasn't clear it ever functioned as expected) as the only backend to leverage the feature crashes transpile() this commit just opts to remove it as a potential breaking change.

Details and comments

This commit removes the "faulty qubits" support from the transpile().
Previously the transpile() function would attempt to filter out any
faulty qubits or gates reported in a BackendV1's BackendProperties
object and remap the qubits based on that. However, this code was never
actually functioning as the first BackendV1 based backend to attempt to
set these flags is causing an internal panic in rustworkx trying to
created the remapped coupling map. This code path was mildly controversial
at the time of introduction (and was already reverted once in the past)
and since it provably isn't valid anymore (and I wasn't clear it ever
functioned as expected) as the only backend to leverage the feature
crashes transpile() this commit just opts to remove it as a potential
breaking change.
@mtreinish mtreinish added the Changelog: Removal Include in the Removed section of the changelog label Apr 3, 2023
@mtreinish mtreinish added this to the 0.24.0 milestone Apr 3, 2023
@mtreinish mtreinish requested a review from a team as a code owner April 3, 2023 19:14
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Apr 3, 2023

Pull Request Test Coverage Report for Build 4618915359

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 85.418%

Files with Coverage Reduction New Missed Lines %
qiskit/compiler/transpiler.py 1 90.12%
qiskit/circuit/quantumcircuit.py 3 93.98%
Totals Coverage Status
Change from base Build 4617662891: -0.01%
Covered Lines: 67531
Relevant Lines: 79059

💛 - Coveralls

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

There's also a couple of minor references to faultiness in comments in DenseLayout and VF2's build_average_error_map that we might want to squash. For completeness for others: there's also test files test/python/providers/{faulty_backends.py,test_faulty_backend.py}, but those are testing the BackendV1 model not transpile so this PR needn't remove them.

Officially this PR isn't entirely within our deprecation policy because we haven't issued any warning before removing this implicit behaviour from the transpiler, but we judged the impact to be minimal as we're not aware of any current backends using this feature at all and in practice the handling doesn't actually work, so we don't think anybody can be using it. Offline, @1ucian0 (one of the original authors of the feature) also signed off on this removal.

edit: ha, I posted this comment then saw that Luciano had assigned himself to do a review as well.

@mtreinish
Copy link
Member Author

I think after #9911 and #9840 merge I think we should update the release note to point people to using BackendV2Converter with the filter flag as an alternative path.

@1ucian0
Copy link
Member

1ucian0 commented Apr 5, 2023

confirming. Let's remove it.

@1ucian0
Copy link
Member

1ucian0 commented Apr 12, 2023

do we want to keep the references in the following files?

  • qiskit/transpiler/passes/layout/vf2_utils.py
  • qiskit/transpiler/passes/layout/dense_layout.py
  • qiskit/providers/models/backendproperties.py
  • qiskit/providers/backend_compat.py

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Besides #9900 (comment) (that can be in a follow up, if necessary), LGTM

@mtreinish
Copy link
Member Author

The second two references need to stay: backendproperties is the model object that defines the payload format and the backend_compat file supports filtering a target to not include them as an alternative to this code. The dense layout reference should be removed in the patch series ending in #9927. The vf2_utils comment I think we can either leave in place or update in a followup. It's just explaining why there is an if statement guard for a condition which should be impossible.

@mtreinish mtreinish added this pull request to the merge queue Apr 12, 2023
@mtreinish
Copy link
Member Author

mtreinish commented Apr 12, 2023

I went ahead and enqueued this for merging because of those remaining references only 1 arguably needs to be updated, but it's just a code comment

Merged via the queue into Qiskit:main with commit b5056fe Apr 12, 2023
giacomoRanieri pushed a commit to giacomoRanieri/qiskit-terra that referenced this pull request Apr 16, 2023
* Remove faulty qubits and gates sections from transpile()

This commit removes the "faulty qubits" support from the transpile().
Previously the transpile() function would attempt to filter out any
faulty qubits or gates reported in a BackendV1's BackendProperties
object and remap the qubits based on that. However, this code was never
actually functioning as the first BackendV1 based backend to attempt to
set these flags is causing an internal panic in rustworkx trying to
created the remapped coupling map. This code path was mildly controversial
at the time of introduction (and was already reverted once in the past)
and since it provably isn't valid anymore (and I wasn't clear it ever
functioned as expected) as the only backend to leverage the feature
crashes transpile() this commit just opts to remove it as a potential
breaking change.

* Fix release note typos
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
* Remove faulty qubits and gates sections from transpile()

This commit removes the "faulty qubits" support from the transpile().
Previously the transpile() function would attempt to filter out any
faulty qubits or gates reported in a BackendV1's BackendProperties
object and remap the qubits based on that. However, this code was never
actually functioning as the first BackendV1 based backend to attempt to
set these flags is causing an internal panic in rustworkx trying to
created the remapped coupling map. This code path was mildly controversial
at the time of introduction (and was already reverted once in the past)
and since it provably isn't valid anymore (and I wasn't clear it ever
functioned as expected) as the only backend to leverage the feature
crashes transpile() this commit just opts to remove it as a potential
breaking change.

* Fix release note typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Removal Include in the Removed section of the changelog priority: high
Projects
None yet
5 participants