-
Notifications
You must be signed in to change notification settings - Fork 139
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
Changes from 5 commits
6f11d53
4e86c04
21fb2ae
a8fa953
a00b378
a9881fa
8af7c47
762a46d
e50cad3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,10 +18,12 @@ | |
|
||
from __future__ import absolute_import | ||
|
||
from functools import wraps | ||
import os | ||
import pathlib | ||
import re | ||
import shutil | ||
import time | ||
from typing import Dict, List | ||
import warnings | ||
|
||
|
@@ -39,9 +41,9 @@ | |
"setup.py", | ||
] | ||
|
||
DEFAULT_PYTHON_VERSION = "3.8" | ||
DEFAULT_PYTHON_VERSION = "3.10" | ||
|
||
UNIT_TEST_PYTHON_VERSIONS: List[str] = ["3.8", "3.9", "3.10", "3.11", "3.12", "3.13"] | ||
UNIT_TEST_PYTHON_VERSIONS: List[str] = ["3.9", "3.10", "3.11", "3.12", "3.13"] | ||
UNIT_TEST_STANDARD_DEPENDENCIES = [ | ||
"mock", | ||
"asyncmock", | ||
|
@@ -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 commentThe 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 commentThe 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 For unit tests, our noxfile currently overrides the effect of the variable In the
The truthfulness of this can be see in the Kokoro CI/CD results which show:
3.9 (shortened for space reasons):
3.13 (shortened for space reasons):
|
||
"tests", | ||
"alembic", | ||
"bqstorage", | ||
], | ||
"3.11": [ | ||
"tests", | ||
"geography", | ||
|
@@ -78,7 +75,7 @@ | |
], | ||
} | ||
|
||
SYSTEM_TEST_PYTHON_VERSIONS: List[str] = ["3.8", "3.12", "3.13"] | ||
SYSTEM_TEST_PYTHON_VERSIONS: List[str] = ["3.9", "3.12", "3.13"] | ||
SYSTEM_TEST_STANDARD_DEPENDENCIES: List[str] = [ | ||
"mock", | ||
"pytest", | ||
|
@@ -91,11 +88,6 @@ | |
"tests", | ||
] | ||
SYSTEM_TEST_EXTRAS_BY_PYTHON: Dict[str, List[str]] = { | ||
"3.8": [ | ||
"tests", | ||
"alembic", | ||
"bqstorage", | ||
], | ||
"3.12": [ | ||
"tests", | ||
"geography", | ||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I removed this code. |
||
"""This decorator prints the execution time for the decorated function.""" | ||
|
||
@wraps(func) | ||
def wrapper(*args, **kwargs): | ||
start = time.monotonic() | ||
result = func(*args, **kwargs) | ||
end = time.monotonic() | ||
total_seconds = round(end - start) | ||
hours = total_seconds // 3600 # Integer division to get hours | ||
remaining_seconds = total_seconds % 3600 # Modulo to find remaining seconds | ||
minutes = remaining_seconds // 60 | ||
seconds = remaining_seconds % 60 | ||
human_time = f"{hours:}:{minutes:0>2}:{seconds:0>2}" | ||
print(f"Session ran in {total_seconds} seconds ({human_time})") | ||
return result | ||
|
||
return wrapper | ||
|
||
|
||
nox.options.sessions = [ | ||
"unit", | ||
"system", | ||
|
@@ -128,13 +141,15 @@ | |
|
||
|
||
@nox.session(python=DEFAULT_PYTHON_VERSION) | ||
@_calculate_duration | ||
def lint(session): | ||
"""Run linters. | ||
|
||
Returns a failure if the linters find linting errors or sufficiently | ||
serious code quality issues. | ||
""" | ||
session.install(FLAKE8_VERSION, BLACK_VERSION) | ||
session.run("python", "-m", "pip", "freeze") | ||
session.run( | ||
"black", | ||
"--check", | ||
|
@@ -144,16 +159,19 @@ def lint(session): | |
|
||
|
||
@nox.session(python=DEFAULT_PYTHON_VERSION) | ||
@_calculate_duration | ||
def blacken(session): | ||
"""Run black. Format code to uniform standard.""" | ||
session.install(BLACK_VERSION) | ||
session.run("python", "-m", "pip", "freeze") | ||
session.run( | ||
"black", | ||
*LINT_PATHS, | ||
) | ||
|
||
|
||
@nox.session(python=DEFAULT_PYTHON_VERSION) | ||
@_calculate_duration | ||
def format(session): | ||
""" | ||
Run isort to sort imports. Then run black | ||
|
@@ -162,6 +180,7 @@ def format(session): | |
session.install(BLACK_VERSION, ISORT_VERSION) | ||
# Use the --fss option to sort imports using strict alphabetical order. | ||
# See https://pycqa.github.io/isort/docs/configuration/options.html#force-sort-within-sections | ||
session.run("python", "-m", "pip", "freeze") | ||
session.run( | ||
"isort", | ||
"--fss", | ||
|
@@ -174,9 +193,11 @@ def format(session): | |
|
||
|
||
@nox.session(python=DEFAULT_PYTHON_VERSION) | ||
@_calculate_duration | ||
def lint_setup_py(session): | ||
"""Verify that setup.py is valid (including RST check).""" | ||
session.install("docutils", "pygments") | ||
session.run("python", "-m", "pip", "freeze") | ||
session.run("python", "setup.py", "check", "--restructuredtext", "--strict") | ||
|
||
|
||
|
@@ -213,6 +234,7 @@ def install_unittest_dependencies(session, *constraints): | |
"protobuf_implementation", | ||
["python", "upb", "cpp"], | ||
) | ||
@_calculate_duration | ||
def unit(session, protobuf_implementation, install_extras=True): | ||
# Install all test dependencies, then install this package in-place. | ||
|
||
|
@@ -239,6 +261,7 @@ def unit(session, protobuf_implementation, install_extras=True): | |
session.install("protobuf<4") | ||
|
||
# Run py.test against the unit tests. | ||
session.run("python", "-m", "pip", "freeze") | ||
session.run( | ||
"py.test", | ||
"--quiet", | ||
|
@@ -288,6 +311,7 @@ def install_systemtest_dependencies(session, *constraints): | |
|
||
|
||
@nox.session(python=SYSTEM_TEST_PYTHON_VERSIONS) | ||
@_calculate_duration | ||
def system(session): | ||
"""Run the system test suite.""" | ||
constraints_path = str( | ||
|
@@ -310,6 +334,7 @@ def system(session): | |
session.skip("System tests were not found") | ||
|
||
install_systemtest_dependencies(session, "-c", constraints_path) | ||
session.run("python", "-m", "pip", "freeze") | ||
|
||
# Run py.test against the system tests. | ||
if system_test_exists: | ||
|
@@ -331,6 +356,7 @@ def system(session): | |
|
||
|
||
@nox.session(python=SYSTEM_TEST_PYTHON_VERSIONS) | ||
@_calculate_duration | ||
def system_noextras(session): | ||
"""Run the system test suite.""" | ||
constraints_path = str( | ||
|
@@ -355,6 +381,7 @@ def system_noextras(session): | |
global SYSTEM_TEST_EXTRAS_BY_PYTHON | ||
SYSTEM_TEST_EXTRAS_BY_PYTHON = False | ||
install_systemtest_dependencies(session, "-c", constraints_path) | ||
session.run("python", "-m", "pip", "freeze") | ||
|
||
# Run py.test against the system tests. | ||
if system_test_exists: | ||
|
@@ -376,6 +403,7 @@ def system_noextras(session): | |
|
||
|
||
@nox.session(python=SYSTEM_TEST_PYTHON_VERSIONS[-1]) | ||
@_calculate_duration | ||
def compliance(session): | ||
"""Run the SQLAlchemy dialect-compliance system tests""" | ||
constraints_path = str( | ||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. The 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. |
||
elif session.python in ["3.12", "3.13"]: | ||
if session.python in ["3.12", "3.13"]: | ||
extras = "[tests,geography]" | ||
else: | ||
extras = "[tests]" | ||
|
@@ -430,19 +456,22 @@ def compliance(session): | |
|
||
|
||
@nox.session(python=DEFAULT_PYTHON_VERSION) | ||
@_calculate_duration | ||
def cover(session): | ||
"""Run the final coverage report. | ||
|
||
This outputs the coverage report aggregating coverage from the unit | ||
test runs (not system test runs), and then erases coverage data. | ||
""" | ||
session.install("coverage", "pytest-cov") | ||
session.run("python", "-m", "pip", "freeze") | ||
session.run("coverage", "report", "--show-missing", "--fail-under=100") | ||
|
||
session.run("coverage", "erase") | ||
|
||
|
||
@nox.session(python="3.10") | ||
@_calculate_duration | ||
def docs(session): | ||
"""Build the docs for this library.""" | ||
|
||
|
@@ -465,6 +494,7 @@ def docs(session): | |
) | ||
|
||
shutil.rmtree(os.path.join("docs", "_build"), ignore_errors=True) | ||
session.run("python", "-m", "pip", "freeze") | ||
session.run( | ||
"sphinx-build", | ||
"-W", # warnings as errors | ||
|
@@ -480,6 +510,7 @@ def docs(session): | |
|
||
|
||
@nox.session(python="3.10") | ||
@_calculate_duration | ||
def docfx(session): | ||
"""Build the docfx yaml files for this library.""" | ||
|
||
|
@@ -502,6 +533,7 @@ def docfx(session): | |
) | ||
|
||
shutil.rmtree(os.path.join("docs", "_build"), ignore_errors=True) | ||
session.run("python", "-m", "pip", "freeze") | ||
session.run( | ||
"sphinx-build", | ||
"-T", # show full traceback on exception | ||
|
@@ -532,6 +564,7 @@ def docfx(session): | |
"protobuf_implementation", | ||
["python", "upb", "cpp"], | ||
) | ||
@_calculate_duration | ||
def prerelease_deps(session, protobuf_implementation): | ||
"""Run all tests with prerelease versions of dependencies installed.""" | ||
|
||
|
@@ -593,6 +626,7 @@ def prerelease_deps(session, protobuf_implementation): | |
"requests", | ||
] | ||
session.install(*other_deps) | ||
session.run("python", "-m", "pip", "freeze") | ||
|
||
# Print out prerelease package versions | ||
session.run( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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. |
||
"3.11": ["tests", "geography", "bqstorage"], | ||
"3.12": ["tests", "geography", "bqstorage"], | ||
"3.13": ["tests", "geography", "bqstorage"], | ||
} | ||
templated_files = common.py_library( | ||
unit_test_python_versions=["3.8", "3.9", "3.10", "3.11", "3.12", "3.13"], | ||
system_test_python_versions=["3.8", "3.12", "3.13"], | ||
unit_test_python_versions=["3.9", "3.10", "3.11", "3.12", "3.13"], | ||
system_test_python_versions=["3.9", "3.12", "3.13"], | ||
cov_level=100, | ||
unit_test_extras=extras, | ||
unit_test_extras_by_python=extras_by_python, | ||
|
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.