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

migrated openqaoa-qiskit to qiskit 1.0 #312

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

prakharb10
Copy link

@prakharb10 prakharb10 commented May 30, 2024

Description

Closes #304

qiskit-ibm-provider is also being phased out in favor of qiskit-ibm-runtime. Perhaps we should think about migrating that too?

Checklist

  • I have performed a self-review of my code.
  • I have commented my code and used numpy-style docstrings
  • I have made corresponding updates to the documentation.
  • My changes generate no new warnings
  • I have added/updated tests to make sure bugfix/feature works.
  • New and existing unit tests pass locally with my changes.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@KilianPoirier KilianPoirier self-requested a review May 30, 2024 03:22
@KilianPoirier
Copy link
Collaborator

KilianPoirier commented May 30, 2024

Hi @prakharb10 , thanks for filing a PR to solve this issue.

Can you change the target of the merge to dev, I'll review the changes and approve the workflow once this is changed.
Also, can you confirm that all tests run locally pass?

@prakharb10 prakharb10 changed the base branch from main to dev May 30, 2024 03:24
@prakharb10
Copy link
Author

prakharb10 commented May 30, 2024

Hi @KilianPoirier

I've switched the target branch to dev. The tests passed locally, but some didn't run due to a lack of credentials (I'm not sure if this is the only reason, though). I'm happy to fix any failing tests in the workflow

@KilianPoirier
Copy link
Collaborator

Seems like some of the changes you made were ignored when merging dev back into your branch.
It also seems like qiskit.opflow was not migrated see migration guide.

@prakharb10
Copy link
Author

A sub-error originated from qiskit-ibm-runtime - Qiskit/qiskit-ibm-runtime#1577.
I added pydantic as a requirement to fix it but that is causing errors in other packages to show up. Trying to relax the version requirements

@KilianPoirier
Copy link
Collaborator

I see, I have experienced warnings with the. versioning of pydantic before but it has never caused issues in installing any of the plugins. Hopefully, this is not becoming a major breaking point.

I noticed your comment on migrating to ibm-runtime. If this is something you want to look into, I would happy to support it. I know that this migration would be a big endeavour and this is why we have not pursued it at the moment.

I haven't had a look at qiskit-runtime in quite a while so the compatibility with our codebase may have changed. We developed a light version of openqaoa using runtime primitives some time ago. You can check it out and see for yourself.

Let me know what you think of it!

@prakharb10
Copy link
Author

The test workflow is run on Python 3.8. I've been running tests on Python 3.10 so far and could not reproduce the errors (though I did receive some other errors). Should the tests be run for all Python versions for which the package is built?

This could be why pydantic is not playing well across dependencies. I'm looking into it more.


I can look into migrating to qiskit-ibm-runtime in a separate PR to keep this clean.

@KilianPoirier
Copy link
Collaborator

KilianPoirier commented Jun 1, 2024

You are right, I overlooked this detail, the external dev test workflow is indeed only run on python 3.8. The rest of the workflow is actually run on all supported versions.

I was discussing dropping support for 3.8 with another unitary hacker, see #311 .

On your side you should still ensure that openqaoa can be installed and the tests pass at least on the oldest (you can change the workflow file to 3.9 if that helps) and most recent (3.10 in your case) versions.
If you come across cryptic error messages feel free to discuss them here.

@prakharb10
Copy link
Author

prakharb10 commented Jun 1, 2024

The errors popping up in my local run of the tests were related to mitiq, which, as the OP points out, is because they no longer support 3.8.
Switching to Python 3.9 as the minimum supported version sounds good. I can wait a day or two for #311 to merge and then rebase to see where we stand with the failing tests.

@KilianPoirier
Copy link
Collaborator

So the tests are all passing locally?

@prakharb10
Copy link
Author

Sorry, I missed your message.

Not precisely, I have an M2 MacBook Air, and when I run make dev-install-tests, it fails with the message:

ERROR: Ignored the following versions that require a different python version: 1.14.0rc1 Requires-Python >=3.10; 3.3 Requires-Python >=3.10; 3.3rc0 Requires-Python >=3.10; 8.19.0 Requires-Python >=3.10; 8.20.0 Requires-Python >=3.10; 8.21.0 Requires-Python >=3.10; 8.22.0 Requires-Python >=3.10; 8.22.1 Requires-Python >=3.10; 8.22.2 Requires-Python >=3.10; 8.23.0 Requires-Python >=3.10; 8.24.0 Requires-Python >=3.10; 8.25.0 Requires-Python >=3.10
ERROR: Could not find a version that satisfies the requirement cplex>=22.1.0.0; extra == "tests" (from openqaoa-core[tests]) (from versions: none)
ERROR: No matching distribution found for cplex>=22.1.0.0; extra == "tests"

It appears that cplex does not have a supporting distribution. So far, I have been running it on a codespace to overcome this, but I can't change the Python version easily.

I've updated the workflow file to run on 3.9. I'll see what tests fail now and work through them.

@KilianPoirier
Copy link
Collaborator

It seems like there are some issues with test collection. Try to pull the changes I made on dev recently to prevent issues coming from mitiq.

@KilianPoirier
Copy link
Collaborator

I have noticed the issue with cplex as well, it seems to not have a valid distribution on M2 chips. I'm looking into the problem but it doesn't seem to have a satisfying solution for now.

If you can run it on a codespace, it should not cause any issue because it seems to be working fine on Ubuntu.

Copy link
Collaborator

@KilianPoirier KilianPoirier left a comment

Choose a reason for hiding this comment

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

Looks good to me! Only one question about the qiskit extension

src/openqaoa-core/requirements.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@KilianPoirier KilianPoirier left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I can't seem to be able to assign you to the issue. Can you try commenting the issue you solved and I'll assign you to the bounty then.

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.

Update openqaoa-qiskit to Qiskit 1.0
2 participants