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

Add support to python 3.11 #311

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

Conversation

rodolfocarobene
Copy link

@rodolfocarobene rodolfocarobene commented May 29, 2024

Fixes #307.

Description

For the moment I've just upgraded all the setup.py and the workflows.
I will remove this PR from draft mode as soon as I'm finished with the changes. In particular, I want to check that the code is all compatible with python3.11 or if modifications are required.
I've yet to check the correctness of the dependencies in the opeqaoa-core: they may be outdated or require changes with the python version

Checklist

  • I have performed a self-review of my code.
  • 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

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

@rodolfocarobene rodolfocarobene marked this pull request as ready for review May 29, 2024 14:42
@rodolfocarobene
Copy link
Author

Hi, locally all the testes pass correctly (or fail because they are using connections I cannot access) so I would say it is all working. We should check with the workflows if everything is truly good.

Tbh, I think this project could benefit of some modification in the setup.py / pyproject / version / dependencies organization. It seems to me that there is some duplication in the versioning and that the setup.py could be completely removed in favor of the pyproject. Moreover, having something like poetry to manage dependency could help automatize some of the things I did today :-)

@KilianPoirier KilianPoirier self-requested a review May 30, 2024 02:43
@KilianPoirier
Copy link
Collaborator

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

I have a few comments/questions:

  • First, can you merge your branch into dev rather than main, this allows us to control the release version more carefully and make sure no bug spreads to the pip version of the package. I approve the workflow run after this step.
  • Is there anything preventing the use of python 3.12? If not, can you also add support for this more recent version of python. I believe most of the packages associated with plugins also support it.
  • Finally, I think the main roadblock when we first tried to upgrade python was the removal of setuptools from the main python library. Can you directly install your version of openqaoa in a fresh python 3.12 environment like we're doing it in the .yml?

If you want to contribute further and reorganise setup.py / pyproject / version / dependencies, feel free to do so. I would be happy to help you integrate these changes as well.
Since this is a Unitary Hack related PR, you may as well tackle this cleaning task after we approve this PR. Let me know what you think!

@rodolfocarobene rodolfocarobene changed the base branch from main to dev May 30, 2024 10:19
@rodolfocarobene
Copy link
Author

Sorry @KilianPoirier , I think I had slightly misunderstood the issue.
I've now removed all the setup.py files and the requirements.txt, moving everything to the pyproject.toml as it is advised (I believe). Since "setuptools" is in the build-system requirements, it should be installed correctly when installing the packages.

I've tried it on a new environment and the installation of every specific package seemed to work. Please let me know if it is ok for you r if there are still modification required.

(Sorry for the useless line diffs, my editor removes automatically leading spaces... If you want I can remove them)

@rodolfocarobene rodolfocarobene changed the title Add support to python 3.11 Add support to python 3.11-3.12 May 30, 2024
@KilianPoirier
Copy link
Collaborator

KilianPoirier commented May 31, 2024

Hey @rodolfocarobene thank you for revisiting your PR and migrating all of the setup instructions to pyproject.toml. It looks good at first glance.

I have approved the test workflow but it seemed like it fails at installation.
I think it is due to the requirements on the version of ipython that is not compatible with the lowest versions of python we are supporting.

I think the best way to ensure that the test pass both locally and in the GH workflow is to use at least two environments, one with python 3.8 and one with a higher version, I think 3.12 should do the trick. I'll wait for the nest changes to approve the tests and review the code once the tests are clear.

@rodolfocarobene
Copy link
Author

Hi, I was trying to fix the errors, but I noticed that mitiq is at the moment not compatible with Python 3.12...
https://github.com/unitaryfund/mitiq/blob/d35b7aad59b393aa539ceb2cb52244e84c19c1ee/setup.py#L70

Since openqaoa-core requires it (in dev) I'm afraid that Python3.12 cannot be fully supported

@KilianPoirier
Copy link
Collaborator

Alright, then I think it's fair to keep it to python 3.11, I also noticed that they recently dropped support for python 3.8.
Maybe we can change our minimum requirements to >=3.9 too.

@rodolfocarobene
Copy link
Author

Alright, then I think it's fair to keep it to python 3.11, I also noticed that they recently dropped support for python 3.8. Maybe we can change our minimum requirements to >=3.9 too.

I think it makes sense, python 3.8 is a bit outdated right now. I'm working on the updates :-)

@rodolfocarobene
Copy link
Author

Now the installation should work both for python3.9 and python3.11.
Some tests are failing locally, but I think it's just because I don't have the "right hardware" (api.QVMError: Could not communicate with QVM at http://127.0.0.1:5000)

@rodolfocarobene rodolfocarobene changed the title Add support to python 3.11-3.12 Add support to python 3.11 Jun 1, 2024
@rodolfocarobene
Copy link
Author

Fixing dependencies once again (maybe one of the last time? :-)). For the moment I limited the version of some libraries, while I think we should instead modify the code to support higher versions of those libraries, but maybe in another PR.

In particular I limited:

Locally the installation works fine, some tests of pyquil fail. In general because of connections, a couple for some other reason that I am having some trouble understanding:

from openqaoa.backends.qaoa_device import create_device
device = create_device(location="qcs", name="7q-noisy-qvm")

>>>ValueError: Invalid device location, Choose from: dict_keys(['local', 'azure', 'aws', 'ibmq'])

Also in the workflow the same error is being raised, is it a version thing or something else?

@KilianPoirier
Copy link
Collaborator

Nice! Let's see how the workflow runs this time.

I'm currently pushing some changes for the mitiq issue as you have noticed but other problems arose when fixing this, it may take a bit more time than expected. No worries though if the only error is this one on your side, I'll approve your PR and fix it myself.

Concerning the QVM issue it may be due to a problem with pyquil, if you don't have the qvm installed separately, the tests won't pass locally. This should be caught by the GH workflow.

@rodolfocarobene
Copy link
Author

There is still some problem with the pyquil tests...

@KilianPoirier
Copy link
Collaborator

KilianPoirier commented Jun 4, 2024

Hey @rodolfocarobene , the error seems to come from a faulty install of pyquil since the pyquil backends are not resolved in the list of backends in the tests.

I also tried to branch out from your changes and test locally but the make command doesn't resolve. Have you tried looking into this? The makefile only runs an install of the pip install -e ./openqaoa-*/.

Here is what happens when I try installing locally using your modified pyproject.toml
image

Maybe the first step to try and debug this would be to make changes one by one, first upgrading the version of python and related dependencies, making sure the tests pass both locally and on the GH workflow. Then, migrate all of the changes to the pyproject.toml. Let me know what you think about this, or if you have any other suggestion.

@rodolfocarobene
Copy link
Author

What version of python are you using? I don't see the see behavior... quite weird.
I'm gonna work on this, if I cannot do it I will revert to the setup.py and split this in half :-)

@rodolfocarobene
Copy link
Author

Hey, @KilianPoirier, I've added the explicit location of the packages (for some reason it was required for me, but I suppose it doesn't hurt).
While for the pyquil plugin I had written "braket" instead "pyquil" so it made sense that it wasn't working... it should be fixed

@KilianPoirier
Copy link
Collaborator

Good news, the tests seem to pass! Nice job fixing all of the issues
I will have a proper review of the code but it should be pretty quick at this point.

@rodolfocarobene
Copy link
Author

Sorry, I had forgot to check some stuff (mainly docs and workflows) that had some outdated stuff. At the same time, with the introduction of the pyproject.toml the _version.py and some other configuration files became obsolete, I should have updated everything in a reasonable manner.
Therefore, the last commit is touching many files, but is mostly redundant/not important things.

Maybe in the future could be convenient a script to update the version (both of python and the packages) in all the repository at once. :-)

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.

Mostly minor comments but I have a few questions concerning the versioning of the plugins.

Overall good job on the implementation! I think we're quite close to solving the issue :)

.github/workflows/test_pypi.yml Show resolved Hide resolved
scripts/push_package_pypi.sh Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
src/openqaoa-braket/README.md Outdated Show resolved Hide resolved
src/openqaoa-azure/pyproject.toml Show resolved Hide resolved
src/openqaoa-qiskit/README.md Outdated Show resolved Hide resolved
src/openqaoa-core/README.md Outdated Show resolved Hide resolved
src/openqaoa-core/README.md Outdated Show resolved Hide resolved
src/openqaoa-braket/README.md Outdated Show resolved Hide resolved
src/openqaoa-braket/README.md Outdated Show resolved Hide resolved
@KilianPoirier
Copy link
Collaborator

I just approved the PR and assigned you to the issue and bounty!
Good job tackling this issue and glad to count you as an openqaoa contributor :)

@rodolfocarobene
Copy link
Author

Thanks @KilianPoirier ! It was a pleasure for me :-) .
Maybe for the hackaton the issue should be closed? Since it seems to me that it still not "resolved" (link)

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 the package to support python >=3.11
2 participants