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 tests for Qiskit 1.0 #41

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Update tests for Qiskit 1.0 #41

merged 3 commits into from
Feb 23, 2024

Conversation

mtreinish
Copy link
Member

This commit updates the execute tests to remove the use of execute() which was removed in Qiskit 1.0 after being deprecated in 0.46. How this managed to merge, I'm not entirely clear on because it should have failed CI on the removal PR, so there is likely a configuration issue in the neko custom action as well that will need to be fixed.

This commit updates the execute tests to remove the use of execute()
which was removed in Qiskit 1.0 after being deprecated in 0.46. How this
managed to merge, I'm not entirely clear on because it should have
failed CI on the removal PR, so there is likely a configuration issue in
the neko custom action as well that will need to be fixed.
Copy link
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.

The execute changes all look totally fine, but I'm not sure if bringing in Runtime to Neko is going to be safe long term. Do you think it could be worth dropping the backend_selection ability of the Aer plugin to get specific IBM-like devices? On a quick scan, I couldn't see any repos actually using the functionality.

@@ -4,6 +4,7 @@ qiskit-aer
qiskit-nature[pyscf]
qiskit-experiments
qiskit-machine-learning
qiskit-ibm-runtime>=0.19
Copy link
Member

Choose a reason for hiding this comment

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

Is there a potential problem here that Runtime doesn't use Neko, so could merge breaking changes to the Neko actions unknowingly?

Copy link
Member

Choose a reason for hiding this comment

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

During my searching for backend_selection, I also realised that qiskit-algorithms doesn't have a Neko job in CI, so could also break it without us knowing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it could be worth dropping the backend_selection ability of the Aer plugin to get specific IBM-like devices? On a quick scan, I couldn't see any repos actually using the functionality.

Is there a potential problem here that Runtime doesn't use Neko, so could merge breaking changes to the Neko actions unknowingly?

Yeah, we should add a CI job to qiskit-ibm-runtime if we go this path. We could potentially remove this functionality and just rely on users to make their own backend plugin to use the fake backends. I was using this locally to test things out when I was first getting neko setup, but I don't know how widely used this part of the interface was. The plugin interface supports it but each plugin doesn't need to implement name selection.

During my searching for backend_selection, I also realised that qiskit-algorithms doesn't have a Neko job in CI, so could also break it without us knowing.

Yeah we should add a job to qiskit-algorithms, we can ask @woodsp-ibm or @ElePT about it.

Copy link
Member

Choose a reason for hiding this comment

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

Well we do have a jobs in Algorithms that run all the unit tests against Qiskit Main so at least from that side we can see and it did turn up an issue along the way to the final 1.0. They are not required for CI, all that is is done against stable release, but they are run in each PR and in the nightly CI run.

image

Copy link
Member

Choose a reason for hiding this comment

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

Neko's a bit different to testing against Qiskit main, though - Algorithms running Neko in CI is / would be just as much a protection for all the other Neko users as for Algorithms itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've proposed a PR to qiskit-ibm-runtime here: Qiskit/qiskit-ibm-runtime#1422 which adds a matching job. We need to update the custom action to leverage the backend selection mechanism to use the fake backends in the package. But that will have to come after this PR as this is necessary to unblock CI for everyone.

mtreinish added a commit to mtreinish/qiskit-ibm-runtime that referenced this pull request Feb 23, 2024
In Qiskit/qiskit-neko#41 we're updating the default simulator backend
plugin used by qiskit-neko to rely on the fake backends provider in the
qiskit-ibm-runtime project. A key aspect of qiskit-neko is the
reciprocity of testing to maintain compatibility in the qiskit
ecosystem (see:
https://github.com/Qiskit/qiskit-neko?tab=readme-ov-file#downstream-usage-in-testing)
so to ensure this is maintainable and the interface remains compatible
for every project using neko this commit adds a corresponding job to
exercise the qiskit-ibm-runtime usage in the tests. However, this is
blocked until after Qiskit/qiskit-neko#41 merges as that is needed to
fix the test suite. A followup will be needed to add a backend selection
to the action configuration so that we can actually use a fake backend.

A longer term step we should investigate is adding a qiskit-neko plugin
to the repo which would enable using qiskit-ibm-runtime's backends and
primitives directly for integration testing with real backends. But that
is much longer term and isn't part of this
Copy link
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.

Yep, this is fine by me - getting everyone's CI less angry immediately is a much better state of affairs, and we can worry about stabilising it against potential breakage afterwards.

@jakelishman jakelishman merged commit 336bd34 into main Feb 23, 2024
10 checks passed
@jakelishman jakelishman deleted the fix-1.0 branch February 23, 2024 13:10
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.

None yet

3 participants