yifanmai commented on a change in pull request #13851:
URL: https://github.com/apache/beam/pull/13851#discussion_r567175544
##########
File path:
sdks/python/apache_beam/runners/portability/fn_api_runner/translations_test.py
##########
@@ -227,6 +232,53 @@ def assert_is_topologically_sorted(transform_id,
visited_pcolls):
assert_is_topologically_sorted(
optimized_pipeline_proto.root_transform_ids[0], set())
+ @attr('ValidatesRunner')
+ def test_run_packable_combine_per_key(self):
+ class MultipleCombines(beam.PTransform):
+ def expand(self, pcoll):
+ assert_that(
Review comment:
Yes this would test without combiner packing. This test serves a
different purpose; it tests that packable combines can be run successfully with
correct outputs, whether or not the packing actually happens. I updated the
comment to clarify this.
The test that combiner packing was triggered has to be runner specific
because not every runner supports combiner packing yet:
- DataflowRunner has a test
[here](https://github.com/apache/beam/blob/a44768f074ad0c55d5ca19c8ec6a47887b7e8538/sdks/python/apache_beam/runners/dataflow/dataflow_runner_test.py#L852-L883)
but only checks for it under an experiment flag.
- FnApiRunner has a test
[here](https://github.com/apache/beam/blob/a44768f074ad0c55d5ca19c8ec6a47887b7e8538/sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner_test.py#L1037-L1040)
but it is disabled because combiner packing doesn't actually work for
PortableRunner and its subclasses yet.
Speaking of experiment flags, ValidatesRunner has the downside of only
testing the case with no experiment flags. So it won't do anything useful until
combiner packing is enabled by default by each runner.
----------------------------------------------------------------
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:
[email protected]
