-
Notifications
You must be signed in to change notification settings - Fork 127
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
Replace qiskit-terra references with qiskit references #1318
Conversation
This change brings qiskit-experiments in line with other packages like qiskit-ibm-provider and qiskit-aer and prepares for the deprecation of the qiskit-terra package. The effective minimum version of qiskit-terra is bumped up one minor version from 0.24 to 0.25 (linked to qiskit 0.44) as part of this change, but since qiskit 0.45 has already been released this should not be an impactful change for most users.
One question is whether to add a |
|
Good point. I was mainly thinking about reporting a dependence on |
@coruscating I removed more terra usage (see edited PR description). The remaining terra references in the repo are in old release notes. The most disruptive changes are renaming the tox environments and the GitHub workflow job for testing again Qiskit main, but I don't think we should stick with the old names to avoid disruption since they will become confusing as the memory of terra fades. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is good overall and I agree with keeping older mentions of Terra in the release notes.
return pkg_resources.get_distribution("qiskit").version | ||
except Exception: # pylint: disable=broad-except | ||
return {"qiskit-terra": terra_version, "qiskit-experiments": experiments_version} | ||
return importlib.metadata.distribution("qiskit").version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not also return the Qiskit Experiments version like the previous code did in the except
clause? I'm not quite sure what this function is meant for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a little confused by this part. Qiskit is a required dependency, so we should be able to get its version without error and not need the except block, so I went with what the try block was doing to continue the existing behavior. I pushed a new commit to switch to the except block style of capturing qiskit and qiskit-experiments. I thought about capturing other packages (qiskit-ibm-experiment?) but I think these two make the most sense. qiskit changes could change transpilation and affect the circuits in something like RB. qiskit-experiments could change the circuits method or the analysis function. So it is good to capture both with the experiment metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, saving both makes sense and we probably should have been doing that earlier. We may have to change this to git commit IDs in the future but this is good for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. I had been thinking about that. I have done it on other repos before.
upgrade: | ||
- | | ||
The dependency on ``qiskit-terra`` was replaced with a dependency on | ||
``qiskit``. This change follows the move in upstream Qiskit to rename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the old and new version numbers be explicitly mentioned here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done but we need to keep this release note in mind if we merge #1288 and increase the minimum qiskit version.
@coruscating Besides addressing your comments, I added an upper bound on the qiskit version like you had suggested outside of the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I added the |
@wshanks I've just been redirecting users to the release notes in the github releases section since I haven't been careful with changelog labels (for example, a lot of backlogged PRs aren't correctly labeled since labels on the original PR don't get carried over to the automatic mergify PRs). We should revisit our changelog policy and decide what to do about them going forward. |
…ity#1318) This change brings qiskit-experiments in line with other packages like qiskit-ibm-provider and qiskit-aer and prepares for the deprecation of the qiskit-terra package. Detailed changes: * Replace qiskit-terra dependency with qiskit dependency - The effective minimum version of qiskit-terra is bumped up one minor version from 0.24 to 0.25 (linked to qiskit 0.44) as part of this change, but since qiskit 0.45 has already been released this should not be an impactful change for most users. * Simplify the code for logging the version of qiskit used. It should always use the Qiskit version as a string. Previously, it could sometimes use a dictionary with the Qiskit and Qiskit Experiments versions. * Remove workaround for `aquire_alignment` misspelling in old versions of terra (because it was commented referencing terra) * Replace qiskit-terra in requirements-dev.txt and tox.ini * Replace various references to "terra" in various code comments with "Qiskit" * Rename terra-main and docs-terra-main tox environments to qiskit-main and docs-qiskit-main * Rename GitHub workflow job terra-main-tests to qiskit-main-tests The only remaining references to "terra" in the codebase are in old release notes.
…ity#1318) This change brings qiskit-experiments in line with other packages like qiskit-ibm-provider and qiskit-aer and prepares for the deprecation of the qiskit-terra package. Detailed changes: * Replace qiskit-terra dependency with qiskit dependency - The effective minimum version of qiskit-terra is bumped up one minor version from 0.24 to 0.25 (linked to qiskit 0.44) as part of this change, but since qiskit 0.45 has already been released this should not be an impactful change for most users. * Simplify the code for logging the version of qiskit used. It should always use the Qiskit version as a string. Previously, it could sometimes use a dictionary with the Qiskit and Qiskit Experiments versions. * Remove workaround for `aquire_alignment` misspelling in old versions of terra (because it was commented referencing terra) * Replace qiskit-terra in requirements-dev.txt and tox.ini * Replace various references to "terra" in various code comments with "Qiskit" * Rename terra-main and docs-terra-main tox environments to qiskit-main and docs-qiskit-main * Rename GitHub workflow job terra-main-tests to qiskit-main-tests The only remaining references to "terra" in the codebase are in old release notes.
…ity#1318) This change brings qiskit-experiments in line with other packages like qiskit-ibm-provider and qiskit-aer and prepares for the deprecation of the qiskit-terra package. Detailed changes: * Replace qiskit-terra dependency with qiskit dependency - The effective minimum version of qiskit-terra is bumped up one minor version from 0.24 to 0.25 (linked to qiskit 0.44) as part of this change, but since qiskit 0.45 has already been released this should not be an impactful change for most users. * Simplify the code for logging the version of qiskit used. It should always use the Qiskit version as a string. Previously, it could sometimes use a dictionary with the Qiskit and Qiskit Experiments versions. * Remove workaround for `aquire_alignment` misspelling in old versions of terra (because it was commented referencing terra) * Replace qiskit-terra in requirements-dev.txt and tox.ini * Replace various references to "terra" in various code comments with "Qiskit" * Rename terra-main and docs-terra-main tox environments to qiskit-main and docs-qiskit-main * Rename GitHub workflow job terra-main-tests to qiskit-main-tests The only remaining references to "terra" in the codebase are in old release notes.
This change brings qiskit-experiments in line with other packages like qiskit-ibm-provider and qiskit-aer and prepares for the deprecation of the qiskit-terra package.
Detailed changes:
aquire_alignment
misspelling in old versions of terra (because it was commented referencing terra)The only remaining references to "terra" in the codebase are in old release notes.