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 deprecation warning to algs module #10406

Merged
merged 8 commits into from
Jul 20, 2023
Merged

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Jul 7, 2023

Summary

This PR shows a deprecation message proposal for the migration of algorithms to a separate community repo (see Qiskit/RFCs#44). Given that there is no module-level decorator, the message is not really standardized, but I believe that wording the message right is particularly important in this case, so please don't hesitate to comment/give suggestions (@1ucian0, @Eric-Arellano)

Details and comments

This PR is blocked by the actual creation of the new repository.

@ElePT ElePT added the on hold Can not fix yet label Jul 7, 2023
@ElePT ElePT added this to the 0.25.0 milestone Jul 7, 2023
Copy link
Collaborator

@Eric-Arellano Eric-Arellano 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! Thanks.

qiskit/algorithms/__init__.py Outdated Show resolved Hide resolved
qiskit/algorithms/__init__.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 7, 2023

Pull Request Test Coverage Report for Build 5611202474

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 9 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.005%) to 86.057%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
crates/qasm2/src/lex.rs 2 90.13%
crates/qasm2/src/parse.rs 6 97.11%
Totals Coverage Status
Change from base Build 5610052878: 0.005%
Covered Lines: 72898
Relevant Lines: 84709

💛 - Coveralls

qiskit/algorithms/__init__.py Outdated Show resolved Hide resolved
qiskit/algorithms/__init__.py Outdated Show resolved Hide resolved
@ElePT ElePT marked this pull request as ready for review July 18, 2023 16:00
@ElePT ElePT requested review from a team and woodsp-ibm as code owners July 18, 2023 16:00
@qiskit-bot
Copy link
Collaborator

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

  • @ElePT
  • @Qiskit/terra-core
  • @woodsp-ibm

Eric-Arellano
Eric-Arellano previously approved these changes Jul 18, 2023
@woodsp-ibm
Copy link
Member

I had mentioned the docs, migration guides in that we might want to add something there. What about release notes - again something can be added after but I imagine this ought to get noted at that level.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

I added a release note in: e1a1c62 and it LGTM now. Feel free to change any of the text in it, I struggled a bit with what to say in there and I'm still not super satisfied with the exact text (but that's probably my bias against the decision to migrate it out of terra leaking through) we can also always tweak the exact release note contents until the final release. I'll hold off on adding to the merge queue until @woodsp-ibm and/or @Cryoris think it's ready to go

mtreinish
mtreinish previously approved these changes Jul 19, 2023
@mtreinish mtreinish removed the on hold Can not fix yet label Jul 19, 2023
@woodsp-ibm
Copy link
Member

It LGTM - the only comment/text I might make/add is that if users are still using the old deprecated algorithms/QuantumInstance they need to first migrate to the primitive based algos and then switch. We might want to point to the algorithm migration guide in the release note along with such a comment to aid them in this regard. If they are already using the primitive based algos then they should be good to use qiskit_algorithms and it should just be that simple import change.

woodsp-ibm
woodsp-ibm previously approved these changes Jul 19, 2023
qiskit/algorithms/__init__.py Outdated Show resolved Hide resolved
qiskit/algorithms/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Matthew Treinish <[email protected]>
@mtreinish mtreinish enabled auto-merge July 20, 2023 12:01
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM and I've verified from qiskit import algorithms raises the warnings as expected so I think we're good to go here and we can concentrate the efforts on getting the qiskit-algorithms package ready.

@mtreinish mtreinish added this pull request to the merge queue Jul 20, 2023
Merged via the queue into Qiskit:main with commit 2616602 Jul 20, 2023
13 checks passed
to24toro pushed a commit to to24toro/qiskit-terra that referenced this pull request Aug 3, 2023
* Add deprecation warning to algs module

* Apply suggestions from code review

Co-authored-by: Eric Arellano <[email protected]>

* Add link to repo

* Add release note

* Add migration guide to release note

* Apply suggestions from code review

Co-authored-by: Matthew Treinish <[email protected]>

---------

Co-authored-by: Eric Arellano <[email protected]>
Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Steve Wood <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog mod: algorithms Related to the Algorithms module priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants