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

build(python): Support Python 3.12 #12094

Merged
merged 7 commits into from
Nov 16, 2023
Merged

build(python): Support Python 3.12 #12094

merged 7 commits into from
Nov 16, 2023

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Oct 29, 2023

Closes #11348

Changes

  • Update Python version in the CI.

@stinodego stinodego changed the title build: Support Python 3.12 build(python): Support Python 3.12 Oct 29, 2023
@github-actions github-actions bot added build Changes that affect the build system or external dependencies python Related to Python Polars rust Related to Rust Polars labels Oct 29, 2023
@stinodego
Copy link
Member Author

@MarcoGorelli @alexander-beedie any idea why the bytecode parser used for the inefficient apply warning fails a bunch of tests on Python 3.12?
https://github.com/pola-rs/polars/actions/runs/6686165281/job/18165214724?pr=12094

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Oct 30, 2023

@MarcoGorelli @alexander-beedie any idea why the bytecode parser used for the inefficient apply warning fails a bunch of tests on Python 3.12?

Probably some new/reworked opcode specializations in 3.12 as part of the "faster cpython" initiative; I can take a look and account for them 👍

@stinodego stinodego added this to the 1.0.0 milestone Nov 7, 2023
@stinodego stinodego added the highlight Highlight this PR in the changelog label Nov 8, 2023
@stinodego stinodego force-pushed the python-3.12 branch 3 times, most recently from ecb7d57 to 17f89fa Compare November 8, 2023 10:28
@stinodego
Copy link
Member Author

@alexander-beedie Only bytecode parser failing on Python 3.12 now... all yours!

@alexander-beedie
Copy link
Collaborator

@alexander-beedie Only bytecode parser failing on Python 3.12 now... all yours!

Done! #12348 ✌️😎

@stinodego
Copy link
Member Author

@alexander-beedie I am consistently getting an import time regression on ubuntu / Python 3.12 😕 Any idea what could have triggered this? Here's the timings:
https://github.com/pola-rs/polars/actions/runs/6827502341/job/18596392679?pr=12094

@alexander-beedie
Copy link
Collaborator

@alexander-beedie I am consistently getting an import time regression on ubuntu / Python 3.12 😕 Any idea what could have triggered this? Here's the timings: https://github.com/pola-rs/polars/actions/runs/6827502341/job/18596392679?pr=12094

Hmm... will try to take a look tomorrow 👌

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Nov 15, 2023

Hmm... will try to take a look tomorrow 👌

And by "tomorrow" I apparently mean two days later 😅 But... I think I have an explanation and will be pushing an update shortly! The good news is that imports have not become meaningfully slower; it appears to be an import-test artefact that I can address to get more consistent/reliable results.

@stinodego
Copy link
Member Author

@alexander-beedie The test still fails even with your latest update 😞

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Nov 15, 2023

@alexander-beedie The test still fails even with your latest update 😞

Maddening... that test is absolutely rock-solid for me now. I'll instrument it some more, but maybe we'll need to adjust the import timing threshold on a per-architecture basis, hmm. Will look at the test runner logs for clues 🤔

From my home machine (an admittedly fast M2 Max) -

Import times achieved over 10 tries:
Min => 38,665μs, Max => 65,463μs)

(0) 38,665μs
(1) 45,161μs
(2) 61,242μs
(3) 58,141μs
(4) 56,683μs
(5) 63,325μs
(6) 69,749μs
(7) 65,463μs
(8) 62,225μs
(9) 55,672μs

Note that those are debug build timings; I see release build timings as low as 28,171μs 🚀 (< 0.03 secs)

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Nov 15, 2023

Instrumented a little and increased the number of allowed tries from three to ten 👀 Results are clear enough; the test now looks pretty stable, but import timings on the 3.12 CI (hardware?) really ARE consistently slow...

Import times achieved over 10 tries:
Min => 244,554μs, Max => 320,321μs)

(0) 268,935μs
(1) 256,196μs
(2) 262,766μs
(3) 264,437μs
(4) 244,554μs
(5) 320,321μs
(6) 269,662μs
(7) 279,400μs
(8) 300,972μs
(9) 261,502μs

Will switch to 3.12 locally and see if there's a drop-off (my own timings were 3.11 on Mac - can also try 3.12 on my NAS/Ubuntu box and see how that looks 💭)

@stinodego
Copy link
Member Author

Will switch to 3.12 locally

It has to be related to Python 3.12, possibly in combination with the CI runners. After all, the Python 3.8 Ubuntu run passes, as well as the Python 3.12 Windows run 🤔

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Nov 15, 2023

It has to be related to Python 3.12, possibly in combination with the CI runners. After all, the Python 3.8 Ubuntu run passes, as well as the Python 3.12 Windows run 🤔

Might have a lead; apparently the performance of coverage has cratered in 3.12 in some cases. The two different machines/architectures I've tried on 3.12 both seem fine when running the import test directly, but that doesn't cause coverage to run - whereas running under CI does...

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Nov 15, 2023

Yup, looks like that's it; was finally able to replicate the slow import timings under 3.12 on the virtualised Ubuntu install on my NAS; enabling coverage causes them to fall off a cliff (eg: 2-3x slower than expected). The same does not seems to happen on macOS (Apple Silicon) or Windows.

Looks related to these issues: python/cpython#107841 and nedbat/coveragepy#1665.
Disabling the coverage flag -cov for 3.12 Ubuntu CI (until this is resolved) should fix the test failure 🤔

@stinodego stinodego force-pushed the python-3.12 branch 2 times, most recently from f97a353 to 2e3d91e Compare November 16, 2023 07:05
@stinodego stinodego merged commit f793bcb into main Nov 16, 2023
18 checks passed
@stinodego stinodego deleted the python-3.12 branch November 16, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Changes that affect the build system or external dependencies highlight Highlight this PR in the changelog python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Python 3.12
2 participants