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.

Sign up for GitHub

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

Jump to bottom

Revert base class change introduced in Primitives V1 deprecation #12871

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Revert base class change introduced in Primitives V1 deprecation #12871

merged 1 commit into from
Jul 31, 2024

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Jul 31, 2024 •
edited
Loading

Summary

#12575 deprecated the non-versioned aliases for the V1 interfaces of the primitives as well as the V1 implementations. The base class of the deprecated implementations was changed from the non-versioned to the versioned class, and looking back I think this change should be reverted.

This change breaks instance checks on implementations based on the primitives, and prevents the deprecation warning from being raised properly. Given that the implementations are deprecated and will be removed as the same time as the base class aliases, we didn't really gain much from changing to an un-deprecated base class, and I think it is safer to keep the base classes unchanged and preserve the instance checks.

Details and comments

@ElePT ElePT added this to the 1.2.0 milestone Jul 31, 2024
@ElePT ElePT added Changelog: None Do not include in changelog mod: primitives Related to the Primitives module labels Jul 31, 2024
@ElePT ElePT marked this pull request as ready for review July 31, 2024 15:28
@ElePT ElePT requested review from a team as code owners July 31, 2024 15:28
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @ajavadia
  • @levbishop
  • @t-imamichi

@ElePT ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jul 31, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10183134998

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 4 files are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.003%) to 89.652%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 92.23%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 10181569904: -0.003%
Covered Lines: 67059
Relevant Lines: 74799
💛 - Coveralls

@mtreinish mtreinish enabled auto-merge July 31, 2024 16:32
@mtreinish mtreinish added this pull request to the merge queue Jul 31, 2024
1ucian0
1ucian0 reviewed Jul 31, 2024
View reviewed changes
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.

Does the reno from #12575 need to be changed?

@ElePT
Copy link
Contributor Author

ElePT commented Jul 31, 2024 •
edited
Loading

@1ucian0 This was a "side effect" form the original PR, it doesn't affect the deprecation or the reno (double checked the latest version of the reno from #12835, not affected) 👍

Merged via the queue into Qiskit:main with commit 919cfd3 Jul 31, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Jul 31, 2024
@ElePT @mergify
Revert base class change for Sampler and Estimator V1 implementations…
6312c70 
… to avoid breaking instance checks. (#12871)

(cherry picked from commit 919cfd3)
@mergify mergify bot mentioned this pull request Jul 31, 2024
Merged
@woodsp-ibm woodsp-ibm mentioned this pull request Jul 31, 2024
Merged
mtreinish pushed a commit that referenced this pull request Jul 31, 2024
@ElePT @mtreinish
Revert base class change for Sampler and Estimator V1 implementations…
8d635a8 
… to avoid breaking instance checks. (#12871)

(cherry picked from commit 919cfd3)
github-merge-queue bot pushed a commit that referenced this pull request Jul 31, 2024
@mergify @ElePT
Revert base class change for Sampler and Estimator V1 implementations…
838751e 
… to avoid breaking instance checks. (#12871) (#12873)

(cherry picked from commit 919cfd3)

Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
@ElePT @Procatv
Revert base class change for Sampler and Estimator V1 implementations…
6bade2e 
… to avoid breaking instance checks. (Qiskit#12871)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtreinish mtreinish mtreinish approved these changes

@1ucian0 1ucian0 1ucian0 approved these changes

Assignees
No one assigned
Labels
Changelog: None Do not include in changelog mod: primitives Related to the Primitives module stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ElePT @qiskit-bot @coveralls @1ucian0 @mtreinish