[Python] Implement combiner deferred side inputs by hjtran · Pull Request #35601 · apache/beam

hjtran · 2025-07-15T23:28:18Z

#19851
Implement deferred side inputs for combiners.

Side inputs aren't implemented at the python sdk worker level (i.e. operations.py does not support deferred side inputs for the combiner operations), so I've just special-cased combiners that have deferred side inputs so they're translated into ParDo version of the combiner. This prevents lifting of combiners but I think that's probably fine as it doesn't seem like this use case was all that used anyways (though Schrodinger use cases keep needing it). If it turns out combiners with deferred side inputs get used a lot, we can implement defererd side inputs at the operations.py level later.

I tested the pipeline noted in the motivating issue and can confirm it works now

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

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
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.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

github-actions · 2025-07-16T12:48:29Z

Assigning reviewers:

R: @claudevdm for label python.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

hjtran · 2025-07-16T13:17:15Z

I could use some help debugging the failing unit test. It passes locally for me but seems to fail on the GH Runner for some reason due to a grpc timeout:

grpc._channel._MultiThreadedRendezvous: <_MultiThreadedRendezvous of RPC that terminated with:
	status = StatusCode.DEADLINE_EXCEEDED
	details = "Deadline Exceeded"
	debug_error_string = "UNKNOWN:Error received from peer  {grpc_message:"Deadline Exceeded", grpc_status:4, created_time:"2025-07-16T11:40:51.901440152+00:00"}"
>
self = <apache_beam.transforms.combiners_test.CombinerWithSideInputs testMethod=test_cpk_with_windows>

    def test_cpk_with_windows(self):
      # With global window side input
>     with TestPipeline() as p:

apache_beam/transforms/combiners_test.py:1173: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
target/.tox-py311-cloud/py311-cloud/lib/python3.11/site-packages/apache_beam/pipeline.py:661: in __exit__
    self.result = self.run()
target/.tox-py311-cloud/py311-cloud/lib/python3.11/site-packages/apache_beam/testing/test_pipeline.py:119: in run
    state = result.wait_until_finish()
target/.tox-py311-cloud/py311-cloud/lib/python3.11/site-packages/apache_beam/runners/portability/portable_runner.py:568: in wait_until_finish
    raise self._runtime_exception
target/.tox-py311-cloud/py311-cloud/lib/python3.11/site-packages/apache_beam/runners/portability/portable_runner.py:574: in _observe_state
    for state_response in self._state_stream:
target/.tox-py311-cloud/py311-cloud/lib/python3.11/site-packages/grpc/_channel.py:543: in __next__
    return self._next()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <_MultiThreadedRendezvous of RPC that terminated with:
	status = StatusCode.DEADLINE_EXCEEDED
	details = "Deadline Exc...ved from peer  {grpc_message:"Deadline Exceeded", grpc_status:4, created_time:"2025-07-16T11:40:51.901440152+00:00"}"
>

    def _next(self) -> Any:
        with self._state.condition:
            if self._state.code is None:
                event_handler = _event_handler(
                    self._state, self._response_deserializer
                )
                self._state.due.add(cygrpc.OperationType.receive_message)
                operating = self._call.operate(
                    (cygrpc.ReceiveMessageOperation(_EMPTY_FLAGS),),
                    event_handler,
                )
                if not operating:
                    self._state.due.remove(cygrpc.OperationType.receive_message)
            elif self._state.code is grpc.StatusCode.OK:
                raise StopIteration()
            else:
                raise self
    
            def _response_ready():
                return self._state.response is not None or (
                    cygrpc.OperationType.receive_message not in self._state.due
                    and self._state.code is not None
                )
    
            _common.wait(self._state.condition.wait, _response_ready)
            if self._state.response is not None:
                response = self._state.response
                self._state.response = None
                return response
            elif cygrpc.OperationType.receive_message not in self._state.due:
                if self._state.code is grpc.StatusCode.OK:
                    raise StopIteration()
                elif self._state.code is not None:
>                   raise self
E                   grpc._channel._MultiThreadedRendezvous: <_MultiThreadedRendezvous of RPC that terminated with:
E                   	status = StatusCode.DEADLINE_EXCEEDED
E                   	details = "Deadline Exceeded"
E                   	debug_error_string = "UNKNOWN:Error received from peer  {grpc_message:"Deadline Exceeded", grpc_status:4, created_time:"2025-07-16T11:40:51.901440152+00:00"}"
E                   >

target/.tox-py311-cloud/py311-cloud/lib/python3.11/site-packages/grpc/_channel.py:969: _MultiThreadedRendezvous

claudevdm · 2025-07-16T15:33:39Z

sdks/python/apache_beam/transforms/core.py

+      # If the CombineFn has deferred side inputs, the python SDK
+      # doesn't implement it.
+      # Use a ParDo-based CombinePerKey instead.
+      from apache_beam.runners.direct.helper_transforms import \


Is this meant to only apply to pipelines running on direct runner?

It seems strange to import transforms from direct runner into transforms.core.

No this should apply to all runners.

I've moved it into apache_beam.transforms.combiners. Let me know if you think it should go somewhere else

codecov · 2025-07-17T02:07:51Z

Codecov Report

Attention: Patch coverage is 95.89041% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.58%. Comparing base (84d423f) to head (42d5929).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
sdks/python/apache_beam/transforms/combiners.py	95.31%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #35601      +/-   ##
============================================
+ Coverage     56.56%   56.58%   +0.02%     
  Complexity     3376     3376              
============================================
  Files          1205     1204       -1     
  Lines        183574   183701     +127     
  Branches       3458     3458              
============================================
+ Hits         103845   103955     +110     
- Misses        76410    76427      +17     
  Partials       3319     3319

Flag	Coverage Δ
python	`80.71% <95.89%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

claudevdm · 2025-07-17T23:41:19Z

sdks/python/apache_beam/transforms/core.py

+      from apache_beam.transforms.combiners import \
+        LiftedCombinePerKey
+      combine_fn, *args = args
+      return LiftedCombinePerKey(combine_fn, args, kwargs)


Maybe we should log a warning here that runner implemented CBK has been replaced due to side inputs and may affect performance? Users might not be aware this is happening.

I'm a bit split. I agree that users will probably not realize this subtle impact from using deferred side inputs with their combiner, but I think ideally there'd also be a way for a user to understand the risk and dismiss the warning.

I'm okay with whichever though, let me know if you still think we should have the warning. I suspect that in most cases even knowing that there may be a reduction in performance that it'd be difficult to actually observe (but maybe I'm underestimating the operations.py combine optimizations

I'd probably vote to leave it as is. We could warn and have a kwarg to silence the warning (or something similar), but I tend to agree that this is a rare case; while the perf implication is non-obvious, adding side inputs basically always comes with some perf cost

claudevdm · 2025-07-17T23:47:59Z

R: @damccorm

github-actions · 2025-07-17T23:49:07Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

damccorm · 2025-07-22T13:55:23Z

sdks/python/apache_beam/transforms/combiners.py

    return accumulator[0]
+
+
+class LiftedCombinePerKey(core.PTransform):


Should this be part of __all__ for imports?

I don't think so. I don't think we want to advertise this since I dont think there are cases where a user should use this CPK specifically over the general CPK

That makes sense, thanks

sdks/python/apache_beam/transforms/combiners.py

damccorm · 2025-07-22T14:06:04Z

sdks/python/apache_beam/transforms/core.py

+      from apache_beam.transforms.combiners import \
+        LiftedCombinePerKey
+      combine_fn, *args = args
+      return LiftedCombinePerKey(combine_fn, args, kwargs)


I'd probably vote to leave it as is. We could warn and have a kwarg to silence the warning (or something similar), but I tend to agree that this is a rare case; while the perf implication is non-obvious, adding side inputs basically always comes with some perf cost

Co-authored-by: Danny McCormick <[email protected]>

damccorm

LGTM, I'll merge once checks complete - thanks!

hjtran added 16 commits March 25, 2024 23:31

squash

10d4c43

revert unnecessary fn_runner change

117a182

Merge branch 'master' into combine_sideinputs

9599fdf

improve test

49fb167

add a test with streaming

63cd8e5

add streaming with matching window test

5704a43

implement streaming support

5b6fe8c

add combiner support

adffdec

implement args support

1c2decf

more rigorously test which combinefn methods are called with side inputs

3503bd4

clean up

bdeb318

use pack/unpack terminology

76036db

add an explanatory comment

2a6c952

revert old unneeded changes

abf311a

tidy

6555e6a

use temp dir for json test file

9ff318b

github-actions bot added python runners labels Jul 15, 2025

hjtran added 4 commits July 15, 2025 19:50

add combineglobally test

d26ab6f

connect args/kwargs to all combinefn methods after all

fb5cc08

enable streaming for streaming tests

469c3eb

remove streaming options

c96faea

hjtran marked this pull request as ready for review July 16, 2025 11:29

hjtran changed the title ~~Combine sideinputs~~ [Python] Implement combiner deferred side inputs Jul 16, 2025

github-actions bot added the Next Action: Reviewers label Jul 16, 2025

hjtran mentioned this pull request Jul 16, 2025

Try bumping up workflow timeouts #35608

Closed

3 tasks

claudevdm reviewed Jul 16, 2025

View reviewed changes

move liftedcombineperkey

48bfe03

hjtran added 3 commits July 16, 2025 18:17

add additional docstring to liftedcombineperkey

f4127e0

Merge branch 'master' into combine_sideinputs

440dc2a

isort

42d5929

claudevdm approved these changes Jul 17, 2025

View reviewed changes

damccorm reviewed Jul 22, 2025

View reviewed changes

hjtran and others added 2 commits July 22, 2025 10:52

Update sdks/python/apache_beam/transforms/combiners.py

ffd9ba6

Co-authored-by: Danny McCormick <[email protected]>

privatize a couple transforms

1d3488c

damccorm approved these changes Jul 22, 2025

View reviewed changes

damccorm merged commit 3ff64c7 into apache:master Jul 22, 2025
91 of 92 checks passed

		return accumulator[0]


		class LiftedCombinePerKey(core.PTransform):

Conversation

hjtran commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GitHub Actions Tests Status (on master branch)

Uh oh!

github-actions bot commented Jul 16, 2025

Uh oh!

hjtran commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claudevdm commented Jul 17, 2025

Uh oh!

github-actions bot commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damccorm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hjtran commented Jul 15, 2025 •

edited

Loading

codecov bot commented Jul 17, 2025 •

edited

Loading