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

__qiskit_version__ deprecation #10242

Merged
merged 16 commits into from
Jul 20, 2023
Merged

__qiskit_version__ deprecation #10242

merged 16 commits into from
Jul 20, 2023

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Jun 7, 2023

Fixes #9753

This PR deprecates qiskit.__qiskit_version__ in favor of qiskit.__version__

The %qiskit_version_table now does not depend on qiskit.__qiskit_version__. I also extended to list in the table all the modules/packages that include the word qiskit to have this output:

Screenshot 2023-06-07 at 17 07 57

@1ucian0 1ucian0 requested review from a team and woodsp-ibm as code owners June 7, 2023 15:08
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Jun 10, 2023

Pull Request Test Coverage Report for Build 5604152404

  • 4 of 19 (21.05%) changed or added relevant lines in 2 files are covered.
  • 22 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 86.054%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/tools/jupyter/version_table.py 0 15 0.0%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.14%
crates/qasm2/src/parse.rs 18 96.65%
Totals Coverage Status
Change from base Build 5603596398: -0.03%
Covered Lines: 72611
Relevant Lines: 84378

💛 - Coveralls

@1ucian0 1ucian0 added this to the 0.25.0 milestone Jun 12, 2023
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.

Overall I'm fine with this, it's a little weird to do the packages with qiskit in the name like that for the magic. But since it's isolated to just the widget it's a reasonable tradeoff for people that like the table in notebooks. Just a couple small inline comments.

@@ -14,6 +14,8 @@
"""A module for monitoring backends."""

import time
from sys import modules
from collections import OrderedDict
Copy link
Member

Choose a reason for hiding this comment

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

Do you need an ordered dict? For all our supported versions of Python all dicts are insertion ordered now.

Copy link
Member Author

@1ucian0 1ucian0 Jul 13, 2023

Choose a reason for hiding this comment

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

Fixed in d0d8588

warnings.warn(
"qiskit.__qiskit_version__ is deprecated since "
"Qiskit Terra 0.25.0, and will be removed 3 months or more later. "
"Instead, you should use qiskit.__version__.",
Copy link
Member

Choose a reason for hiding this comment

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

You might want to mention that for other packages that used to be in __qiskit_version__ those packages should have a __version__ attribute too.

Copy link
Member Author

@1ucian0 1ucian0 Jul 13, 2023

Choose a reason for hiding this comment

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

good point. done in a846ffe

Comment on lines 4 to 5
The magic ``%qiskit_version_table`` from ``qiskit.tools.jupyter`` now includes all
the modules with ``'qiskit`` in the name.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe say "all imported packages with qiskit in the name"?

Copy link
Member Author

@1ucian0 1ucian0 Jul 13, 2023

Choose a reason for hiding this comment

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

reworded in bab5901

qiskit/tools/jupyter/version_table.py Show resolved Hide resolved
@mtreinish mtreinish added the Changelog: Deprecation Include in "Deprecated" section of changelog label Jul 13, 2023
Comment on lines 50 to 55
qiskit_modules = {module.split(".")[0] for module in modules.keys() if "qiskit" in module}
for qiskit_module in qiskit_modules:
try:
packages[metadata(qiskit_module)["Name"]] = metadata(qiskit_module)["Version"]
except PackageNotFoundError:
packages["qiskit"] = None
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this with fresh eyes my concern here is that the module name in sys.modules isn't necessarily going to be the package name that importlib.metadata will find. The best example of this is actually qiskit, if you just have terra installed you would need to call metadata("qiskit-terra") not metadata("qiskit") to get the installed package version for the qiskit module in sys.modules.

I think it might be better to do:

Suggested change
qiskit_modules = {module.split(".")[0] for module in modules.keys() if "qiskit" in module}
for qiskit_module in qiskit_modules:
try:
packages[metadata(qiskit_module)["Name"]] = metadata(qiskit_module)["Version"]
except PackageNotFoundError:
packages["qiskit"] = None
qiskit_modules = {module.split(".")[0] for module in modules.keys() if "qiskit" in module}
for qiskit_module in qiskit_modules:
packages[qiskit_module] = getattr(sys.modules[qiskit_module], "__version__", None)

so it just uses the version reported in the __version__ attribute.

Comment on lines 50 to 55
qiskit_modules = {module.split(".")[0] for module in modules.keys() if "qiskit" in module}
for qiskit_module in qiskit_modules:
try:
packages[metadata(qiskit_module)["Name"]] = metadata(qiskit_module)["Version"]
except PackageNotFoundError:
packages["qiskit"] = None
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be qiskit_module right, because if a version can't be found for qiskit_module you don't want to invalidate the qiskit package versio.

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
mtreinish
mtreinish previously approved these changes Jul 18, 2023
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.

LGTM, thanks for the update

@mtreinish mtreinish enabled auto-merge July 18, 2023 19:00
@mtreinish mtreinish disabled auto-merge July 18, 2023 19:02
qiskit/version.py Outdated Show resolved Hide resolved
@mtreinish mtreinish dismissed their stale review July 18, 2023 19:04

One more comment

@1ucian0 1ucian0 marked this pull request as draft July 19, 2023 11:28
@mtreinish mtreinish marked this pull request as ready for review July 19, 2023 21:13
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@mtreinish mtreinish enabled auto-merge July 19, 2023 21:13
@mtreinish mtreinish added this pull request to the merge queue Jul 19, 2023
Merged via the queue into Qiskit:main with commit d97f805 Jul 20, 2023
@1ucian0 1ucian0 deleted the fixes/9753/1 branch July 24, 2023 11:23
to24toro pushed a commit to to24toro/qiskit-terra that referenced this pull request Aug 3, 2023
* __qiskit_version__ deprecation

* reno

* reno feature

* PackageNotFoundError

* revert stacklevel=2

* revert change in test

* remove OrderedDict

* other qiskit.__qiskit_version__  packages have their own __version__

* wording in the reno

* lint

* Update qiskit/tools/jupyter/version_table.py

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Update qiskit/tools/jupyter/version_table.py

* Restore removed version checks

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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 priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update __qiskit_version__
4 participants