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

chore: python version to 3.11 (while supporting 3.10) #31503

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Dec 17, 2024

Bumping python to 3.11 as the default version, supporting 3.10, and deprecating 3.9 support

Currently in this PR I'm setting next to 3.11 and current to 3.11.11. This means when 3.11.12 shows up CI will run on two different versions within 3.11. Eventually we can make next point to 3.12 as lib support grows.

about 3.12

Seems 3.12 is not easy at the moment, hit some issues that pointed to upgrading dependencies, but hit some conflicting diamond dependencies as I was doing so. #31313 would help sort this out at a faster pace, without the bugs and slowness of pip-compile-multi

Closes #31516

@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Dec 17, 2024
Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

I've completed my review and didn't find any issues... but I did find this bear.

/  \.-"""-./  \
\    -   -    /
 |   o   o   |
 \  .-'''-.  /
  '-\__Y__/-'
     `---`

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

  • You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
  • You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
  • Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
  • Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
  • Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

  • Check out our docs on how you can make Korbit work best for you and your team.
  • Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings
Setting Value
Review Schedule Automatic excluding drafts
Max Issue Count 10
Automatic PR Descriptions
Issue Categories
Category Enabled
Naming
Database Operations
Documentation
Logging
Error Handling
Systems and Environment
Objects and Data Structures
Readability and Maintainability
Asynchronous Processing
Design Patterns
Third-Party Libraries
Performance
Security
Functionality

Feedback and Support

@mistercrunch mistercrunch changed the title chore: try and bump python version to 3.11 (while supporting 3.10 and 3.12) chore: try and bump python version to 3.11 (while supporting 3.10) Dec 18, 2024
@mistercrunch mistercrunch changed the title chore: try and bump python version to 3.11 (while supporting 3.10) chore: python version to 3.11 (while supporting 3.10) Dec 18, 2024
@villebro
Copy link
Member

Should we first merge #31313 before proceeding with this? Also, should we not wait until 3.12 is supported before moving the window of supported versions forward?

@michael-s-molina
Copy link
Member

deprecating 3.9 support

@mistercrunch There's a 5.0 proposal to drop support for 3.9. We should wait for the breaking window unless you remove the 3.9 deprecation of this PR's scope.

@mistercrunch mistercrunch added the v5.0 Label added by the release manager to track PRs to be included in the 5.0 branch label Dec 18, 2024
@mistercrunch
Copy link
Member Author

@michael-s-molina yup I'm taking this on as part of the 5.0 effort.

Should we first merge #31313

@villebro yes, that will help sorting things out. I'll push on that one to go first (pre 5.0)

should we not wait until 3.12 is supported ?

I don't think that's required, my take is we should support previous and current, and next is a nice to have as a placeholder to start getting things working. The next construct is useful to support the process of getting ready for it, but not needed to deprecate a previous version. Given the state of CI, and the fact that only some checks run on previous and next, we should strongly recommend using the current version, the next/previous are useful if/when we want to support more or while we're in the process of establishing or deprecating support.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

One main comment about the versions perhaps being inverted on current vs next. My main gripe with this PR is the fact that we're essentially running two 3.11 CI checks, which seems wasteful. As 3.12 really isn't yet supported, I think we should consider just disabling the CI step for next for now until we are actually able to test against it.

echo "PYTHON_VERSION=3.10" >> $GITHUB_ENV
echo "PYTHON_VERSION=3.11.11" >> $GITHUB_ENV
elif [ "${{ inputs.python-version }}" = "next" ]; then
# TODO: set to 3.12 when libs are ready - seemed hard on 12/17/24
echo "PYTHON_VERSION=3.11" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these reversed, i.e. current should point to 3.11 and next should point to 3.11.11?

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 thought 3.11.11 would be the pinned current, and 3.11 will eventually point to future as 3.11.12 and future version come out. As of now I believe 3.11 == 3.11.11

pyproject.toml Outdated
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
# "Programming Language :: Python :: 3.12", # not just yet ...
Copy link
Member

Choose a reason for hiding this comment

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

If 3.12 isn't supported yet, I'd just remove this for now.

Suggested change
# "Programming Language :: Python :: 3.12", # not just yet ...

@mistercrunch
Copy link
Member Author

@villebro I wish it was easier to disable next, but given it's marked as a required_status_checks in .asf.yml, it would require a preliminary PR removing the check, merged in master, rebasing this one, and then reenabling it eventually.

I guess we could decide that next isn't required now and in the future, more of an indicator on readiness on the next version. I kind of like the idea, except showing any kind of red on PR checks is unsettling/confusing to contributors...

@villebro
Copy link
Member

villebro commented Jan 7, 2025

I guess we could decide that next isn't required now and in the future, more of an indicator on readiness on the next version. I kind of like the idea, except showing any kind of red on PR checks is unsettling/confusing to contributors...

@mistercrunch I wonder if there's some way to downplay a CI check to yellow or something else to not make it look like a hard failure? I personally always get put off by failing checks, even if they're not required, and I suspect it will be even more confusing to a casual contributor who doesn't have context on this forthcoming work..

@mistercrunch
Copy link
Member Author

Screenshot 2025-01-07 at 10 01 44 AM

@villebro
Copy link
Member

villebro commented Jan 7, 2025

I googled it, and came across this: https://stackoverflow.com/questions/57687369/how-to-write-a-github-action-with-a-neutral-check-run-status. So I don't think it works.

I discovered through some old documentation that exit code 78 gives a neutral status for the step. But how do I give the entire job a neutral status?

Update: After testing, I discovered that exit code 78 does not affect the status of the step.

mistercrunch added a commit that referenced this pull request Jan 7, 2025
As discussed in
#31503 (comment),
let's make the CI checks against the `next` version of python optional.

This PR needs to get through as policies that live on `master` in
`.asf.yml` are applied to ongoing PRs. After this is merged, I'll add
the solution described in #31503,
that should mark the related failing jobs as `neutral`, which should
show as grey in the GitHub UI.
@mistercrunch
Copy link
Member Author

Here -> #31740

@mistercrunch
Copy link
Member Author

Mmmmh, yeah goes back to 2019 so my hopes aren't up on this one, I couldn't find a decent workaround.Maybe it's a good think to make optional as in #31740 though?

@mistercrunch
Copy link
Member Author

Few options:

  • leave this PR as is, and assume for now that next means the latest 3.11.x
  • set 3.12 as next and live with the red, deal with confusion for contributors (I think that's not great)
  • set 3.12 as next and force a green status, have to dig in logs to see if it's actually working
  • set 3.12 as next and keep pushing on this PR until we get it working

@villebro
Copy link
Member

villebro commented Jan 7, 2025

@mistercrunch what was missing for 3.12 support? Numpy wheels? Let's give it one more go, and if it just doesn't work out let's settle for one of the compromises listed above..

@mistercrunch
Copy link
Member Author

Ok, added a commit bringing in 3.12 in the mix, let's see what fails.

@mistercrunch
Copy link
Member Author

mistercrunch commented Jan 8, 2025

Ok I pushed by bumping a bunch of libraries here, and ultimately got to a place where next hung on setup-backend while installing python deps. Turns out there were no wheels out for pandas/numpy for the version we had pinned. So I bumped it beyond the range that was specified in our pyproject.toml (there was no note around why it had an upper bound) and uncovered some other issue we have with this newer version pandas.

Certainly a good thing to unpin and push through, but might require some work or waiting for newer version of libs to move forward.

@mistercrunch
Copy link
Member Author

mistercrunch commented Jan 8, 2025

The new pandas df.to_sql seem doesn't support sqlalchemy datatypes or at least not the same way -> pandas-dev/pandas#57049 .

Seems the issue is with pandas>=2.2.0 so applying an upper bound and retrying. Sometimes it's good to let things library bake a little, though worth trying to see how far we get.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code preset-io size/L v5.0 Label added by the release manager to track PRs to be included in the 5.0 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

73 - Drop support for Python 3.9
4 participants