[GitHub] [beam] yifanmai commented on a change in pull request #14160: [BEAM-11357] Add the ability to declare annotations to Python.

[GitBox <gi...@apache.org>]

yifanmai commented on a change in pull request #14160:
URL: https://github.com/apache/beam/pull/14160#discussion_r589641337



##########
File path: sdks/python/apache_beam/pipeline.py
##########
@@ -1028,6 +1032,26 @@ def __init__(
     self.parts = []  # type: List[AppliedPTransform]
     self.environment_id = environment_id if environment_id else None  # type: Optional[str]
 
+    if annotations is None and transform:
+
+      def annotation_to_bytes(key, a: Any) -> bytes:
+        if isinstance(a, bytes):
+          return a
+        elif isinstance(a, str):
+          return a.encode('ascii')

Review comment:
       Why is this ASCII instead of UTF-8?

##########
File path: sdks/python/apache_beam/pipeline.py
##########
@@ -1028,6 +1032,26 @@ def __init__(
     self.parts = []  # type: List[AppliedPTransform]
     self.environment_id = environment_id if environment_id else None  # type: Optional[str]
 
+    if annotations is None and transform:

Review comment:
       For my knowledge, why do we need to check `transform` here?

##########
File path: sdks/python/apache_beam/pipeline_test.py
##########
@@ -924,6 +924,35 @@ def test_requirements(self):
         common_urns.requirements.REQUIRES_BUNDLE_FINALIZATION.urn,
         proto.requirements)
 
+  def test_annotations(self):
+    class EmptyTransform(beam.PTransform):
+      def expand(self, pcoll):
+        return pcoll
+
+      def annotations(self):
+        return {'foo': 'some_string'}
+
+    class NonEmptyTransform(beam.PTransform):
+      def expand(self, pcoll):
+        return pcoll | beam.Map(lambda x: x)
+
+      def annotations(self):
+        return {'foo': b'some_bytes'}
+
+    p = beam.Pipeline()
+    _ = p | beam.Create([]) | EmptyTransform() | NonEmptyTransform()

Review comment:
       Add test to cover the Proto code path?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org