-
Notifications
You must be signed in to change notification settings - Fork 138
feat: remove python 3.8 support #1215
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
Conversation
@@ -56,11 +58,6 @@ | |||
"tests", | |||
] | |||
UNIT_TEST_EXTRAS_BY_PYTHON: Dict[str, List[str]] = { | |||
"3.8": [ |
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.
Do we want to change this to 3.9 instead?
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.
Broadly, the unit & system test extras are not handled well in this (and likely all our noxfiles
). As I will show below, we do fully test this library during unit tests, but I think it could be cleaned up (determine which extras need to be done with which Python versions, why, provide comments to clarify the why, and ensure that the logic across the setup.py
, owlbot.py
, noxfile.py
are all in sync, etc.). I recommend that the clean up be a separate Issue as out of scope for the focus of this PR.
For unit tests, our noxfile currently overrides the effect of the variable
UNIT_TEST_EXTRAS_BY_PYTHON
. UNIT_TEST_EXTRAS_BY_PYTHON
is used in the function install_unittest_dependencies()
.
In the unit
session there is another snippet that also selects extras to be installed. As can be seen below, for any Python version from 3.11
-3.13
we test against alembic
. But we see that for any Python version outside of 3.11
-3.13
we test against all
extras, which by definition in setup.py includes alembic
.
if install_extras and session.python in ["3.11", "3.12", "3.13"]:
install_target = ".[geography,alembic,tests,bqstorage]"
elif install_extras:
install_target = ".[all]"
else:
install_target = "."
session.install("-e", install_target, "-c", constraints_path)
The truthfulness of this can be see in the Kokoro CI/CD results which show:
- the install command:
[all]
versus[geography,alembic,tests,bqstorage]
based on the Python version - the actual install results from
pip freeze
(which includes alembic in every case) - the successful run for the alembic tests (in every case)
- NOTE: I only show two versions, but I hand-checked every version.
3.9 (shortened for space reasons):
nox > Running session unit-3.9(protobuf_implementation='cpp')
...
nox > python -m pip install -e '.[all]' -c /tmpfs/src/github/python-bigquery-sqlalchemy/testing/constraints-3.9.txt
...
nox > python -m pip freeze
alembic==1.16.4
asyncmock==0.4.2
...
tests/unit/test__types.py ........... [ 9%]
tests/unit/test_alembic.py .. [ 10%]
tests/unit/test_catalog_functions.py ................................... [ 21%]
3.13 (shortened for space reasons):
nox > Running session unit-3.13(protobuf_implementation='upb')
...
nox > python -m pip install -e '.[geography,alembic,tests,bqstorage]' -c /tmpfs/src/github/python-bigquery-sqlalchemy/testing/constraints-3.13.txt
nox > python -m pip freeze
alembic==1.16.4
asyncmock==0.4.2
...
tests/unit/test_alembic.py .. [ 10%]
tests/unit/test_catalog_functions.py ................................... [ 21%]
CONTRIBUTING.rst
Outdated
@@ -148,7 +148,7 @@ Running System Tests | |||
|
|||
.. note:: | |||
|
|||
System tests are only configured to run under Python 3.8, 3.12, and 3.13. | |||
System tests are only configured to run under Python 3.12, and 3.13. |
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.
Need to add 3.9 here
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.
Good catch, thanks! Fixed.
@@ -398,9 +426,7 @@ def compliance(session): | |||
"-c", | |||
constraints_path, | |||
) | |||
if session.python == "3.8": | |||
extras = "[tests,alembic]" |
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 wonder if we still need coverage for alembic?
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.
The compliance
session selects extras slightly differently than the unit
session.
I added alembic
to this list of extras to account for that difference. DONE.
I assert that as part of the cleanup task discussed in another comment, all our noxfiles need to be checked to see when/why we choose to do some tests and not others. I suspect there was a historical reason that may OR may not apply.
@@ -30,14 +30,13 @@ | |||
# ---------------------------------------------------------------------------- | |||
extras = ["tests"] | |||
extras_by_python = { | |||
"3.8": ["tests", "alembic", "bqstorage"], |
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.
It feels like we need to change this into 3.9 too?
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 added 3.9 with alembic as a system test.
NOTE: there appear to be issues with how owlbot and noxfile, etc are interacting, much like described above for the unit tests. That should be looked into as part of a separate cleanup task.
noxfile.py
Outdated
@@ -110,6 +102,27 @@ | |||
|
|||
CURRENT_DIRECTORY = pathlib.Path(__file__).parent.absolute() | |||
|
|||
|
|||
def _calculate_duration(func): |
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.
If this is not necessary to the 3.8 removal and is a "would be nice", please split into a second PR - I'm happy to be a reviewer for it!
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 removed this code.
@@ -62,7 +62,7 @@ def readme(): | |||
# https://github.com/grpc/grpc/pull/15254 | |||
"grpcio >= 1.47.0, < 2.0.0", | |||
"grpcio >= 1.49.1, < 2.0.0; python_version>='3.11'", | |||
"pyarrow >= 3.0.0", | |||
"pyarrow >= 5.0.0", |
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.
Confirming - tests would fail if there were consequences to this +2 major version jump, right?
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.
Tests will fail.
For context: the current version of pyarrow is 20.0
Why version 5.0
?:
- Version
5.0
came out in 2021. - Version
3.0
and version4.0
do NOT run on Python 3.9. - Version
5.0
is the first and oldest version that ran on Python 3.9.
Also adds several minor tweaks that were useful for debugging some of the mods to this PR (and are useful in general for all future debugging efforts), such as:
pip freeze
to display what dependencies have been installed* UPDATE: removed upon request:@calculate_duration
decorator to confirm the total time to execute anox session
.