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

Python 3.11 is only supported python version #1420

Merged
merged 8 commits into from
Jul 31, 2024
Merged

Conversation

psschwei
Copy link
Collaborator

@psschwei psschwei commented Jul 23, 2024

Summary

Only build and test against python 3.11

Details and comments

Removes changes made in #1034

@psschwei
Copy link
Collaborator Author

We have a whole mechanism to allow users to choose their Python version that will also need to be ripped out

@Tansito
Copy link
Member

Tansito commented Jul 24, 2024

We have a whole mechanism to allow users to choose their Python version that will also need to be ripped out

Good catch, thinking in this feature... Probably we could remove the config by now from the client and in the api use always the same image node_image = settings.RAY_NODE_IMAGE. With the providers I don't think it has sense to allow this kind of configuration from the user, wdyt? 🤔

@Tansito Tansito closed this Jul 24, 2024
@Tansito Tansito reopened this Jul 24, 2024
@Tansito
Copy link
Member

Tansito commented Jul 24, 2024

Sorry Paul, missclick 😅

@psschwei
Copy link
Collaborator Author

test failures should be resolved by 1b3e8ee , so let's get the UBI PR merged first then return to this one

@psschwei
Copy link
Collaborator Author

psschwei commented Jul 24, 2024

let's get the UBI PR merged first then return to this one

UBI PR merged, so let's see if we're in business 🤞

@psschwei
Copy link
Collaborator Author

This also drops the number of gh actions we run to 10 (down from 18)

@Tansito Tansito requested review from akihikokuroda and Tansito July 25, 2024 12:50
@akihikokuroda
Copy link
Collaborator

With the providers I don't think it has sense to allow this kind of configuration from the user, wdyt?

It maybe too late but it may be useful for the preparation for the next python version update. The provider can test their function with the new python version in advance.

@@ -37,7 +37,7 @@ jobs:
sed -i "s/ray-node:${OLDNUM}/ray-node:${NEWNUM}/" charts/qiskit-serverless/charts/gateway/values.yaml
sed -i "s/qiskit-serverless\/proxy:${OLDNUM}/qiskit-serverless\/proxy:${NEWNUM}/" charts/qiskit-serverless/charts/gateway/values.yaml
sed -i "s/tag: \"${OLDNUM}\"/tag: \"${NEWNUM}\"/" charts/qiskit-serverless/values.yaml
sed -i "s/tag: \"${OLDNUM}-py39\"/tag: \"${NEWNUM}-py39\"/" charts/qiskit-serverless/values.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nope

@psschwei
Copy link
Collaborator Author

The provider can test their function with the new python version in advance.

Good point. Maybe when the project stabilizes we'll need to start cutting release candidates (similar to what qiskit does) for users to test new versions?

PYTHON_VERSIONS = [
(PYTHON_V3_8, "Version 3.8"),
(PYTHON_V3_9, "Version 3.9"),
(PYTHON_V3_10, "Version 3.10"),
(PYTHON_V3_11, "Version 3.11"),
]
python_version = models.CharField(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we take this python_version out, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure if we needed that for legacy data, but can try taking it out.

Copy link
Member

Choose a reason for hiding this comment

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

Not needed. In fact we didn't move data from our old database to the new one in the migration what it simplifies even more this.

Copy link
Member

Choose a reason for hiding this comment

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

What we are going to need to test it's if somethings breaks if old data exists in the database. I can help with that test later.

@Tansito
Copy link
Member

Tansito commented Jul 25, 2024

The provider can test their function with the new python version in advance.

Yep, I didn't think about this but it's a good point for the future, Aki. I agree, let's do the next:

  • Let's keep the configuration by now (I liked the idea of the releases candidates)
  • Let's remove python 3.9 and 3.10 support only
  • Let's maintain the -py311

Thank you guys! 🙏

@Tansito Tansito mentioned this pull request Jul 25, 2024
@psschwei
Copy link
Collaborator Author

I'd still drop the python version. Testing against a release candidate shouldn't be exposed publicly, instead we'd set it up on a staging environment and run tests against that explicitly. Plus, if we're making this available for providers only then we'd need to wrap some kind of permissions around that field so that only providers could call it. Much simpler to just drop it completely.

@Tansito
Copy link
Member

Tansito commented Jul 26, 2024

@psschwei changes look good, if you remove python_version from the model I can help to test it 👍

@Tansito
Copy link
Member

Tansito commented Jul 26, 2024

And after reading you correctly, I agree. Let's see in future discussions how can we improve that way to facilitate the access to the providers to these nightlies versions.

@psschwei
Copy link
Collaborator Author

@akihikokuroda @Tansito I think I've got the last bits of python_version out now, so this one is ready for another look

Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

@psschwei can we regenerate the migrations to have only one migration? For the rest of the things I see everything fine. I added @IceKhan13 too to the PR for his awareness 😄

I continue discussing with myself if this approach has sense in our context or not. But I don't see a way where we can support more than one python version without dying in the effort at this moment.

@Tansito Tansito self-requested a review July 30, 2024 18:56
Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

LGTM Paul, thank you! I would like to know if @akihikokuroda and @IceKhan13 agrees with the change too

@Tansito
Copy link
Member

Tansito commented Jul 30, 2024

Btw @psschwei I think I added you some conflicts due to my merge 😅

Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
psschwei added 7 commits July 30, 2024 15:43
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
Signed-off-by: Paul S. Schweigert <paul@paulschweigert.com>
@Tansito
Copy link
Member

Tansito commented Jul 31, 2024

@psschwei feel free to merge it so I can update #1416 and we merge both PRs today 💪

@psschwei psschwei merged commit 000dc60 into Qiskit:main Jul 31, 2024
10 checks passed
@psschwei psschwei deleted the py311-only branch July 31, 2024 16:19
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.

3 participants