-
Notifications
You must be signed in to change notification settings - Fork 6
deps!: Drop support for Python 3.7 and 3.8 (AI Experiment) #337
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
base: main
Are you sure you want to change the base?
Conversation
Removes support for Python 3.7 and 3.8, establishing Python 3.9 as the new minimum supported version. This change involves: - Updating `python_requires` and classifiers in `setup.py`. - Modifying Python versions in `noxfile.py` (default, unit tests, system tests) and ensuring constraint file logic remains correct. - Updating the GitHub Actions workflow (`unittest.yml`) matrix, runner, and coverage job version. - Deleting constraint files for Python 3.7 and 3.8 (`testing/constraints-3.7.txt`, `testing/constraints-3.8.txt`). - Removing Kokoro sample configuration directories (`.kokoro/samples/python3.7/`, `.kokoro/samples/python3.8/`). - Updating supported version mentions in `README.rst`. - Removing 3.7 and 3.8 from the `ALL_VERSIONS` list in `samples/snippets/noxfile.py`.
Just a suggestion, maybe use |
Conventional Commit points at this page as reference for prefixes: 'build', If Google also uses deps, then sure. |
Wow, I suggested it because we used it in the commit for python-bigquery. Apparently it's not part of convetional commits, but it worked. Edit: |
Done. |
db_dtypes/__init__.py
Outdated
# Inside db_dtypes/__init__.py (TEMPORARY DEBUG) | ||
print(f"DEBUG: Inside __init__, JSONArray type: {type(JSONArray)}, value: {repr(JSONArray)}") | ||
print(f"DEBUG: Inside __init__, JSONDtype type: {type(JSONDtype)}, value: {repr(JSONDtype)}") |
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.
We'll want to remove these before merging.
# Inside db_dtypes/__init__.py (TEMPORARY DEBUG) | |
print(f"DEBUG: Inside __init__, JSONArray type: {type(JSONArray)}, value: {repr(JSONArray)}") | |
print(f"DEBUG: Inside __init__, JSONDtype type: {type(JSONDtype)}, value: {repr(JSONDtype)}") |
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.
Done.
db_dtypes/__init__.py
Outdated
if sys_major == 3 and sys_minor in (7, 8): | ||
warnings.warn( | ||
"The python-bigquery library as well as the python-db-dtypes-pandas library no " | ||
"longer supports Python 3.7 and Python 3.8. " | ||
f"Your Python version is {sys_major}.{sys_minor}.{sys_micro}. We " | ||
"recommend that you update soon to ensure ongoing support. For " | ||
"more details, see: [Google Cloud Client Libraries Supported Python Versions policy](https://cloud.google.com/python/docs/supported-python-versions)", | ||
FutureWarning, | ||
stacklevel=2, # Point warning to the caller of __init__ | ||
) |
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 you use python_requires
in setup.py
, I'm having a hard time seeing how someone could end up in this state.
Do we want to issue this in a separate PR and release so folks see it before fully dropping?
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 can do that.
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.
Done.
db_dtypes/__init__.py
Outdated
# sys_major, sys_minor, sys_micro = _versions_helpers.extract_runtime_version() | ||
# if sys_major == 3 and sys_minor in (7, 8): | ||
# warnings.warn( | ||
# "The python-bigquery library as well as the python-db-dtypes-pandas library no " | ||
# "longer supports Python 3.7 and Python 3.8. " | ||
# f"Your Python version is {sys_major}.{sys_minor}.{sys_micro}. We " | ||
# "recommend that you update soon to ensure ongoing support. For " | ||
# "more details, see: [Google Cloud Client Libraries Supported Python Versions policy](https://cloud.google.com/python/docs/supported-python-versions)", | ||
# FutureWarning, | ||
# ) | ||
|
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.
Let's remove this commented out code.
# sys_major, sys_minor, sys_micro = _versions_helpers.extract_runtime_version() | |
# if sys_major == 3 and sys_minor in (7, 8): | |
# warnings.warn( | |
# "The python-bigquery library as well as the python-db-dtypes-pandas library no " | |
# "longer supports Python 3.7 and Python 3.8. " | |
# f"Your Python version is {sys_major}.{sys_minor}.{sys_micro}. We " | |
# "recommend that you update soon to ensure ongoing support. For " | |
# "more details, see: [Google Cloud Client Libraries Supported Python Versions policy](https://cloud.google.com/python/docs/supported-python-versions)", | |
# FutureWarning, | |
# ) |
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.
Done
_check_python_version() | ||
|
||
# Assign __all__ by calling the function | ||
__all__ = _determine_all(JSONArray, JSONDtype) |
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.
This seems a bit too "magic" to me, but I suppose it could make testing easier?
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.
This code was originally just in the file with no function def. We added _check_python_version...
To test against supported and unsupported versions we attempted to mock the version but timing became an issue, partly because dependencies also rely on version checks so our mock ended up preventing error-free installs of some dependencies.
We tried half a dozen ways to alter when the mock happened but none hit that sweet spot of reliably allowing the dependencies to do their thing and slipping in to allow our test to run.
Similarly, we ran into a mess of problems with import timing and mocking whether JSONArray JSONDtype did or did not import as expected and so ...
putting them in functions made things much easier in the testing arena.
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.
Any hard feelings if we resolve this conversation?
testing/constraints-3.7.txt
Outdated
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.
Make sure to update constraints-3.9.txt with our minimum supported package versions from setup.py
.
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.
Done.
@@ -24,7 +24,7 @@ | |||
import pytest | |||
|
|||
# NDArrayBacked2DTests suite added in https://github.com/pandas-dev/pandas/pull/44974 | |||
pytest.importorskip("pandas", minversion="1.5.0dev") | |||
pytest.importorskip("pandas") |
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.
pandas
is a strict dependency. I don't think we need this line. We were just using it for the min version before.
pytest.importorskip("pandas") |
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.
Since we rely on pandas
version >=1.5.0
this whole file feels unnecessary.
- I am gonna move this test to the generic
date_compliance
file - I will delete this
1_5
date_compliance
-specific file - I am gonna move a similar test from
test_time_compliance_1_5.py
to the generictime_compliance
file - I will delete this
1_5
time_compliance
-specific file
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.
Done.
@@ -24,7 +24,7 @@ | |||
import pytest | |||
|
|||
# NDArrayBacked2DTests suite added in https://github.com/pandas-dev/pandas/pull/44974 | |||
pytest.importorskip("pandas", minversion="1.5.0dev") | |||
pytest.importorskip("pandas") |
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.
Same here
pytest.importorskip("pandas") |
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.
See comment above about date_compliance.
Done.
tests/unit/test__init__.py
Outdated
@@ -0,0 +1,137 @@ | |||
import sys |
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.
Missing license header.
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.
Fixed.
tests/unit/test__init__.py
Outdated
import pytest | ||
import types | ||
import warnings | ||
from unittest import mock | ||
import pyarrow as pa |
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.
Imports should be grouped in the following order:
- Standard library imports.
- Related third party imports.
- Local application/library specific imports.
You should put a blank line between each group of imports.
https://peps.python.org/pep-0008/#imports
import pytest | |
import types | |
import warnings | |
from unittest import mock | |
import pyarrow as pa | |
import types | |
import warnings | |
from unittest import mock | |
import pyarrow as pa | |
import pytest |
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.
Done.
Can you confirm comment resolution? And if you feel comfortable, approve? I am unable to get There is a class in There are a pair of code branches that for the life of me, no matter what I tried, I could not get coverage to acknowledge that the code was covered despite confirming that those lines of code did execute. I flagged both of those with |
This branch is generated automagically via AI as an experiment.
Requires a comprehensive human review and comment.
Do not merge until approved by an authorized reviewer.
Removes support for Python 3.7 and 3.8, establishing Python 3.9 as the new minimum supported version.
This change involves:
python_requires
and classifiers insetup.py
.noxfile.py
(default, unit tests, system tests) and ensuring constraint file logic remains correct.unittest.yml
) matrix, runner, and coverage job version.testing/constraints-3.7.txt
,testing/constraints-3.8.txt
)..kokoro/samples/python3.7/
,.kokoro/samples/python3.8/
).README.rst
.ALL_VERSIONS
list insamples/snippets/noxfile.py
.