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

Verify that deprecate_arg has not introduced message regressions #9992

Closed
10 tasks
jakelishman opened this issue Apr 19, 2023 · 1 comment · Fixed by #9884
Closed
10 tasks

Verify that deprecate_arg has not introduced message regressions #9992

jakelishman opened this issue Apr 19, 2023 · 1 comment · Fixed by #9884
Milestone

Comments

@jakelishman
Copy link
Member

jakelishman commented Apr 19, 2023

With #9865 open (possible fix in #9884), there is currently a possible regression. If an argument to be deprecated can reasonably be passed positionally and was previously handled by a hard-coded if x is not None (or similar) check inside the function body, the warning will no longer be raised. We need to ensure by some mechanism that all these warnings are being correctly raised in the Terra 0.24.

Our unit tests should have caught any regressions, but we don't necessarily have great coverage of those, and there are some examples (e.g. in #9864 and #9869) where tests of the deprecations were changed to allow a CI pass.

This issue can be solved either by #9884 or an alternative merging, or by us manually going through the current uses of deprecate_args, and reverting any places where the argument would / could reasonably be passed positionally (e.g. if it's 4 arguments deep into keyworded defaults, it's highly unlikely anyone's passing it positionally). This can be checked by searching for deprecate_arg in the code base, or alternatively the PRs that introduced most uses are:

These should be ticked when it is validated that they contain no regressions.

(the most important ones to revisit are the merged ones)

@jakelishman jakelishman added this to the 0.24.0 milestone Apr 19, 2023
@Eric-Arellano
Copy link
Collaborator

Agreed. Note that this is a regression from direct calls to warnings.warn to @deprecate_arg. It is safe to migrate from @deprecate_arguments to @deprecate_arg because it had this same deficiency.

Also, @deprecate_arg is handling one edge case we were not properly handling before, when the user explicitly sets bad_arg=None. That will cause their code to break in future Qiskit versions, so it's an issue that we were not handling before. But, it was already broken, so a higher priority is probably avoiding the introduction of a new regression.

--

I'll work on your review feedback in #9884 today, and then we can decide tomorrow whether to move forward with #9884 vs. the audit you propose above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants