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 qpy deprecation warning class #11260

Merged
merged 5 commits into from Dec 20, 2023
Merged

Add qpy deprecation warning class #11260

merged 5 commits into from Dec 20, 2023

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Nov 16, 2023

Summary

This PR proposes a new QPYLoadingDeprecatedFeatureWarning class inspired in numpy's VisibleDeprecationWarning with the purpose of ensuring that deprecation messages reach the user even if they are raised from variable points in the call stack. The intended use case are qpy loading functions that can be called recursively (hence the name).

Details and comments

The original issue was found during the review of #11257

Note: I have tested that the warning is visible using the example from #11257. I dumped a schedule with a complex amplitude using qpy v.5 (qiskit-terra 0.21) and tried loading it with the latest qpy version. The warning wouldn't surface with the usual DeprecationWarning class but it did with QPYLoadingDeprecatedFeatureWarning:

Screenshot 2023-11-20 at 15 26 33

@coveralls
Copy link

coveralls commented Nov 16, 2023

Pull Request Test Coverage Report for Build 7274445496

  • 4 of 4 (100.0%) changed or added relevant lines in 3 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 87.555%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 5 91.41%
Totals Coverage Status
Change from base Build 7266513358: 0.03%
Covered Lines: 59161
Relevant Lines: 67570

💛 - 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.

Don't forget to add a documentation line for the new warning in the QPY API docs (you can use autoexception, because Python makes tons of sense).

@TsafrirA
Copy link
Collaborator

Do you plan on having this limited to DeprecationWarning class? It will be helpful to be able to raise other types of warnings.

@ElePT ElePT marked this pull request as ready for review November 20, 2023 14:21
@ElePT ElePT requested a review from a team as a code owner November 20, 2023 14:21
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

@ElePT
Copy link
Contributor Author

ElePT commented Nov 20, 2023

Do you plan on having this limited to DeprecationWarning class? It will be helpful to be able to raise other types of warnings.

What other types of warnings are you thinking about @TsafrirA? This seemed to be an issue particularly for DeprecationWarning, but I think that most of the exceptions we work with should normally surface without the need for a new class.

@TsafrirA
Copy link
Collaborator

What other types of warnings are you thinking about

I am planning to replace the deprecation warning with a user warning once the process of #11257 is completed. That shouldn't be a problem?

@ElePT
Copy link
Contributor Author

ElePT commented Nov 21, 2023

What other types of warnings are you thinking about

I am planning to replace the deprecation warning with a user warning once the process of #11257 is completed. That shouldn't be a problem?

You can replace DeprecationWarning with QPYLoadingDeprecatedFeatureWarning in #11257 without issues. The only thing to keep in mind is that to catch it, you must specify the QPYLoadingDeprecatedFeatureWarning type.

@TsafrirA
Copy link
Collaborator

I didn't explain myself well.

Once we remove the support which is deprecated in #11257, pulses loaded from old QPY files will be altered. I was planning of issuing a user warning at that point to alert the user to the change. Do we then need a similar mechanism for a user warning?

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.

This looks sensible to me (except for why Python calls warnings exceptions), thanks!

@jakelishman jakelishman added this pull request to the merge queue Dec 20, 2023
@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog mod: qpy Related to QPY serialization labels Dec 20, 2023
@jakelishman jakelishman added this to the 1.0.0 milestone Dec 20, 2023
Merged via the queue into main with commit 08fcdb5 Dec 20, 2023
15 checks passed
@ElePT ElePT deleted the qpy-depr-warning branch January 8, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog mod: qpy Related to QPY serialization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants