Skip to content

Fix deprecation warning for deprecated_functionality#8851

Merged
mergify[bot] merged 3 commits into
Qiskit:mainfrom
garrison:deprecated-functionality
Oct 12, 2022
Merged

Fix deprecation warning for deprecated_functionality#8851
mergify[bot] merged 3 commits into
Qiskit:mainfrom
garrison:deprecated-functionality

Conversation

@garrison
Copy link
Copy Markdown
Member

@garrison garrison commented Oct 7, 2022

Summary

This fixes some deprecation warnings that pop up during my use of main (or, equivalently, qiskit-terra==0.22.0rc1)

Details and comments

@garrison garrison requested review from a team, eggerdj and wshanks as code owners October 7, 2022 17:08
@qiskit-bot
Copy link
Copy Markdown
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

mtreinish
mtreinish previously approved these changes Oct 7, 2022
Copy link
Copy Markdown
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.

Lol, I haven't gone through the release notes yet, I didn't realize that we deprecated a deprecation decorator. I find that quite amusing

This LGTM, thanks for identifying and fixing this.

@mtreinish mtreinish added automerge Changelog: None Do not include in the GitHub Release changelog. stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. labels Oct 7, 2022
@wshanks
Copy link
Copy Markdown
Contributor

wshanks commented Oct 7, 2022

I didn't realize that we deprecated a deprecation decorator. I find that quite amusing

I found it confusing that the PR that did this (#8696) didn't have a single joke in it.

@garrison
Copy link
Copy Markdown
Member Author

garrison commented Oct 7, 2022

When I saw

DeprecationWarning: Deprecated since Terra 0.22.0. Use 'qiskit.utils.deprecate_function' instead.
    def to_dict(self) -> Dict[str, Any]:

I thought for sure that there must have been an off-by-one error in the stacklevel of the warning. But no, I was wrong about that. 😄

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 7, 2022

Pull Request Test Coverage Report for Build 3236527215

  • 2 of 5 (40.0%) changed or added relevant lines in 1 file are covered.
  • 42 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.05%) to 84.712%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/pulse/transforms/alignments.py 2 5 40.0%
Files with Coverage Reduction New Missed Lines %
qiskit/pulse/utils.py 1 94.29%
qiskit/pulse/transforms/alignments.py 3 91.78%
src/sabre_swap/neighbor_table.rs 7 69.23%
src/sampled_exp_val.rs 14 63.93%
src/results/marginalization.rs 17 78.74%
Totals Coverage Status
Change from base Build 3236526677: -0.05%
Covered Lines: 61897
Relevant Lines: 73068

💛 - Coveralls

@garrison
Copy link
Copy Markdown
Member Author

Lint is failing because qiskit.pulse.transforms.alignments -> qiskit.utils means there are now cyclic imports.

@mtreinish
Copy link
Copy Markdown
Member

Sigh, our import "tree" is kind of a disaster it looks like the root cause is that the utils path is shared with the new decorator and the quantum instance (which imports everything because it's a big layer violation). I think the best way to handle this is probably to just avoid the decorator and emit the warning inline with the desired text.

mtreinish
mtreinish previously approved these changes Oct 11, 2022
@garrison
Copy link
Copy Markdown
Member Author

While writing the warning string, I learned that these to_dict() methods were deprecated as part of #7300, which was part of Terra 0.21.

@mtreinish
Copy link
Copy Markdown
Member

It looks like reno is getting confused by the merge commit which moved a bunch of release notes in #8850 and is causing it to get confused about the source of release notes. I think the only way to fix that is to rebase the branch on main and force push it to your fork so that reno can track the history with the file moves.

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.

I'd love to have the time to go through Terra and reorganise the imports such that there's a proper import-time hierarchy, places that import "higher" in the hierarchy only do it at runtime, and it's all enforced by lint, but it's such a complex task, I've no idea when that would become possible. This seems sensible in the interim.

@mergify mergify Bot merged commit 43cf42e into Qiskit:main Oct 12, 2022
mergify Bot added a commit that referenced this pull request Oct 12, 2022
* Fix deprecation warning for `deprecated_functionality`

* Emit the deprecation warning inline to avoid cyclic import

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 43cf42e)
@garrison garrison deleted the deprecated-functionality branch October 12, 2022 18:41
mergify Bot added a commit that referenced this pull request Oct 12, 2022
* Fix deprecation warning for `deprecated_functionality`

* Emit the deprecation warning inline to avoid cyclic import

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 43cf42e)

Co-authored-by: Jim Garrison <garrison@ibm.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog. stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants