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
12 changes: 11 additions & 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", "_prerelease"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this more succinct code should work (I haven't tested it) and would be easier to maintain.
Ref: https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations

        option: ["", "_grpc_gcp", "_wo_grpc"]
        python:
          - "3.7"
          - "3.8"
          - "3.9"
          - "3.10"
          - "3.11"
          - "3.12"
        exclude:
          - option: "_wo_grpc"
            python: 3.7
          - option: "_wo_grpc"
            python: 3.8
          - option: "_wo_grpc"
            python: 3.9
         include:
          - option: "_prerelease"
            python: 3.12

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment is obsolete with the changes in e1efa83

python:
- "3.7"
- "3.8"
Expand All @@ -26,6 +26,16 @@ jobs:
python: 3.8
- option: "_wo_grpc"
python: 3.9
- option: "_prerelease"
python: 3.7
- option: "_prerelease"
python: 3.8
- option: "_prerelease"
python: 3.9
- option: "_prerelease"
python: 3.10
- option: "_prerelease"
python: 3.11
steps:
- name: Checkout
uses: actions/checkout@v4
Expand Down
91 changes: 66 additions & 25 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 @@ -184,11 +188,22 @@ def _list_operations(
"google.longrunning.Operations.ListOperations"
]

request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True,
)
# 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."]:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True, # type: ignore # backward compatibility
)
else:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
always_print_fields_with_no_presence=True,
)

transcoded_request = path_template.transcode(http_options, **request_kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

You have this block repeated in four places. I suggest factoring it out into a helper function that only needs to take one parameter:

Suggested change
# 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."]:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True, # type: ignore # backward compatibility
)
else:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
always_print_fields_with_no_presence=True,
)
transcoded_request = path_template.transcode(http_options, **request_kwargs)
transcoded_request = _transcode_request(request)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5a49a53


uri = transcoded_request["uri"]
Expand All @@ -199,7 +214,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 +279,22 @@ def _get_operation(
"google.longrunning.Operations.GetOperation"
]

request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True,
)
# 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."]:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True, # type: ignore # backward compatibility
)
else:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
always_print_fields_with_no_presence=True,
)

transcoded_request = path_template.transcode(http_options, **request_kwargs)

uri = transcoded_request["uri"]
Expand All @@ -280,7 +305,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 +363,21 @@ def _delete_operation(
"google.longrunning.Operations.DeleteOperation"
]

request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True,
)
# 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."]:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True, # type: ignore # backward compatibility
)
else:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
always_print_fields_with_no_presence=True,
)
transcoded_request = path_template.transcode(http_options, **request_kwargs)

uri = transcoded_request["uri"]
Expand All @@ -354,7 +388,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 +444,28 @@ def _cancel_operation(
"google.longrunning.Operations.CancelOperation"
]

request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True,
)
# 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."]:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
including_default_value_fields=True, # type: ignore # backward compatibility
)
else:
request_kwargs = json_format.MessageToDict(
request,
preserving_proto_field_name=True,
always_print_fields_with_no_presence=True,
)
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 +477,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 Down
105 changes: 89 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,41 @@ 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)
if install_grpc:
session.install(
"-e",
".[grpc]",
"-c",
f"{constraints_dir}/constraints-{session.python}.txt",
)
else:
session.install(
"-e", ".", "-c", f"{constraints_dir}/constraints-{session.python}.txt"
)
parthea marked this conversation as resolved.
Show resolved Hide resolved

# 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 +194,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[-1])
def unit_prerelease(session):
Copy link
Contributor

Choose a reason for hiding this comment

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

So this tests running against the pre-release versions of the dependencies, right? And we only bother to do this with the latest Python run-time.

A comment might be helpful. Also, my personal preference would to rename this (and similar code elsewhere) as unit_with_prerelease_deps. Every time I read something like unit_prerelease my mind first assumes we're testing the pre-release version of this library, not of its dependencies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in e1efa83

"""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 +225,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 +239,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 +253,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
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