[BEAM-13052] Implement ProtoPlusCoder and add it to the default options #15817

dpcollins-google · 2021-10-27T18:42:08Z

Google cloud python client libraries use proto-plus for their protocol message wrappers, this coder enables them to be used in beam pipelines

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

dpcollins-google · 2021-10-27T18:42:20Z

R: @chamikaramj

chamikaramj · 2021-10-27T18:43:05Z

cc: @robertwb

codecov · 2021-10-27T19:15:04Z

Codecov Report

Merging #15817 (5e21f43) into master (03d4e42) will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #15817      +/-   ##
==========================================
+ Coverage   83.53%   83.61%   +0.07%     
==========================================
  Files         445      445              
  Lines       61385    61356      -29     
==========================================
+ Hits        51279    51301      +22     
+ Misses      10106    10055      -51

Impacted Files	Coverage Δ
sdks/python/apache_beam/utils/interactive_utils.py	`90.24% <0.00%> (-4.88%)`	⬇️
...eam/portability/api/beam_expansion_api_pb2_grpc.py	`57.89% <0.00%> (-4.02%)`	⬇️
...eam/portability/api/beam_provision_api_pb2_grpc.py	`73.68% <0.00%> (-2.51%)`	⬇️
...hon/apache_beam/runners/direct/test_stream_impl.py	`94.02% <0.00%> (-2.24%)`	⬇️
...e_beam/portability/api/beam_runner_api_pb2_grpc.py	`78.94% <0.00%> (-2.01%)`	⬇️
sdks/python/apache_beam/internal/metrics/metric.py	`90.00% <0.00%> (-1.00%)`	⬇️
...pache_beam/runners/interactive/interactive_beam.py	`75.60% <0.00%> (-0.98%)`	⬇️
...ache_beam/portability/api/beam_job_api_pb2_grpc.py	`56.04% <0.00%> (-0.95%)`	⬇️
...beam/portability/api/beam_artifact_api_pb2_grpc.py	`56.04% <0.00%> (-0.95%)`	⬇️
sdks/python/apache_beam/io/textio.py	`96.78% <0.00%> (-0.57%)`	⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10d9594...5e21f43. Read the comment docs.

chamikaramj

Thanks!

chamikaramj · 2021-11-01T19:19:31Z

sdks/python/apache_beam/coders/coder_impl.py

+    return value._pb.SerializePartialToString(deterministic=True)
+
+  def decode(self, value):
+    return self.proto_plus_type.deserialize(value)


To confirm, the API to be used here is different from ProtoCoder above (which uses 'ParseFromString') ?

This is the preferred way to deserialize proto plus types. I can't use the serialize method on line 321 because it is not deterministic, so must drop down to the protobuf interface to do so.

chamikaramj · 2021-11-01T19:20:20Z

sdks/python/apache_beam/coders/coder_impl.py

@@ -310,6 +311,19 @@ def encode(self, value):
    return value.SerializePartialToString(deterministic=True)


+class ProtoPlusCoderImpl(SimpleCoderImpl):


Do you think this could affect the performance characteristics of existing Python GCP connectors ?

It shouldn't- either they're currently not resolving a coder (and broken) or theyre already registering a custom coder (and this is ignored) or they're not using proto-plus types in their interfaces.

sdks/python/apache_beam/coders/coders.py

chamikaramj · 2021-11-01T19:24:39Z

sdks/python/apache_beam/coders/coders.py

+
+  @classmethod
+  def from_type_hint(cls, typehint, unused_registry):
+    if issubclass(typehint, proto.Message):


To clarify, currently such messages get encoded using the default coder ?

Yes. Currently they fall back to picklecoder and fail to serialize the state.

chamikaramj · 2021-11-01T19:25:39Z

sdks/python/apache_beam/coders/coders_test.py

+
+
+class ProtoPlusCoderTest(unittest.TestCase):
+  def test_proto_plus_coder(self):


Should we also add tests that include more complex field types (for example, maps) ?

See line 140, this is tested.

chamikaramj · 2021-11-03T17:25:40Z

Run PythonDocker PreCommit

chamikaramj · 2021-11-03T17:25:49Z

Run Portable_Python PreCommit

chamikaramj · 2021-11-05T17:57:44Z

Run Portable_Python PreCommit

chamikaramj · 2021-11-05T17:57:53Z

Run PythonDocker PreCommit

chamikaramj · 2021-11-05T18:02:04Z

Run Portable_Python PreCommit

chamikaramj · 2021-11-10T00:29:18Z

Run PythonDocker PreCommit

chamikaramj · 2021-11-10T00:29:26Z

Run PythonDocker PreCommit

chamikaramj · 2021-11-10T17:29:54Z

Seems like this is blocked by a dependency update ?

16:45:43 ERROR: Cannot install -r /tmp/base_image_requirements.txt (line 37) and protobuf==3.17.3 because these package versions have conflicting dependencies.
16:45:43
16:45:43 The conflict is caused by:
16:45:43 The user requested protobuf==3.17.3
16:45:43 proto-plus 1.19.7 depends on protobuf>=3.19.0
16:45:43

Google cloud python client libraries use proto-plus for their protocol message wrappers, this coder enables them to be used in beam pipelines

chamikaramj

Thanks.

sdks/python/apache_beam/coders/typecoders.py

chamikaramj · 2021-11-12T02:10:39Z

Thanks. LGTM.

chamikaramj · 2021-11-12T02:10:49Z

cc: @tvalentyn

chamikaramj · 2021-11-12T02:12:02Z

Run Python PostCommit 3.8

chamikaramj · 2021-11-12T02:13:15Z

Run Python 3.8 PostCommit

chamikaramj · 2021-11-12T17:45:49Z

PostCommit failure is unrelated (https://issues.apache.org/jira/browse/BEAM-13218)

…lt options (apache#15817)" on 2.35.0 branch. This reverts commit be22467.

chamikaramj reviewed Nov 1, 2021

View reviewed changes

dpcollins-google added 5 commits November 10, 2021 23:11

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options

936bfda

Google cloud python client libraries use proto-plus for their protocol message wrappers, this coder enables them to be used in beam pipelines

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options

d2e94ec

Google cloud python client libraries use proto-plus for their protocol message wrappers, this coder enables them to be used in beam pipelines

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options

5f1de7f

Google cloud python client libraries use proto-plus for their protocol message wrappers, this coder enables them to be used in beam pipelines

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options

cb4b96b

Google cloud python client libraries use proto-plus for their protocol message wrappers, this coder enables them to be used in beam pipelines

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options

b7bfcf3

Google cloud python client libraries use proto-plus for their protocol message wrappers, this coder enables them to be used in beam pipelines

dpcollins-google force-pushed the proto-plus-coder branch from f6670ad to b7bfcf3 Compare November 11, 2021 04:12

chamikaramj reviewed Nov 11, 2021

View reviewed changes

sdks/python/apache_beam/coders/typecoders.py Show resolved Hide resolved

sdks/python/apache_beam/coders/typecoders.py Outdated Show resolved Hide resolved

Roll back CoderRegistry __init__

5e21f43

chamikaramj merged commit be22467 into apache:master Nov 12, 2021

tvalentyn added a commit to tvalentyn/beam that referenced this pull request Nov 19, 2021

Revert "[BEAM-13052] Implement ProtoPlusCoder and add it to the defau…

9f007bf

…lt options (apache#15817)" on 2.35.0 branch. This reverts commit be22467.

tvalentyn mentioned this pull request Dec 21, 2021

[BEAM-13388] [BEAM-13052] Cherry-pick ProtoCoder implementation onto 2.35.0 release branch. #16301

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options #15817

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options #15817

dpcollins-google commented Oct 27, 2021

dpcollins-google commented Oct 27, 2021

chamikaramj commented Oct 27, 2021

codecov bot commented Oct 27, 2021 •

edited

Loading

chamikaramj left a comment

chamikaramj Nov 1, 2021

dpcollins-google Nov 1, 2021

chamikaramj Nov 1, 2021

dpcollins-google Nov 1, 2021

chamikaramj Nov 1, 2021

dpcollins-google Nov 1, 2021

chamikaramj Nov 1, 2021

dpcollins-google Nov 1, 2021

chamikaramj commented Nov 3, 2021

chamikaramj commented Nov 3, 2021

chamikaramj commented Nov 5, 2021

chamikaramj commented Nov 5, 2021

chamikaramj commented Nov 5, 2021

chamikaramj commented Nov 10, 2021

chamikaramj commented Nov 10, 2021

chamikaramj commented Nov 10, 2021

chamikaramj left a comment

chamikaramj commented Nov 12, 2021

chamikaramj commented Nov 12, 2021

chamikaramj commented Nov 12, 2021

chamikaramj commented Nov 12, 2021

chamikaramj commented Nov 12, 2021

		@@ -310,6 +311,19 @@ def encode(self, value):
		return value.SerializePartialToString(deterministic=True)


		class ProtoPlusCoderImpl(SimpleCoderImpl):



		class ProtoPlusCoderTest(unittest.TestCase):
		def test_proto_plus_coder(self):

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options #15817

[BEAM-13052] Implement ProtoPlusCoder and add it to the default options #15817

Conversation

dpcollins-google commented Oct 27, 2021

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

dpcollins-google commented Oct 27, 2021

chamikaramj commented Oct 27, 2021

codecov bot commented Oct 27, 2021 • edited Loading

Codecov Report

chamikaramj 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

chamikaramj commented Nov 3, 2021

chamikaramj commented Nov 3, 2021

chamikaramj commented Nov 5, 2021

chamikaramj commented Nov 5, 2021

chamikaramj commented Nov 5, 2021

chamikaramj commented Nov 10, 2021

chamikaramj commented Nov 10, 2021

chamikaramj commented Nov 10, 2021

chamikaramj left a comment

Choose a reason for hiding this comment

chamikaramj commented Nov 12, 2021

chamikaramj commented Nov 12, 2021

chamikaramj commented Nov 12, 2021

chamikaramj commented Nov 12, 2021

chamikaramj commented Nov 12, 2021

`ValidatesRunner` compliance status (on master branch)

codecov bot commented Oct 27, 2021 •

edited

Loading