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(package-managers/python)!: Support Python 3.11 #7598

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

fviernau
Copy link
Member

@fviernau fviernau commented Sep 28, 2023

Add "3.11" to the list of python versions which ORT allows to pass to
the -p option of python-inspector. This enables the successful
analysis of Python projects which require Python 3.11 via both, Pip
and Poetry. In particular, it does not seem to require upgrading
the Python installation to version 3.11.

The change is breaking, because also the default Python version is
changed to 3.11. For analyzing Python projects targeting 3.10 the
Python version as of now must be explicitly specified.

Note: The reason why the binary artifact URL of MarkupSafe became empty
in the expected result for the test is simply, because there is no
"Built Distribution", but only a "Source Distribution", see [1].
Furthermore, it's still correct to list MarkupSafe as dependency,
because it specifies >= 3.6 as Python version constraint, mich
matches version 3.11.

[1] https://pypi.org/project/MarkupSafe/2.0.1/#files

Fixes #7476.


Note: This PR conflicts with [1], I didn't intentionally repeat the work from [1], but have been debugging analyzing pyproject.toml without being aware [1] is related, and this is the result from it. So, I believe this PR minimizes the changes for supporting 3.11.

[1] #7533

@fviernau fviernau requested a review from a team as a code owner September 28, 2023 11:06
@fviernau fviernau force-pushed the python311-support-minimal branch from e195fe8 to 7b03315 Compare September 28, 2023 11:07
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (e3bbcdb) 68.03% compared to head (2f56af6) 68.03%.

❗ Current head 2f56af6 differs from pull request most recent head ecac1d4. Consider uploading reports for the commit ecac1d4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7598   +/-   ##
=========================================
  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-docker 69.40% <ø> (ø)
funTest-non-docker 36.44% <ø> (ø)
test 35.53% <ø> (ø)

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.

@fviernau fviernau force-pushed the python311-support-minimal branch 2 times, most recently from 1e7e428 to ebd62e7 Compare September 28, 2023 11:45
@@ -103,10 +103,10 @@ packages:
description: "Safely add untrusted strings to HTML/XML markup."
homepage_url: "https://palletsprojects.com/p/markupsafe/"
binary_artifact:
url: "https://files.pythonhosted.org/packages/53/e8/601efa63c4058311a8bda7984a2fe554b9da574044967d7aee253661ee46/MarkupSafe-2.0.1-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_12_x86_64.manylinux2010_x86_64.whl"
url: ""
Copy link
Member

Choose a reason for hiding this comment

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

This diff is unexpected to me even after reading the commit message again.

Copy link
Member Author

@fviernau fviernau Sep 28, 2023

Choose a reason for hiding this comment

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

Why exactly is it unexpected to you? ...and what do you suggest?

Maybe as a note: If I adjust test to pass explicitly python version 3.10 as option, then there is not diff here.

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.

I would have expected either no change to the artifact's metadata at all, or a change that reflects the 3.10 -> 3.11 version upgrade, e.g. by pointing to a new artifact that does not contain "cp310" but "cp311" as part of its URL. Anyway, I do not understand why this being empty should be correct. I suggest explaining in the commit message why there is no according Python 3.11 artifact.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't figured this out myself yet. Would you be ok, if we pinned the test to version 3.10 for now and leave this open for future investigation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've investigated a bit further and updated the commit message with additional info.

Copy link
Member Author

@fviernau fviernau Sep 28, 2023

Choose a reason for hiding this comment

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

If I upgrade python on docker to 3.11, then the test diff seems to be empty.
So, this is the way to go then, @sschuberth ?

Note: I believe meanwhile, that there is a general problem with the approach of combining poetry export with python-inspector, in case the python version passed to python-inspector differs from the Python version installed. Trying this out now by making a separate PR for updating python, to see if it breaks the poetry fun test.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, https://pypi.org/project/MarkupSafe/2.0.1/#files shows that there's indeed no pre-built binary distribution for Python 3.11, so it's probably fine for the binary artifact to become empty, as the source artifact is still there.

As a side note, https://pypi.org/project/MarkupSafe/2.1.2/#files starts to have 3.11 binary artifacts, so if the project would upgrade to MarkupSafe 2.1.2, this should not become empty anymore.

Copy link
Member Author

@fviernau fviernau Sep 29, 2023

Choose a reason for hiding this comment

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

BTW, https://pypi.org/project/MarkupSafe/2.0.1/#files shows that there's indeed no pre-built binary distribution for Python 3.11, so it's probably fine for the binary artifact to become empty, as the source artifact is still there.

This is almost exactly what the (updated) commit message now says.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I upgrade python on docker to 3.11, then the test diff seems to be empty.
So, this is the way to go then, @sschuberth ?

This turned out to be wrong. A mistake during debugging.

@mawl
Copy link

mawl commented Sep 28, 2023

If this is sufficient to support analysis of pyproject.toml files with python 3.11 as requirement I'm happy and we can close #7533 - Thanks :)

@fviernau fviernau force-pushed the python311-support-minimal branch from ebd62e7 to b87f3d1 Compare September 28, 2023 14:55
@fviernau
Copy link
Member Author

If this is sufficient to support analysis of pyproject.toml files with python 3.11 as requirement I'm happy and we can close #7533 - Thanks :)

Let's see. There is this diff in the test, which some solution for is needed. Did you run into the same @mawl ?

@mawl
Copy link

mawl commented Sep 29, 2023

Let's see. There is this diff in the test, which some solution for is needed. Did you run into the same @mawl ?

@fviernau: I saw the diff of the PoetryFunTest in my PR but haven't tried to fix it.

@fviernau fviernau force-pushed the python311-support-minimal branch 2 times, most recently from 660e654 to 2f56af6 Compare September 29, 2023 07:30
The file has been deleted an then re-created with `poetry lock`.
Do so, to be on the safe side regarding running into issues with a lock
file created from with an older Poetry version.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau requested a review from sschuberth September 29, 2023 13:09
Add "3.11" to the list of python versions which ORT allows to pass to
the `-p` option of `python-inspector`. This enables the successful
analysis of Python projects which require Python 3.11 via both, `Pip`
and `Poetry`. In particular, it does not seem to require upgrading
the Python installation to version 3.11.

The change is breaking, because also the default Python version is
changed to 3.11. For analyzing Python projects targeting 3.10 the
Python version as of now must be explicitly specified.

Note: The reason why the binary artifact URL of `MarkupSafe` became empty
in the expected result for the test is simply, because there is no
"Built Distribution", but only a "Source Distribution", see [1].
Furthermore, it's still correct to list `MarkupSafe` as dependency,
because it specifies `>= 3.6` as Python version constraint, which
matches version 3.11.

[1] https://pypi.org/project/MarkupSafe/2.0.1/#files

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau force-pushed the python311-support-minimal branch from 2f56af6 to ecac1d4 Compare September 29, 2023 13:10
[[package]]
name = "appdirs"
version = "1.4.3"
version = "1.4.4"
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this commit, but since you were investigating things, do you know why most of the packages in this lock file do not show up in the ORT dependency tree at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated to this commit, but since you were investigating things, do you know why most of the packages in this lock file do not show up in the ORT dependency tree at all?

I remind touching this thing and I have some guess, I'll investigate this next week. It should be relatively easy now, as I'm into it currently.

@fviernau fviernau enabled auto-merge (rebase) September 29, 2023 13:24
@fviernau fviernau merged commit 74f14a6 into main Sep 29, 2023
20 checks passed
@fviernau fviernau deleted the python311-support-minimal branch September 29, 2023 13:57
@fviernau fviernau added release On the topic of making releases release notes and removed release On the topic of making releases labels Oct 5, 2023
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.

Python 3.11 Support
3 participants