[GitHub] [beam] robertwb commented on a change in pull request #12605: Copy subtransforms to output of translations.pipeline_from_stages()

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

robertwb commented on a change in pull request #12605:
URL: https://github.com/apache/beam/pull/12605#discussion_r473198225



##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/translations.py
##########
@@ -524,6 +524,22 @@ def add_parent(child, parent):
         add_parent(parent, parents.get(parent))
       components.transforms[parent].subtransforms.append(child)
 
+  copied_subtransform_ids = set()

Review comment:
       Given that the set of subtransformation is a tree (no cycles) we can get rid of this.

##########
File path: sdks/python/apache_beam/runners/portability/fn_api_runner/translations.py
##########
@@ -541,6 +557,14 @@ def add_parent(child, parent):
     components.transforms[transform_id].CopyFrom(transform)
     add_parent(transform_id, stage.parent)
 
+  if partial:

Review comment:
       I think we only want to copy sub-transforms of the transforms from line 552 above, perhaps move this logic up there? (Just a thought, maybe having the method be called (`copy_subtransforms` and having it copy (recursively) just the subtransforms could make this even easier to follow). 




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