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

Pin Python version to 3.11 #1118

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Jan 12, 2024

Don't we pin Python already?

We don't, or not strictly enough. Currently we have

ods-ci/pyproject.toml

Lines 9 to 10 in afa1de6

[tool.poetry.dependencies]
python = "^3.8.1"

which means "use any Python >=3.8.1, <4.0.0".

Why pin Python version?

Because we can put everybody on the same version.
This way,
we make it less likely different people will hit different infra issues.

Why use Python 3.11?

It is well established,
binary wheels exist for it in PyPI,
it is available on UBI 8 and 9, and on Fedora starting with Fedora 37.

Why say python = ">=3.11.5, <3.12.0"?

The syntax uses basic math instead of tildes and carets,
which not everybody might've learned what they mean.
That makes the requirement easier to read and understand.

Will this break CI infra?

Most likely, which is why this PR exists, to try things out.

All things in order, everything is very good, 🌈 🦄 🌇

@jiridanek jiridanek changed the title Jd python11 Pin Python version to 3.11 Jan 12, 2024
@jiridanek jiridanek added needs testing Needs to be tested in Jenkins utils Enhancements in scripts and CI/CD (PR will be listed in release-notes) labels Jan 12, 2024
@jiridanek jiridanek self-assigned this Jan 12, 2024
@jiridanek
Copy link
Member Author

And, it fails CI as expected, rhods-ci-pr-test/2308

21:19:50  The currently activated Python version 3.9.17 is not supported by the project (>=3.11.5, <3.12.0).
21:19:50  Trying to find and use a compatible version. 
21:19:50  
21:19:50  Poetry was unable to find a compatible version. If you have one, you can explicitly use it via the "env use" command.
21:19:51  ods_ci/run_robot_test.sh: line 320: /bin/activate: No such file or directory

So, I'll have to update Python in CI. Then, I'll announce that contributors should do dnf install python3.11; poetry env use /usr/bin/python3.11, and after some time it should be safe to merge this change.

@jiridanek
Copy link
Member Author

jiridanek commented Jan 13, 2024

Here is a CI image update that needs to be merged first, ods/rhods-qe-tools/-/merge_requests/19

Copy link
Contributor

github-actions bot commented Jan 13, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
401 0 0 401 100

@jiridanek
Copy link
Member Author

Also need Python 3.11 in the Dockerfile here, https://github.com/red-hat-data-services/ods-ci/blob/master/ods_ci/build/Dockerfile

@jiridanek jiridanek force-pushed the jd_python11 branch 2 times, most recently from f34449e to d80de04 Compare January 16, 2024 11:33
ods_ci/run_robot_test.sh Outdated Show resolved Hide resolved
@jiridanek
Copy link
Member Author

jiridanek commented Jan 16, 2024

CI: rhods-ci-pr-test/2323 (smokes do run)

New CI runs:

  • job/rhods/job/rhods-ci-pr-test/2470/console (--dryrun)
  • job/rhods/job/rhods-ci-pr-test/247/console (-i smoke)

both jobs currently in progress

@jiridanek jiridanek added the verified This PR has been tested with Jenkins label Jan 16, 2024
@jiridanek jiridanek marked this pull request as ready for review January 16, 2024 12:17
@jiridanek
Copy link
Member Author

Here are "migration" steps. After this is merged, your existing ods-ci checkout will start failing on various poetry commands

Current Python version (3.9.18) is not allowed by the project (>=3.11.5, <3.12.0).
Please change python executable via the "env use" command.

So you do

dnf install python3.11 python3.11-devel
poetry env use /usr/bin/python3.11
poetry install --sync

and you are good.

@jgarciao jgarciao added the do not merge Do not merge this yet please label Jan 16, 2024
@jgarciao
Copy link
Contributor

I've added the "do not merge" label until we have an agreement on slack about merging this

@jiridanek jiridanek force-pushed the jd_python11 branch 3 times, most recently from 30589fb to 3a51201 Compare February 1, 2024 07:13
## Why pin the Python version?

Because we _can_ put everybody on the same version.
This way,
we make it less likely different people will hit different issues.

## Why use Python 3.11?

It is well established,
binary wheels exist for it in PyPI,
it is available on UBI 8 and 9, and on Fedora starting with Fedora 37.

## Why say `python = ">=3.11.5, <3.12.0"`?

The syntax uses basic math instead of tildes and carets,
which not everybody might've learned what they mean.
That makes the requirement easier to read and understand.

Signed-off-by: Jiri Daněk <jdanek@redhat.com>
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jiridanek
Copy link
Member Author

This should be all the needed changes. Now onto rechecking it in CI.

@jiridanek jiridanek removed verified This PR has been tested with Jenkins do not merge Do not merge this yet please labels Feb 13, 2024
@jiridanek
Copy link
Member Author

jiridanek commented Feb 13, 2024

There are two todo's that I'll do when this is merged

  • go to gitlab.cee and in ods/jenkins change the pr job so that it does not archive .venv directory
  • go to gitlab.cee and in ods/rhods-qe-tools make python3.11 be the default python for poetry to save CI time
    • ods/rhods-qe-tools/-/merge_requests/25

@@ -24,16 +24,16 @@ def __init__(self):
self._message = MIMEMultipart()

def prepare_payload(
self, text: str = "", attachments: Optional[List[Any]] = None
self, text: str = "", attachments: list[Any] | None = None
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

linter wants this when you have a py3.11 project

at some point they made list[...] be an alias for typing.List[...], so that's a cleanup it wants

not sure about why it dislikes Optional, that should be fine imo, and | None is not a clear win in my eyes

@jiridanek
Copy link
Member Author

job/rhods/job/rhods-ci-pr-test/2487, still looks good

@jiridanek jiridanek merged commit 867a617 into red-hat-data-services:master Feb 13, 2024
11 checks passed
@jiridanek jiridanek deleted the jd_python11 branch February 13, 2024 21:55
@jiridanek jiridanek added verified This PR has been tested with Jenkins and removed needs testing Needs to be tested in Jenkins labels Feb 16, 2024
jstourac added a commit to jstourac/ods-ci that referenced this pull request Feb 18, 2024
This is a followup of [1,2] and fixes image build creation after we
moved from Python 3.9 to Python 3.11

On top of that, it also removes dnf cache after all installation is
done. There is no change in the sice of resulting image but it's a
standard step to do in the Dockerfiles in general.

[1] red-hat-data-services#1118
[2] 867a617
jstourac added a commit that referenced this pull request Feb 19, 2024
This is a followup of [1,2] and fixes image build creation after we
moved from Python 3.9 to Python 3.11

On top of that, it also removes dnf cache after all installation is
done. There is no change in the sice of resulting image but it's a
standard step to do in the Dockerfiles in general.

[1] #1118
[2] 867a617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
utils Enhancements in scripts and CI/CD (PR will be listed in release-notes) verified This PR has been tested with Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants