-
Notifications
You must be signed in to change notification settings - Fork 133
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: Enable support and testing for python 3.11 #512
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
Is there a particular reason for the upper bound on the python version? This library doesn't seem particularly likely to be broken by new Python versions. |
There's no reason that I know of - I was following the established convention. I'm happy to remove it if a reviewer believes that we're better off without an upper bound. |
Please, review and aprove this! |
@tswast @parthea @chalmerlowe Can you merge it? |
I will look at this later today. We did a similar change to Python-BIgquery just recently, so this makes sense. |
@chalmerlowe Have you had a chance to review? I'm happy to make changes that you believe are needed. |
@tswast @parthea @chalmerlowe @prash-mi Is there anything I can do to facilitate this PR getting reviewed? It has been waiting on a review for over a month and I anticipate this being a pretty straightforward review. |
@@ -51,13 +51,12 @@ | |||
"tests", | |||
"alembic", | |||
], | |||
"3.10": [ | |||
"3.11": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are our guidelines on what goes into the Unit Test Extras?
Are those always used solely in conjunction with the earliest and latest Python Versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely lowest so that we make sure the lower dependency bounds we have in setup.py
and our contraints files are correct. Latest SGTM so long as the extra dependencies have been released. In the past we'd update before pyarrow had cut a release, but this doesn't appear to be the case right now.
@@ -0,0 +1,40 @@ | |||
# Format: //devtools/kokoro/config/proto/build.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these files typically autogenerated OR do we add the manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect that these are autogenerated. However, I tried adding them manually as we had a test failure relating to Kokoro: https://source.cloud.google.com/results/invocations/b41e3d1e-1e73-454d-a24a-d1a790c7b129/targets/cloud-devrel%2Fclient-libraries%2Fpython%2Fgoogleapis%2Fpython-bigquery-sqlalchemy%2Fpresubmit%2Fpresubmit/log
I believe that the failure was the result of Session unit-3.11 failed: Python interpreter 3.11 not found.
and I was experimenting whether adding these files resolved that issue. If it doesn't I'll go ahead and remove those files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh shoot - it appears that I can't find that out the results of that experiment without adding the kokoro:run
label, which I'm unable to do. I'll revert those additions as it was a slim chance that this would fix the build.
@tswast While you can't easily see the build now, Kokoro failed: https://source.cloud.google.com/results/invocations/b41e3d1e-1e73-454d-a24a-d1a790c7b129/targets/cloud-devrel%2Fclient-libraries%2Fpython%2Fgoogleapis%2Fpython-bigquery-sqlalchemy%2Fpresubmit%2Fpresubmit/log It complained of Secondly, while poking around in there, I realized that owlbot seems to generate the Kokoro configuration files and one hasn't been created for Python 3.11. Should that be done before this PR is ready to release? |
My colleague parthea jumped in and added Python 3.11 to our docker images via a separate repo that controls some of what happens in our CI/CD pipeline. That may help to clear up some of the issues associated with a missing 3.11 interpreter in both this and another of our repositories. I appreciate all you are doing to try and assist with moving this forward. |
@chalmerlowe @parthea I'm curious how you'd like to handle the failures on the current build. It appears that "Kokoro prerelease" is failing because nox is allowing the prerelease installation of sqlalchemy and sqlalchemy 2.0.0 is in pre-release. This contradicts the dependencies of python-biquery-sqlalchemy, which requires sqlalchemy to be < 2.0.0. Do you want to:
The SQLAlchemy compliance check is failing and complaining about legacy features not available in sqlalchemy 2.0, and I'm curious if the method of addressing the pre-release check would inform the compliance check as well. |
The prerelease checks are not required to pass in order to merge, and since the failures aren't caused by this change, we can safely ignore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As noted by @tswast compliance and prerelease tests are not caused by this change, but as demonstrated by @mr-mcox they are a result of the fact that the sqlalchemy project has V2 of slqalchemy tagged as pre-release and this library is not compatible with V2. I am fine with merging this change to enable the use of Python 3.11. |
Done:
Fixes #500 🦕
I followed the pattern from the support for python 3.10 PR , so I hope that I caught everything that needed to change.