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

Update test imports for qiskit-aer 0.11 #913

Merged
merged 5 commits into from
Sep 22, 2022
Merged

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Sep 16, 2022

Summary

qiskit-aer moved from a namespace package at qiskit.providers.aer to its own package qiskit_aer with version 0.11. Tests in qiskit-experiments importing from the old path fail with qiskit-aer 0.11. This PR updates those imports.

Closes #912

Details and comments

This is a follow up to #911.

For the aer change, see See Qiskit/qiskit-aer#1526.

@conradhaupt and I were running into this, but I can't add him as a reviewer.

@wshanks wshanks requested a review from coruscating September 16, 2022 17:37
@wshanks
Copy link
Collaborator Author

wshanks commented Sep 16, 2022

Reading #1526 more closely, I see that Qiskit/qiskit#5089 added some importing magic to redirect qiskit.providers.aer to qiskit_aer which allowed the qiskit-experiments tests to keep passing with 0.11. The actual issue requiring qiskit-experiments to update the aer imports is that pylint is not smart enough to follow the importing magic and complains about qiskit.providers.aer imports when qiskit-aer 0.11 is installed. See this run for example.

Maybe we could have satisfied pylint in a different way without updating the imports yet, but we only use aer in the tests, so it doesn't seem like there is a need to keep compatibility with qiskit-aer<0.11.

@conradhaupt
Copy link
Contributor

conradhaupt commented Sep 16, 2022

Maybe we could have satisfied pylint in a different way without updating the imports yet, but we only use aer in the tests, so it doesn't seem like there is a need to keep compatibility with qiskit-aer<0.11.

We probably could have configured pylint to see it, but I feel like that would have been far more work than is necessary. As you said, we only use Aer in tests. My understanding of Qiskit/qiskit#5089 and Qiskit/qiskit-aer#1526 is that Qiskit Aer has officially moved to qiskit_aer, but that qiskit.provider.aer is provided so that old code still works? If that is the case, then it's better that we migrate to using qiskit_aer.

@wshanks
Copy link
Collaborator Author

wshanks commented Sep 16, 2022

My understanding of Qiskit/qiskit#5089 and Qiskit/qiskit-aer#1526 is that Qiskit Aer has officially moved to qiskit_aer, but that qiskit.provider.aer is provided so that old code still works?

Right. If aer were an actual dependency of qiskit-experiments, I think maybe we should have figured out how to get pylint to ignore it since it seems unnecessary to force users to update to keep pylint happy, but since this is just the version that gets used in tests it seems okay to require the new version.

@mtreinish
Copy link
Contributor

Yeah, the new path is qiskit_aer. The import redirect hooks were added to qiskit-terra (which owns the qiskit.* namespace) for backwards compatibility. So that anyone with existing code, or code they want to support > 1 version of qiskit-aer, can continue to use the old namespace. Eventually importing from qiskit.providers.aer or qiskit.Aer will emit a deprecation warning asking you to move to qiskit_aer. It is better longer term to update the imports to the new path, but you're both correct it's not actually required. You could just add qiskit.providers.aer to the pylint module ignore list in the config to fix the failures. I guess it depends on what you want do with supporting aer versions and whether >=0.11.0 is required

@wshanks
Copy link
Collaborator Author

wshanks commented Sep 19, 2022

Since aer is only used in tests, I think it is okay to increase the minimum version. It is probably uncommon for someone to want to run the tests and not be able to update aer (in the worst case, they can make a separate environment for running qiskit-experiments tests). We could ignore the package in pylint, but then we have the small downside of pylint not checking aer function signatures. Maybe doing try/except with both import paths could support 0.10 or 0.11 and still get pylint to check the aer signatures.

I don't feel too strongly. The main thing is that we pinned aer in #911 to get tests working again and should try to get it unpinned. I defer to @coruscating on which way is best.

@itoko
Copy link
Contributor

itoko commented Sep 20, 2022

+1 to increase the minimum version (in requirements-dev.txt) since aer is only used in tests.

Copy link
Collaborator

@coruscating coruscating 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

@wshanks wshanks merged commit d42cb84 into qiskit-community:main Sep 22, 2022
@wshanks wshanks deleted the aer branch September 22, 2022 00:01
nkanazawa1989 added a commit that referenced this pull request Sep 29, 2022
* Backend timing helper for experiments (#860)

Added `BackendTiming` class with methods for helping round delay and pulse durations to match backend timing constraints.

Co-authored-by: Naoki Kanazawa <[email protected]>

* Update test imports for qiskit-aer 0.11 (#913)

Update test imports for qiskit-aer 0.11

See Qiskit/qiskit-aer#1526

Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
nkanazawa1989 pushed a commit to nkanazawa1989/qiskit-experiments that referenced this pull request Sep 29, 2022
wshanks added a commit to thaddeus-pellegrini/qiskit-experiments that referenced this pull request Oct 14, 2022
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 this pull request may close these issues.

Tests not compatible with qiskit-aer 0.11
5 participants