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

feat(python): #7476 - Support Python 3.11 #7533

Closed
wants to merge 4 commits into from

Conversation

mawl
Copy link

@mawl mawl commented Sep 19, 2023

Fix issue #7476 - Support Python 3.11

Python 3.11 is supported by python-inspector already: aboutcode-org/python-inspector@8c6ee63

As it is my first PR in your project, feel free to correct it and get me on the right track.

@mawl mawl requested a review from a team as a code owner September 19, 2023 09:36
@mawl mawl changed the title Issue #7476 fix: #7476 - Support python 3.11 Sep 19, 2023
@mawl mawl changed the title fix: #7476 - Support python 3.11 feat(python): #7476 - Support python 3.11 Sep 19, 2023
Copy link
Member

@sschuberth sschuberth left a comment

Choose a reason for hiding this comment

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

Please rewrite commit messages to adhere to Conventional Commits, and capitalize proper nouns like "Python".

Dockerfile Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (49bf674) 68.03% compared to head (f2e8e91) 68.03%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7533   +/-   ##
=========================================
  Coverage     68.03%   68.03%           
  Complexity     2022     2022           
=========================================
  Files           344      344           
  Lines         16723    16723           
  Branches       2370     2370           
=========================================
  Hits          11377    11377           
  Misses         4363     4363           
  Partials        983      983           
Flag Coverage Δ
funTest-non-docker 36.44% <ø> (ø)
test 35.53% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mawl mawl force-pushed the Issue-7476 branch 3 times, most recently from 29476de to da01f67 Compare September 19, 2023 10:55
@sschuberth sschuberth requested a review from a team September 19, 2023 11:03
@mawl mawl force-pushed the Issue-7476 branch 2 times, most recently from 3383d8e to 4c18f88 Compare September 19, 2023 13:46
@mawl mawl changed the title feat(python): #7476 - Support python 3.11 feat(python): #7476 - Support Python 3.11 Sep 19, 2023
@mawl mawl force-pushed the Issue-7476 branch 6 times, most recently from ced1553 to cc95613 Compare September 26, 2023 06:05
@mawl
Copy link
Author

mawl commented Sep 26, 2023

@sschuberth: Thanks for your support. I have squashed the commits and upgraded the relevant tools (python, pyenv and poetry). Tests are green now. Is there anything else I can do or can we merge it now?

@sschuberth
Copy link
Member

@mawl by now the commit diff doesn't seem to match the commit message anymore, as only the Poetry version is bumped.

@mawl
Copy link
Author

mawl commented Sep 26, 2023

@mawl by now the commit diff doesn't seem to match the commit message anymore, as only the Poetry version is bumped.

I squashed too much and the changes are gone. I'll take a look.

@mawl
Copy link
Author

mawl commented Sep 26, 2023

@mawl by now the commit diff doesn't seem to match the commit message anymore, as only the Poetry version is bumped.

Changes are back now.

Currently Python 3.11 isn't in the list of available Python versions.
This commit extends the list of versions and sets 3.11 as default version.

+ Update versions of Python tools:

* Python 3.10.8 to 3.11.4
* Pyenv v2.3.7 to v2.3.27 to have Python 3.11.4 environment available
* Poetry 1.2.2 to 1.6.1
  to fix oss-review-toolkit#7333

Signed-off-by: mawl <[email protected]>
@sschuberth sschuberth dismissed their stale review September 26, 2023 06:30

Comments addressed.

@sschuberth
Copy link
Member

I'm basically ok with the changes, but I'm not in a position to judge whether this would break things for people at Bosch or EPAM. So I'd prefer @mnonnenmacher / @MarcelBochtler and @fviernau to have a look.

Dockerfile Outdated
@@ -138,7 +138,7 @@ RUN curl -kSs https://pyenv.run | bash \
ARG CONAN_VERSION=1.57.0
ARG PYTHON_INSPECTOR_VERSION=0.9.8
ARG PYTHON_PIPENV_VERSION=2022.9.24
ARG PYTHON_POETRY_VERSION=1.2.2
ARG PYTHON_POETRY_VERSION=1.6.1
Copy link
Member

Choose a reason for hiding this comment

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

Please upgrade the Poetry version in Dockerfile-legacy as well. Dockerfile-legacy is used by CI, maybe that is related to the failing tests.

Copy link
Author

Choose a reason for hiding this comment

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

@mnonnenmacher: I hope you are fine with changing python-inspector to 0.9.8 as well, as this will resolve: #7333 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yes please, versions should in general stay in sync. Installing Python 3.11 would be more difficult with the legacy file, so I think it's fine to stay with the default 3.10 there. python-inspector should still support 3.11 as long as no setup.py file which depends on 3.11 is involved.

@mnonnenmacher
Copy link
Member

I'm basically ok with the changes, but I'm not in a position to judge whether this would break things for people at Bosch or EPAM. So I'd prefer @mnonnenmacher / @MarcelBochtler and @fviernau to have a look.

We use the Dockerfile-legacy so the Python version upgrade would not affect us. Changing the default version used by python-inspector should be fine as it is easy to configure the version to be used.

Update Dockerfile-legacy

* python-inspector from 0.9.6 to 0.9.8
* poetry from 1.2.2 to 1.6.1

Both updates are necessary to fix oss-review-toolkit#7333 and the CI pipeline

Signed-off-by: mawl <[email protected]>
@heliocastro
Copy link
Contributor

With the new docker merged, all version changes should be done in one single place, .versions
So both Docker, Docker extended and the local docker_build script will know how to build or not the modified image.

@heliocastro
Copy link
Contributor

Please modify the .versions file as well

Update Python from 3.10.13 to 3.11.4
Update Pyenv from v2.3.25 to v2.3.27

Signed-off-by: mawl <[email protected]>
Copy link
Contributor

@heliocastro heliocastro 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

Copy link
Member

@sschuberth sschuberth left a comment

Choose a reason for hiding this comment

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

We do not allow merge commits in PRs.

@@ -126,8 +126,8 @@ RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \
tk-dev \
&& sudo rm -rf /var/lib/apt/lists/*

ARG PYTHON_VERSION=3.10.8
ARG PYENV_GIT_TAG=v2.3.7
ARG PYTHON_VERSION=3.11.4
Copy link
Member

Choose a reason for hiding this comment

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

to fix #7333

It's confusing to refer to that issue as it's already closed.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why upgrading python is needed in order to support 3.11?

In particular, would it suffice to stick to 3.10 (not saying we should) and only pass -p 3.11 to python inspector by default ?

@@ -33,7 +33,7 @@ ARG ORT_VERSION="DOCKER-SNAPSHOT"
ARG NUGET_INSPECTOR_VERSION=0.9.12

# Set this to the Python Inspector version to use.
ARG PYTHON_INSPECTOR_VERSION="0.9.6"
ARG PYTHON_INSPECTOR_VERSION="0.9.8"
Copy link
Member

@sschuberth sschuberth Sep 28, 2023

Choose a reason for hiding this comment

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

Both updates are necessary to fix #7333 and the CI pipeline

Again, it's confusing to refer to that issue as it's already closed, and the previous commit already was said to fix it, too.

@mawl
Copy link
Author

mawl commented Sep 29, 2023

I think we should stop the work here as more happens in #7598 - I will close this PR for now.

@mawl mawl closed this Sep 29, 2023
@mawl mawl deleted the Issue-7476 branch September 29, 2023 06:52
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.

5 participants