Skip to content

Conversation

mtreinish
Copy link
Member

Summary

This commit reverts an unecessary breaking API change that was introduced in #9175. In that PR the data type for two arguments, gate_lengths and gate_errors, that was potentially passed to plugins was changed. This change was well intentioned as it was used by the default plugin as part of the changes to the default plugin made in that PR. But this neglected the downstream effects for any existing plugin interface users relying on the input to those fields. Making this change to existing fields would break any plugins that were using the old data format and is a violation of the Qiskit stability and deprecation policy. This commit reverts that change so that gate_lengths and gate_errors contain the same data format as in previous releases. Instead to facilitate the new workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit, were added that leverage the new format introduced by #9175. By adding this as new optional fields existing users can continue to work as before, but the default plugin which requires the different data format can use the new data type.

Details and comments

This commit reverts an unecessary breaking API change that was
introduced in Qiskit#9175. In that PR the data type for two arguments,
gate_lengths and gate_errors, that was potentially passed to
plugins was changed. This change was well intentioned as it was
used by the default plugin as part of the changes to the default
plugin made in that PR. But this neglected the downstream effects
for any existing plugin interface users relying on the input to
those fields. Making this change to existing fields would break
any plugins that were using the old data format and is a violation
of the Qiskit stability and deprecation policy. This commit reverts
that change so that gate_lengths and gate_errors contain the same data
format as in previous releases. Instead to facilitate the new
workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit,
were added that leverage the new format introduced by Qiskit#9175. By adding
this as new optional fields existing users can continue to work as
before, but the default plugin which requires the different data
format can use the new data type.
@mtreinish mtreinish added priority: high Changelog: None Do not include in changelog labels May 2, 2023
@mtreinish mtreinish added this to the 0.24.0 milestone May 2, 2023
@mtreinish mtreinish requested review from a team, alexanderivrii and ShellyGarion as code owners May 2, 2023 21:19
@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

@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label May 2, 2023
@coveralls
Copy link

coveralls commented May 2, 2023

Pull Request Test Coverage Report for Build 4875941352

  • 33 of 59 (55.93%) changed or added relevant lines in 2 files are covered.
  • 18 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.1%) to 85.811%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 27 53 50.94%
Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/squ.py 2 80.0%
crates/qasm2/src/lex.rs 4 91.65%
crates/qasm2/src/parse.rs 12 97.11%
Totals Coverage Status
Change from base Build 4873720919: -0.1%
Covered Lines: 70670
Relevant Lines: 82355

💛 - Coveralls

@ajavadia
Copy link
Member

ajavadia commented May 3, 2023

I did add the API change label and upgrade releasenote in that PR.
Are there synthesis plugins that actually use these gate errors/lengths? I'm afraid this migration path from uniform gates to heterogeous gates will be very slow and protracted if we keep adding code without removing old ones.
So maybe at least mark those ones as deprecated to tell downstream to update. But I don't know who's actually using it.

@mtreinish
Copy link
Member Author

Yeah, you did the right things to document a breaking change, but that doesn't mean we really should be breaking the API in this case. I don't really know if there are any plugins out there using these interfaces either, but to a certain degree we can't really know for sure because there is no requirement for any downstream plugin interface users to publish or document their usage. If there isn't any value in having the old view moving forward we can deprecate it in 0.25.0 (we have to release the alternative first before marking it as deprecated). The key though is that we give any plugin authors/maintainers a chance to migrate before removing the API they've potentially been relying on.

Although, in this case I'm personally unclear if there is any harm in keeping both views around long term there really isn't a guarantee that a plugin supports synthesizing to heterogeneous basis gates (lol or that it even respects the basis gates set in the target), but we can discuss that in a deprecation PR.

Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

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

Two documentation comments, otherwise LGTM.

@mtreinish mtreinish requested a review from kdk May 3, 2023 19:15
@kdk kdk enabled auto-merge May 3, 2023 20:38
@kdk kdk added this pull request to the merge queue May 3, 2023
Merged via the queue into Qiskit:main with commit 72e7481 May 3, 2023
mergify bot pushed a commit that referenced this pull request May 3, 2023
* Undo breaking API change in unitary synthesis plugin interface

This commit reverts an unecessary breaking API change that was
introduced in #9175. In that PR the data type for two arguments,
gate_lengths and gate_errors, that was potentially passed to
plugins was changed. This change was well intentioned as it was
used by the default plugin as part of the changes to the default
plugin made in that PR. But this neglected the downstream effects
for any existing plugin interface users relying on the input to
those fields. Making this change to existing fields would break
any plugins that were using the old data format and is a violation
of the Qiskit stability and deprecation policy. This commit reverts
that change so that gate_lengths and gate_errors contain the same data
format as in previous releases. Instead to facilitate the new
workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit,
were added that leverage the new format introduced by #9175. By adding
this as new optional fields existing users can continue to work as
before, but the default plugin which requires the different data
format can use the new data type.

* Update docstring

* Fix copy paste error

(cherry picked from commit 72e7481)
mtreinish added a commit that referenced this pull request May 3, 2023
… (#10075)

* Undo breaking API change in unitary synthesis plugin interface

This commit reverts an unecessary breaking API change that was
introduced in #9175. In that PR the data type for two arguments,
gate_lengths and gate_errors, that was potentially passed to
plugins was changed. This change was well intentioned as it was
used by the default plugin as part of the changes to the default
plugin made in that PR. But this neglected the downstream effects
for any existing plugin interface users relying on the input to
those fields. Making this change to existing fields would break
any plugins that were using the old data format and is a violation
of the Qiskit stability and deprecation policy. This commit reverts
that change so that gate_lengths and gate_errors contain the same data
format as in previous releases. Instead to facilitate the new
workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit,
were added that leverage the new format introduced by #9175. By adding
this as new optional fields existing users can continue to work as
before, but the default plugin which requires the different data
format can use the new data type.

* Update docstring

* Fix copy paste error

(cherry picked from commit 72e7481)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@mtreinish mtreinish deleted the revert-breaking-api-change branch May 4, 2023 10:51
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
…t#10066)

* Undo breaking API change in unitary synthesis plugin interface

This commit reverts an unecessary breaking API change that was
introduced in Qiskit#9175. In that PR the data type for two arguments,
gate_lengths and gate_errors, that was potentially passed to
plugins was changed. This change was well intentioned as it was
used by the default plugin as part of the changes to the default
plugin made in that PR. But this neglected the downstream effects
for any existing plugin interface users relying on the input to
those fields. Making this change to existing fields would break
any plugins that were using the old data format and is a violation
of the Qiskit stability and deprecation policy. This commit reverts
that change so that gate_lengths and gate_errors contain the same data
format as in previous releases. Instead to facilitate the new
workflow a new fields, gate_lengths_by_qubit and gate_errors_by_qubit,
were added that leverage the new format introduced by Qiskit#9175. By adding
this as new optional fields existing users can continue to work as
before, but the default plugin which requires the different data
format can use the new data type.

* Update docstring

* Fix copy paste error
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 priority: high stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants