[GitHub] [beam] ryanthompson591 commented on a diff in pull request #17465: Add element weighting parameter to BatchElements.

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

ryanthompson591 commented on code in PR #17465:
URL: https://github.com/apache/beam/pull/17465#discussion_r860311591


##########
sdks/python/apache_beam/transforms/util.py:
##########
@@ -632,6 +649,8 @@ def __init__(
       max_batch_size=10000,
       target_batch_overhead=.05,
       target_batch_duration_secs=1,
+      *,

Review Comment:
   why * here?



##########
sdks/python/apache_beam/transforms/util_test.py:
##########
@@ -228,6 +228,16 @@ def test_windowed_batches(self):
               7,  # elements in [30, 47)
           ]))
 
+  def test_sized_batches(self):

Review Comment:
   I think when someone looks at this in the future and says, "why is this here", you could illustrate it better with a test like this (I'm not sure if this works but you get the point).
   
   def test_element_size_fn()
   """ Tests passing different sized arrays as elements and then weighting them by how large the arrays are. Batch sizes will depend on the size of the arrays within."""
   ...
   | beam.Create([[1]*1, [1] *1, [10] *10, [6] * 6 ...], reshuffle=False)
   | util.BatchElements(min_batch_size =10, max_batch_size=10, element_size_fn=lambda x: len(x))
   | beam.Map(len)
   assert_that(res, equal_to([12, 11, 10]))
   ...
   
   I think that might just make it clearer to the future reader as to why this feature might exist.  Feel free to take or ignore this suggestion.



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

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

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