Skip to content

Add qpy deprecation warning class#11260

Merged
jakelishman merged 5 commits into
mainfrom
qpy-depr-warning
Dec 20, 2023
Merged

Add qpy deprecation warning class#11260
jakelishman merged 5 commits into
mainfrom
qpy-depr-warning

Conversation

@ElePT
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

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

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

@ElePT
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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: Added Add an "Added" entry in the GitHub Release 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
@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: Added Add an "Added" entry in the GitHub Release changelog. mod: qpy Related to QPY serialization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants