fix: add support for protobuf 5.x #644

parthea · 2024-04-27T13:20:27Z

Fixes #628 🦕
Fixes #641 🦕
Fixes #642 🦕

Regarding the including_default_value_fields argument of json_format.MessageToDict() in protobuf:

In protobuf version 3.19.6 - The default value of including_default_value_fields was False
https://github.com/protocolbuffers/protobuf/blob/5cba162a5d93f8df786d828621019e03e50edb4f/python/google/protobuf/json_format.py#L92

At the time that the argument was removed in protobuf 5.x - the default value of including_default_value_fields was False
protocolbuffers/protobuf@2699579#diff-8de817c14d6a087981503c9aea38730b1b3e98f4e306db5ff9d525c7c304f234

IOW, setting argument including_default_value_fields to False had no effect as it was the default behaviour.

The Unit tests / unit_prerelease-3.12 presubmit check was added to run tests against the latest pre-release version of protobuf as several dependencies have a constraint on protobuf<5

anndro · 2024-05-16T08:53:33Z

Hello @parthea is there any scheduled date for release this pull request?

zhangskz · 2024-05-30T16:36:03Z

@parthea Is there anything blocking this PR still?

vchudnov-g

A couple of suggestions, and some questions, but nothing major. This looks good!

vchudnov-g · 2024-05-31T17:54:09Z

google/api_core/operations_v1/transports/rest.py

+        # 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)


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)

Fixed in 5a49a53

noxfile.py

vchudnov-g · 2024-05-31T18:11:10Z

noxfile.py

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


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?

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.

python-api-core/testing/constraints-3.7.txt

Lines 1 to 7 in 126b5c7

# 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

python-api-core/noxfile.py

Lines 79 to 88 in 7fbce0d

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)

vchudnov-g · 2024-05-31T18:15:54Z

noxfile.py

@@ -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):


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.)

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.

vchudnov-g · 2024-05-31T18:25:32Z

noxfile.py

 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):


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.

Fixed in e1efa83

vchudnov-g · 2024-05-31T18:48:55Z

.github/workflows/unittest.yml

@@ -11,7 +11,7 @@ jobs:
    runs-on: ubuntu-latest
    strategy:
      matrix:
-        option: ["", "_grpc_gcp", "_wo_grpc"]
+        option: ["", "_grpc_gcp", "_wo_grpc", "_prerelease"]


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

This comment is obsolete with the changes in e1efa83

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…gleapis/python-api-core into add-support-for-protobuf-5-x

parthea · 2024-06-11T20:07:10Z

@ohmayr PTAL

ohmayr · 2024-06-12T16:56:41Z

google/api_core/operations_v1/__init__.py

+from google.api_core.operations_v1.abstract_operations_client import (
+    AbstractOperationsClient,
+)


Why is this file changed?

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

ohmayr · 2024-06-12T17:06:01Z

noxfile.py

@@ -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)


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

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

ohmayr · 2024-06-12T17:24:23Z

google/api_core/operations_v1/transports/rest.py

@@ -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,
        )


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!

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.

JackCloudman · 2024-06-17T06:19:54Z

Hello, I am eagerly anticipating the next release! Could you please provide any updates on the release date? Thank the whole team for your hard work and dedication.

product-auto-label bot added the size: m Pull request size is medium. label Apr 27, 2024

parthea force-pushed the add-support-for-protobuf-5-x branch 7 times, most recently from f8f719b to a650198 Compare April 27, 2024 15:11

fix: add support for protobuf 5.x

ac9cad8

parthea force-pushed the add-support-for-protobuf-5-x branch from a650198 to ac9cad8 Compare April 27, 2024 15:12

Merge branch 'main' into add-support-for-protobuf-5-x

a62ea6e

parthea marked this pull request as ready for review April 29, 2024 14:36

parthea requested review from a team as code owners April 29, 2024 14:36

blunderbuss-gcf bot assigned ohmayr Apr 29, 2024

remove pin for types-protobuf

13e1567

parthea added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 29, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 29, 2024

remove pytest from noxfile

8b6bc48

bourgeoisor mentioned this pull request May 21, 2024

chore(deps): update dependency requests to v2.32.0 [security] - autoclosed GoogleCloudPlatform/microservices-demo#2557

Closed

1 task

Merge branch 'main' into add-support-for-protobuf-5-x

583b8ce

parthea assigned vchudnov-g May 30, 2024

parthea requested review from vchudnov-g and ohmayr May 30, 2024 17:24

vchudnov-g reviewed May 31, 2024

View reviewed changes

parthea assigned parthea and unassigned vchudnov-g and ohmayr Jun 4, 2024

parthea mentioned this pull request Jun 4, 2024

chore(main): release 2.19.1 #650

Open

veblush mentioned this pull request Jun 4, 2024

[Dep] Upgrading Protobuf to v27.0 grpc/grpc#36753

Open

parthea and others added 8 commits June 11, 2024 19:40

refactor common code

5a49a53

Refactor

3bca72c

Co-authored-by: Victor Chudnovsky <vchudnov@google.com>

run black

f263d69

🦉 Updates from OwlBot post-processor

2396495

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Merge branch 'add-support-for-protobuf-5-x' of https://github.com/goo…

ef6050f

…gleapis/python-api-core into add-support-for-protobuf-5-x

mypy

7fbce0d

run pre-release test against all python versions

e1efa83

filter warning

28a41f8

parthea assigned ohmayr and unassigned parthea Jun 11, 2024

ohmayr approved these changes Jun 12, 2024

View reviewed changes

parthea assigned parthea and unassigned ohmayr Jun 19, 2024

Merge branch 'main' into add-support-for-protobuf-5-x

0037d7a

parthea enabled auto-merge (squash) June 19, 2024 15:21

parthea merged commit fda0ca6 into main Jun 19, 2024
31 checks passed

parthea deleted the add-support-for-protobuf-5-x branch June 19, 2024 15:22

parthea mentioned this pull request Jun 19, 2024

chore: Release candidate v2.19.1rc0 #672

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add support for protobuf 5.x #644

fix: add support for protobuf 5.x #644

parthea commented Apr 27, 2024 •

edited

anndro commented May 16, 2024

zhangskz commented May 30, 2024

vchudnov-g left a comment

vchudnov-g May 31, 2024

parthea Jun 11, 2024

vchudnov-g May 31, 2024

parthea Jun 11, 2024

vchudnov-g May 31, 2024

parthea Jun 11, 2024

vchudnov-g May 31, 2024

parthea Jun 11, 2024

vchudnov-g May 31, 2024

parthea Jun 11, 2024

parthea commented Jun 11, 2024

ohmayr Jun 12, 2024

parthea Jun 12, 2024

ohmayr Jun 12, 2024

parthea Jun 12, 2024

ohmayr Jun 12, 2024

parthea Jun 19, 2024

JackCloudman commented Jun 17, 2024

	# 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
	)
	]

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

fix: add support for protobuf 5.x #644

fix: add support for protobuf 5.x #644

Conversation

parthea commented Apr 27, 2024 • edited

anndro commented May 16, 2024

zhangskz commented May 30, 2024

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthea commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackCloudman commented Jun 17, 2024

parthea commented Apr 27, 2024 •

edited