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

fix: add support for protobuf 5.x #644

Merged
merged 14 commits into from
Jun 19, 2024
2 changes: 1 addition & 1 deletion .github/workflows/unittest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
option: ["", "_grpc_gcp", "_wo_grpc"]
option: ["", "_grpc_gcp", "_wo_grpc", "_with_prerelease_deps"]
python:
- "3.7"
- "3.8"
Expand Down
6 changes: 4 additions & 2 deletions google/api_core/operations_v1/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

"""Package for interacting with the google.longrunning.operations meta-API."""

from google.api_core.operations_v1.abstract_operations_client import AbstractOperationsClient
from google.api_core.operations_v1.abstract_operations_client import (
AbstractOperationsClient,
)
Comment on lines +17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ran black on all of the files and this file was formatted

from google.api_core.operations_v1.operations_async_client import OperationsAsyncClient
from google.api_core.operations_v1.operations_client import OperationsClient
from google.api_core.operations_v1.transports.rest import OperationsRestTransport
Expand All @@ -23,5 +25,5 @@
"AbstractOperationsClient",
"OperationsAsyncClient",
"OperationsClient",
"OperationsRestTransport"
"OperationsRestTransport",
]
2 changes: 1 addition & 1 deletion google/api_core/operations_v1/transports/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def __init__(
self,
*,
host: str = DEFAULT_HOST,
credentials: ga_credentials.Credentials = None,
credentials: Optional[ga_credentials.Credentials] = None,
credentials_file: Optional[str] = None,
scopes: Optional[Sequence[str]] = None,
quota_project_id: Optional[str] = None,
Expand Down
67 changes: 41 additions & 26 deletions google/api_core/operations_v1/transports/rest.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,13 @@
from google.longrunning import operations_pb2 # type: ignore
from google.protobuf import empty_pb2 # type: ignore
from google.protobuf import json_format # type: ignore
import google.protobuf

import grpc
from .base import DEFAULT_CLIENT_INFO as BASE_DEFAULT_CLIENT_INFO, OperationsTransport

PROTOBUF_VERSION = google.protobuf.__version__

OptionalRetry = Union[retries.Retry, object]

DEFAULT_CLIENT_INFO = gapic_v1.client_info.ClientInfo(
Expand Down Expand Up @@ -66,7 +70,7 @@ def __init__(
self,
*,
host: str = "longrunning.googleapis.com",
credentials: ga_credentials.Credentials = None,
credentials: Optional[ga_credentials.Credentials] = None,
credentials_file: Optional[str] = None,
scopes: Optional[Sequence[str]] = None,
client_cert_source_for_mtls: Optional[Callable[[], Tuple[bytes, bytes]]] = None,
Expand Down Expand Up @@ -184,11 +188,7 @@ def _list_operations(
"google.longrunning.Operations.ListOperations"
]

request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True,
)
request_kwargs = self._convert_protobuf_message_to_dict(request)
transcoded_request = path_template.transcode(http_options, **request_kwargs)

uri = transcoded_request["uri"]
Expand All @@ -199,7 +199,6 @@ def _list_operations(
json_format.ParseDict(transcoded_request["query_params"], query_params_request)
query_params = json_format.MessageToDict(
query_params_request,
including_default_value_fields=False,
preserving_proto_field_name=False,
use_integers_for_enums=False,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is essentially also converting a message to a dict. So we could use a similar approach to what we're doing for request_params and define a helper function so we're consistent. But it's more of a preference and could be a follow up.

I'll leave it up to you since this isn't a blocker!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took a closer look at making this change and I don't believe the helper function will result in fewer lines of code. The reason that we had the helper function elsewhere was to remove duplication for the protobuf 3.x/4.x compatibility code but we don't have the duplication here.

Expand Down Expand Up @@ -265,11 +264,7 @@ def _get_operation(
"google.longrunning.Operations.GetOperation"
]

request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True,
)
request_kwargs = self._convert_protobuf_message_to_dict(request)
transcoded_request = path_template.transcode(http_options, **request_kwargs)

uri = transcoded_request["uri"]
Expand All @@ -280,7 +275,6 @@ def _get_operation(
json_format.ParseDict(transcoded_request["query_params"], query_params_request)
query_params = json_format.MessageToDict(
query_params_request,
including_default_value_fields=False,
preserving_proto_field_name=False,
use_integers_for_enums=False,
)
Expand Down Expand Up @@ -339,11 +333,7 @@ def _delete_operation(
"google.longrunning.Operations.DeleteOperation"
]

request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True,
)
request_kwargs = self._convert_protobuf_message_to_dict(request)
transcoded_request = path_template.transcode(http_options, **request_kwargs)

uri = transcoded_request["uri"]
Expand All @@ -354,7 +344,6 @@ def _delete_operation(
json_format.ParseDict(transcoded_request["query_params"], query_params_request)
query_params = json_format.MessageToDict(
query_params_request,
including_default_value_fields=False,
preserving_proto_field_name=False,
use_integers_for_enums=False,
)
Expand Down Expand Up @@ -411,19 +400,14 @@ def _cancel_operation(
"google.longrunning.Operations.CancelOperation"
]

request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True,
)
request_kwargs = self._convert_protobuf_message_to_dict(request)
transcoded_request = path_template.transcode(http_options, **request_kwargs)

# Jsonify the request body
body_request = operations_pb2.CancelOperationRequest()
json_format.ParseDict(transcoded_request["body"], body_request)
body = json_format.MessageToDict(
body_request,
including_default_value_fields=False,
preserving_proto_field_name=False,
use_integers_for_enums=False,
)
Expand All @@ -435,7 +419,6 @@ def _cancel_operation(
json_format.ParseDict(transcoded_request["query_params"], query_params_request)
query_params = json_format.MessageToDict(
query_params_request,
including_default_value_fields=False,
preserving_proto_field_name=False,
use_integers_for_enums=False,
)
Expand All @@ -458,6 +441,38 @@ def _cancel_operation(

return empty_pb2.Empty()

def _convert_protobuf_message_to_dict(
self, message: google.protobuf.message.Message
):
r"""Converts protobuf message to a dictionary.

When the dictionary is encoded to JSON, it conforms to proto3 JSON spec.

Args:
message(google.protobuf.message.Message): The protocol buffers message
instance to serialize.

Returns:
A dict representation of the protocol buffer message.
"""
# For backwards compatibility with protobuf 3.x 4.x
# Remove once support for protobuf 3.x and 4.x is dropped
# https://github.com/googleapis/python-api-core/issues/643
if PROTOBUF_VERSION[0:2] in ["3.", "4."]:
result = json_format.MessageToDict(
message,
preserving_proto_field_name=True,
including_default_value_fields=True, # type: ignore # backward compatibility
)
else:
result = json_format.MessageToDict(
message,
preserving_proto_field_name=True,
always_print_fields_with_no_presence=True,
)

return result

@property
def list_operations(
self,
Expand Down
100 changes: 84 additions & 16 deletions noxfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
from __future__ import absolute_import
import os
import pathlib
import re
import shutil
import unittest

# https://github.com/google/importlab/issues/25
import nox # pytype: disable=import-error
Expand All @@ -26,6 +28,8 @@
# Black and flake8 clash on the syntax for ignoring flake8's F401 in this file.
BLACK_EXCLUDES = ["--exclude", "^/google/api_core/operations_v1/__init__.py"]

PYTHON_VERSIONS = ["3.7", "3.8", "3.9", "3.10", "3.11", "3.12"]

DEFAULT_PYTHON_VERSION = "3.10"
CURRENT_DIRECTORY = pathlib.Path(__file__).parent.absolute()

Expand Down Expand Up @@ -72,17 +76,46 @@ def blacken(session):
session.run("black", *BLACK_EXCLUDES, *BLACK_PATHS)


def default(session, install_grpc=True):
def install_prerelease_dependencies(session, constraints_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I have gaps in my mental model for testing prerelease;

This installs our dependencies at pre-release versions, right?

Is the only difference, then, the use of --pre below? If it's the same set of dependencies, it seems it would be clearer to have them loaded in the same place for pre-release and stable versions, and add the extra install parameters conditionally on each thing we install.

(It might be easier to talk synchronously about this.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a difference in that we don't allow transitive dependencies in the install_prerelease_dependencies session as we want to ensure that we're installing the pre-release version of each dependency. In the non-prerelease session, we are installing transitive dependencies.

with open(constraints_path, encoding="utf-8") as constraints_file:
constraints_text = constraints_file.read()
# Ignore leading whitespace and comment lines.
constraints_deps = [
match.group(1)
for match in re.finditer(
r"^\s*(\S+)(?===\S+)", constraints_text, flags=re.MULTILINE
)
]
session.install(*constraints_deps)
prerel_deps = [
"google-auth",
"googleapis-common-protos",
"grpcio",
"grpcio-status",
"proto-plus",
"protobuf",
]

for dep in prerel_deps:
session.install("--pre", "--no-deps", "--upgrade", dep)

# Remaining dependencies
other_deps = [
"requests",
]
session.install(*other_deps)


def default(session, install_grpc=True, prerelease=False):
"""Default unit test session.

This is intended to be run **without** an interpreter set, so
that the current ``python`` (on the ``PATH``) or the version of
Python corresponding to the ``nox`` binary the ``PATH`` can
run the tests.
"""
constraints_path = str(
CURRENT_DIRECTORY / "testing" / f"constraints-{session.python}.txt"
)
if prerelease and not install_grpc:
unittest.skip("The pre-release session cannot be run without grpc")

session.install(
"dataclasses",
Expand All @@ -92,10 +125,36 @@ def default(session, install_grpc=True):
"pytest-xdist",
)

if install_grpc:
session.install("-e", ".[grpc]", "-c", constraints_path)
constraints_dir = str(CURRENT_DIRECTORY / "testing")

if prerelease:
install_prerelease_dependencies(
session, f"{constraints_dir}/constraints-{PYTHON_VERSIONS[0]}.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused: in line 206, where we call default(..., prerelease=True), the decorator constrains python=PYTHON_VERSIONS[-1]. But here we are referencing the constraints for PYTHON_VERSIONS[0]. Could you clarify?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As per the comment in below, the constraints file for the lowest supported python version contains a list of all of the dependencies of the library. The function install_prerelease_dependencies will extract the dependencies from the file.

# This constraints file is used to check that lower bounds
# are correct in setup.py
# List *all* library dependencies and extras in this file.
# Pin the version to the lower bound.
#
# e.g., if setup.py has "foo >= 1.14.0, < 2.0.0dev",
# Then this file should have foo==1.14.0

def install_prerelease_dependencies(session, constraints_path):
with open(constraints_path, encoding="utf-8") as constraints_file:
constraints_text = constraints_file.read()
# Ignore leading whitespace and comment lines.
constraints_deps = [
match.group(1)
for match in re.finditer(
r"^\s*(\S+)(?===\S+)", constraints_text, flags=re.MULTILINE
)
]

Once we have a list of dependencies, we will install them independently with the --pre and --no-deps option. `--upgrade is also added to ensure that we get the latest pre-release.

python-api-core/noxfile.py

Lines 99 to 100 in 7fbce0d

for dep in prerel_deps:
session.install("--pre", "--no-deps", "--upgrade", dep)

)
# This *must* be the last install command to get the package from source.
session.install("-e", ".", "--no-deps")
else:
session.install("-e", ".", "-c", constraints_path)
session.install(
"-e",
".[grpc]" if install_grpc else ".",
"-c",
f"{constraints_dir}/constraints-{session.python}.txt",
)

# Print out package versions of dependencies
session.run(
"python", "-c", "import google.protobuf; print(google.protobuf.__version__)"
)
# Support for proto.version was added in v1.23.0
# https://github.com/googleapis/proto-plus-python/releases/tag/v1.23.0
session.run(
"python",
"-c",
"""import proto; hasattr(proto, "version") and print(proto.version.__version__)""",
)
if install_grpc:
session.run("python", "-c", "import grpc; print(grpc.__version__)")
session.run("python", "-c", "import google.auth; print(google.auth.__version__)")

pytest_args = [
"python",
Expand Down Expand Up @@ -130,15 +189,26 @@ def default(session, install_grpc=True):
session.run(*pytest_args)


@nox.session(python=["3.7", "3.8", "3.9", "3.10", "3.11", "3.12"])
@nox.session(python=PYTHON_VERSIONS)
def unit(session):
"""Run the unit test suite."""
default(session)


@nox.session(python=["3.7", "3.8", "3.9", "3.10", "3.11", "3.12"])
@nox.session(python=PYTHON_VERSIONS)
def unit_with_prerelease_deps(session):
"""Run the unit test suite."""
default(session, prerelease=True)


@nox.session(python=PYTHON_VERSIONS)
def unit_grpc_gcp(session):
"""Run the unit test suite with grpcio-gcp installed."""
"""
Run the unit test suite with grpcio-gcp installed.
`grpcio-gcp` doesn't support protobuf 4+.
Remove extra `grpcgcp` when protobuf 3.x is dropped.
https://github.com/googleapis/python-api-core/issues/594
"""
constraints_path = str(
CURRENT_DIRECTORY / "testing" / f"constraints-{session.python}.txt"
)
Expand All @@ -150,7 +220,7 @@ def unit_grpc_gcp(session):
default(session)


@nox.session(python=["3.8", "3.10", "3.11", "3.12"])
@nox.session(python=PYTHON_VERSIONS)
def unit_wo_grpc(session):
"""Run the unit test suite w/o grpcio installed"""
default(session, install_grpc=False)
Expand All @@ -164,10 +234,10 @@ def lint_setup_py(session):
session.run("python", "setup.py", "check", "--restructuredtext", "--strict")


@nox.session(python="3.8")
@nox.session(python=DEFAULT_PYTHON_VERSION)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, do we want to run this against all versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We normally only run it on 1 version but we can expand it if there is value

def pytype(session):
"""Run type-checking."""
session.install(".[grpc]", "pytype >= 2019.3.21")
session.install(".[grpc]", "pytype")
session.run("pytype")


Expand All @@ -178,9 +248,7 @@ def mypy(session):
session.install(
"types-setuptools",
"types-requests",
# TODO(https://github.com/googleapis/python-api-core/issues/642):
# Use the latest version of types-protobuf.
"types-protobuf<5",
"types-protobuf",
"types-mock",
"types-dataclasses",
)
Expand Down
4 changes: 2 additions & 2 deletions owlbot.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
".flake8", # flake8-import-order, layout
".coveragerc", # layout
"CONTRIBUTING.rst", # no systests
".github/workflows/unittest.yml", # exclude unittest gh action
".github/workflows/lint.yml", # exclude lint gh action
".github/workflows/unittest.yml", # exclude unittest gh action
".github/workflows/lint.yml", # exclude lint gh action
"README.rst",
]
templated_files = common.py_library(microgenerator=True, cov_level=100)
Expand Down
2 changes: 2 additions & 0 deletions pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,5 @@ filterwarnings =
ignore:.*pkg_resources is deprecated as an API:DeprecationWarning
# Remove once https://github.com/grpc/grpc/issues/35086 is fixed (and version newer than 1.60.0 is published)
ignore:There is no current event loop:DeprecationWarning
# Remove after support for Python 3.7 is dropped
ignore:After January 1, 2024, new releases of this library will drop support for Python 3.7:DeprecationWarning
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
release_status = "Development Status :: 5 - Production/Stable"
dependencies = [
"googleapis-common-protos >= 1.56.2, < 2.0.dev0",
"protobuf>=3.19.5,<5.0.0.dev0,!=3.20.0,!=3.20.1,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5",
"protobuf>=3.19.5,<6.0.0.dev0,!=3.20.0,!=3.20.1,!=4.21.0,!=4.21.1,!=4.21.2,!=4.21.3,!=4.21.4,!=4.21.5",
"proto-plus >= 1.22.3, <2.0.0dev",
"google-auth >= 2.14.1, < 3.0.dev0",
"requests >= 2.18.0, < 3.0.0.dev0",
Expand Down
Loading