[Python] Add Watch transform with growth_of polling SDF#39023
[Python] Add Watch transform with growth_of polling SDF#39023Eliaaazzz wants to merge 3 commits into
Conversation
0cd6937 to
5b039a0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39023 +/- ##
============================================
- Coverage 55.16% 54.15% -1.01%
Complexity 1676 1676
============================================
Files 1068 1071 +3
Lines 167265 166847 -418
Branches 1208 1208
============================================
- Hits 92265 90352 -1913
- Misses 72816 74311 +1495
Partials 2184 2184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5b039a0 to
9acab87
Compare
Abacn
left a comment
There was a problem hiding this comment.
Thanks, went through watch.py changes
| pending: PollResult | ||
|
|
||
|
|
||
| _GrowthState = Any # Union[_PollingGrowthState, _NonPollingGrowthState] |
There was a problem hiding this comment.
What's the point of making an Any alias?
Would it be preferred to make it an empty dataclass and make _PollingGrowthState, _NonPollingGrowthState inherit it?
There was a problem hiding this comment.
That is a cleaner design, thank you. I replaced the Any alias with an empty _GrowthState base class and had both restriction states inherit it, so it now reads as a real union type.
| # ------------------------------------------------------------------------------ | ||
|
|
||
|
|
||
| class _HashCode128Coder(Coder): |
There was a problem hiding this comment.
Python coders are not implemented identically as Java. In Java we have HashCode128Coder to write fix size 16 bytes. In Python encode() encodes to bytes, and will be length-prefixed in the raw stream.
Just use Beam's BytesCoder may have a better performance (reduced a layer of wrapper) while the length check can be done elsewhere
There was a problem hiding this comment.
You have a point that the wrapper adds little in Python. I switched to BytesCoder, and since the digest is always 16 bytes from blake2b, I dropped the length guard as redundant.
| return True | ||
|
|
||
|
|
||
| class _TimestampedValueCoder(Coder): |
There was a problem hiding this comment.
The Python SDK ships no coder for
TimestampedValue
A little bit surprising. Likely due to Python SDK has some special handling:
beam/sdks/python/apache_beam/runners/common.py
Line 1773 in a86e611
converting TimestampedValue to a WindowedValue, and WindowedValue has a standard coder
I'm considering make TimestampedValue a public interface like Java SDK we current have (could be a separate PR)
There was a problem hiding this comment.
Thank you for the explanation, that helps a lot. Since TimestampedValue gets unwrapped into a WindowedValue, I reworded the docstring to say the coder only exists to keep it inside the restriction state. Please let me know if you would like me to build on your public coder PR once it lands.
| output_coder: Optional[Coder] = None, | ||
| now_fn: Optional[Callable[[], float]] = None): | ||
| super().__init__() | ||
| self._poll_fn = poll_fn |
There was a problem hiding this comment.
We should be able to wrap the poll_fn with a PollFn if it's typehint is compatible; or we should be able to derive typehints for the callables in expand beneath.
There was a problem hiding this comment.
Thank you, I took another pass at this. In expand I now derive the output element type from the input type and the resolved coder and set it with with_output_types, so the (input, output) pairs stay typed downstream. A PollFn can still declare its coder through default_output_coder, and I added a test that checks the derived type.
| """Optional base for a poll function ``input -> PollResult``. | ||
|
|
||
| Any callable with that signature works; subclass only to attach an output | ||
| coder hint via :meth:`default_output_coder`. |
There was a problem hiding this comment.
As it's declared in __all__, consider add an example about creating a PollFn that is callable and has default_output_coder implemented
There was a problem hiding this comment.
That is a helpful suggestion, thank you. I added a short example to the PollFn docstring that shows a callable alongside a default_output_coder implementation.
| output_coder = hint or coders.PickleCoder() | ||
| if not output_coder.is_deterministic(): | ||
| _LOGGER.warning( | ||
| 'Watch dedup uses a non-deterministic output coder (%s); equal ' |
There was a problem hiding this comment.
This warning message needs more consideration --- if it's statement is true we should raise here, as the whole point of the Watch.growth is that it only emits newly seen elements.
I think as long as key is deterministic it should be fine.
In Java it made a distinction between "output" and "outputKey", as well as its own Fn and coders. KeyCoder needs to be deterministic.
I see Python implementation has a key_coder as well. Would check key coder be sufficient?
There was a problem hiding this comment.
I agree that a warning is too soft here. I changed expand to raise when the resolved coder is non-deterministic. The dedup key is the encoded output, and the key coder is the output coder in this version, so this single check covers the key. I also updated the end-to-end tests to pass a deterministic coder and added a test for the new error.
|
|
||
| element = holder[0] | ||
| now = Timestamp.of(self._now()) | ||
| result = self._poll_fn(element) |
There was a problem hiding this comment.
put poll inside try_claim is not recommended as long running try_claim could cause issues (see #36750, it's fixed in Java, not sure if it could also happens in Python, probably worth testing)
poll can take very long if it makes API calls (e.g. list all files in gcs). Would it be possible to put it inside DoFn's process element?
There was a problem hiding this comment.
This was an important point, thank you for raising it. I moved the poll into process ahead of the claim, so it no longer runs while the tracker lock is held. The dedup and checkpoint logic now sits in a small pure function, and try_claim simply records the primary and residual. I reran the tests and lint afterward and both pass.
9acab87 to
5a54aa6
Compare
|
Thanks for the thorough review. I addressed all the comments and pushed. PTAL when you have time. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces an experimental Watch transform to the Apache Beam Python SDK, designed to handle periodic polling of sources. It provides a robust mechanism for watching growing inputs, deduplicating results, and managing watermarks in an unbounded PCollection. The implementation leverages splittable DoFns to ensure polling remains durable across bundles and worker restarts, with comprehensive support for custom termination policies. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces an experimental Watch transform for the Apache Beam Python SDK, which continuously polls for new outputs per input element using a Splittable DoFn. The review feedback highlights critical issues with watermark estimation during both the replay and active polling phases. Specifically, unconditionally setting the watermark to the input element's timestamp before yielding outputs can prematurely advance the watermark and cause outputs to be treated as late data. The reviewer recommends holding the watermark at the minimum of the input timestamp and the planned watermark before yielding, and releasing the watermark hold to MAX_TIMESTAMP upon termination to prevent the pipeline watermark from getting stuck.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if isinstance(restriction, _NonPollingGrowthState): | ||
| # Replay the outputs already emitted this round, then stop. No poll. | ||
| if not tracker.try_claim(_replay_plan(restriction)): | ||
| return | ||
| _set_watermark_if_greater(watermark_estimator, timestamp) | ||
| for output in restriction.pending.outputs: | ||
| yield TimestampedValue((element, output.value), output.timestamp) | ||
| return |
There was a problem hiding this comment.
In the replay block for _NonPollingGrowthState, the watermark is unconditionally set to timestamp before yielding the outputs. If the pending watermark (restriction.pending.watermark) is less than timestamp, this will advance the watermark past the outputs' timestamps, causing them to be treated as late data. Additionally, since this is a terminal state that stops polling, the watermark hold must be released to MAX_TIMESTAMP after yielding the outputs to prevent the pipeline watermark from being permanently stuck.
if isinstance(restriction, _NonPollingGrowthState):\n # Replay the outputs already emitted this round, then stop. No poll.\n if not tracker.try_claim(_replay_plan(restriction)):\n return\n watermark = restriction.pending.watermark\n if watermark is not None:\n _set_watermark_if_greater(watermark_estimator, min(timestamp, watermark))\n else:\n _set_watermark_if_greater(watermark_estimator, timestamp)\n for output in restriction.pending.outputs:\n yield TimestampedValue((element, output.value), output.timestamp)\n _set_watermark_if_greater(watermark_estimator, MAX_TIMESTAMP)\n returnThere was a problem hiding this comment.
Could you please check these two comments?
There was a problem hiding this comment.
Thanks for pointing these out. The block quoted here was rewritten in c559963, so the code no longer looks like this.
The replay branch now leaves the watermark estimator untouched. It claims the pending state, replays the outputs and returns, at watch.py lines 566 to 572. Nothing advances the watermark ahead of the replayed outputs.
On releasing the hold to MAX_TIMESTAMP, I traced the runner before deciding. The estimator state is only read when an invocation produces a deferred residual, in runners/common.py around lines 1064 to 1082, and delayed applications and output watermarks are built only from residuals in bundle_processor.py. A terminal round produces no residual, so its last watermark is never reported as a hold and cannot pin the stage. The DirectRunner releases explicitly when there is no residual, in sdf_direct_runner.py around lines 367 to 374.
I added a test in aeafcb2, test_replay_round_leaves_the_watermark_alone, which pins that the replay round keeps the watermark at the seed and reports no residual.
Happy to revisit if you would rather have the explicit release.
| # Seed the watermark hold from the input event time after the claim. | ||
| _set_watermark_if_greater(watermark_estimator, timestamp) | ||
| for output in plan.emit: | ||
| yield TimestampedValue((element, output.value), output.timestamp) | ||
| if plan.stop: | ||
| # The input is finished, so release the watermark hold to MAX. | ||
| _set_watermark_if_greater(watermark_estimator, MAX_TIMESTAMP) | ||
| return | ||
| if plan.watermark is not None: | ||
| _set_watermark_if_greater(watermark_estimator, plan.watermark) | ||
| tracker.defer_remainder(self._poll_interval) |
There was a problem hiding this comment.
Unconditionally setting the watermark to timestamp before yielding the outputs can cause correctness issues. If plan.watermark is less than timestamp, setting the watermark to timestamp first will advance the watermark past the outputs' timestamps, causing them to be treated as late data. To prevent this, the watermark should be held at min(timestamp, plan.watermark) (if plan.watermark is not None) before yielding, and then advanced to plan.watermark after yielding.
| # Seed the watermark hold from the input event time after the claim. | |
| _set_watermark_if_greater(watermark_estimator, timestamp) | |
| for output in plan.emit: | |
| yield TimestampedValue((element, output.value), output.timestamp) | |
| if plan.stop: | |
| # The input is finished, so release the watermark hold to MAX. | |
| _set_watermark_if_greater(watermark_estimator, MAX_TIMESTAMP) | |
| return | |
| if plan.watermark is not None: | |
| _set_watermark_if_greater(watermark_estimator, plan.watermark) | |
| tracker.defer_remainder(self._poll_interval) | |
| # Seed the watermark hold from the input event time after the claim.\n if plan.watermark is not None:\n _set_watermark_if_greater(watermark_estimator, min(timestamp, plan.watermark))\n else:\n _set_watermark_if_greater(watermark_estimator, timestamp)\n for output in plan.emit:\n yield TimestampedValue((element, output.value), output.timestamp)\n if plan.stop:\n # The input is finished, so release the watermark hold to MAX.\n _set_watermark_if_greater(watermark_estimator, MAX_TIMESTAMP)\n return\n if plan.watermark is not None:\n _set_watermark_if_greater(watermark_estimator, plan.watermark)\n tracker.defer_remainder(self._poll_interval) |
There was a problem hiding this comment.
Same here, this path was rewritten in c559963.
The per round watermark advance now happens after the outputs are emitted. Outputs are yielded at watch.py line 589 and the advance is at line 603, so the watermark never moves ahead of what was just emitted.
The only write before the yields is a single seed from the element timestamp, applied once while the estimator is still unset, at watch.py lines 563 to 564. Without it the stage reports a watermark of None, which a runner reads as the minimum timestamp and holds there until the first output.
I added a test in aeafcb2, test_terminal_round_after_deferring_leaves_no_residual. It defers twice, carries the estimator state across both resumes, then completes, and asserts the watermark stays where the last poll left it while the terminal round reports no residual.
I also checked the tests against deliberate regressions. Re-adding the MAX release, writing the watermark inside the replay block, or dropping the seed guard each make them fail.
Please let me know if you would like anything tightened.
|
Assigning reviewers: R: @shunping for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Add an experimental Watch transform that watches a growing set of outputs per input element. Watch.growth_of(poll_fn) runs a periodic poll loop as a splittable DoFn and emits an unbounded PCollection of (input, output) pairs. Each process() performs one poll round and self-checkpoints via defer_remainder. New outputs are deduplicated with a stable 128-bit blake2b hash of the encoded output, and a manual watermark estimator advances per poll. Per-input termination supports never() and after_total_of(); polling also stops when a poll returns PollResult.complete(). The DoFn is its own RestrictionProvider, and restriction state serializes through a tagged GrowthState coder. Tests cover termination conditions, coder round-trips, the restriction tracker claim/checkpoint/dedup logic, and DirectRunner end-to-end runs.
5a54aa6 to
1eb9284
Compare
Abacn
left a comment
There was a problem hiding this comment.
Thanks, the changes seem to raising a few more questions, commented below
Please do not force push after PR received initial review, unless there is a merge conflict (even so preserve the number of commits during rebase)
| spec.update(changes) | ||
| return Watch(**spec) | ||
|
|
||
| def with_poll_interval(self, poll_interval) -> 'Watch': |
There was a problem hiding this comment.
Constructor already have "poll_interval", "output_coder", etc. No need providing another methods to set them (Java withXXX() way). Referring to example utility transforms (apache_beam/transform), we use Pythonic way to construct transform.
There was a problem hiding this comment.
You are right, the setter methods added nothing on top of the constructor. I removed growth_of and all the with_* methods, so the transform is now built in one call like the other utility transforms, and poll_interval became a required constructor argument since polling cannot run without it.
| output = ( | ||
| p | beam.Create(['k:']) | ||
| | Watch.growth_of(_complete_poll).with_poll_interval( | ||
| Duration(1)).with_output_coder(StrUtf8Coder())) |
There was a problem hiding this comment.
I notice many with_output_coder(StrUtf8Coder()) added in latest diff. This rather suggests coder inference is either broken or not working in the latest change.
There was a problem hiding this comment.
Good catch, inference was missing. Expansion fell back to PickleCoder, and the determinism error from the last round then rejected it, which is what forced every test to pass a coder by hand. The output coder is now inferred. A PollFn can provide default_output_coder, and a plain function can annotate its return type as PollResult[str] so the element type resolves through the coder registry. Together with the key coder change, the fallback coder also works without any hint, so the explicit coders in the tests are gone.
| # Dedup hashes the encoded output as its key, so a non-deterministic coder | ||
| # would hash equal outputs differently and re-emit them, defeating the | ||
| # transform. Require determinism rather than silently emitting duplicates. | ||
| if not output_coder.is_deterministic(): |
There was a problem hiding this comment.
It's still not the right check. We only need to make sure key_coder isdeterministic here.
I think what happens to the code completion is that it doesn't understand the difference between key_coder and output_coder
This line in the
`self._key_coder = output_coder`
also suggests this.
Please check Java implication again. outputKeyCoder is involved when OutputKeyFn is configured. Otherwise dedup key defaults to be the same as value, and only in this case self._key_coder is equivalent to output_coder.
There was a problem hiding this comment.
I went through the Java expansion again and you are right, I had taken the key coder name without the distinction behind it. The transform now takes output_key_fn and output_key_coder. When no key fn is given, the dedup key is the output itself and the key coder defaults to the output coder, and the determinism requirement applies only to the key coder. Expansion now converts it with as_deterministic_coder, the same conversion grouping keys go through, so the pickle based fallback hashes deterministically and only a coder with no deterministic form is rejected. The output coder itself no longer needs to be deterministic.
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class _PollPlan: |
There was a problem hiding this comment.
We need to check again this newly introduced PollPlan. From the pydoc it sounds an ad-hoc solution to put poll outside of tryClaim. Constructively we should just be able to use PollResult as its Restriction Tracker. It now needs other information now likely because the responsibility boundary of process and try_claim is still not clear
There was a problem hiding this comment.
I agree, _PollPlan was patching around the boundary instead of fixing it, and it is gone now. The claimed position is the poll round itself, a pair of the deduped PollResult and the termination state. try_claim validates that pair against the restriction, and try_split derives the replay primary and the merged polling residual, so the checkpoint logic lives in the tracker again and process only polls, dedups, emits and decides whether to stop or resume. One behavior worth noting, a runner initiated split that lands right after a terminal round is claimed leaves a polling residual, so that input polls once more and stops on the next round. I also moved the watermark updates onto the resume path only, seeded from the element timestamp on the first round.
- Replace growth_of and the with_* builders with constructor arguments. - Infer the output coder from PollFn.default_output_coder() or a PollResult[V] return annotation via the coder registry; require determinism only of the dedup key coder, converting it GroupByKey-style with as_deterministic_coder, and add output_key_fn/output_key_coder mirroring Java's outputKeyFn/outputKeyCoder. - Rework the restriction tracker to Java's design: try_claim takes the (PollResult, termination_state) round and validates it against the restriction; try_split derives the replay primary and merged residual. - Match Java's watermark handling: seed the estimator from the input timestamp and advance it to the poll watermark or earliest new output when resuming.
|
Sorry about the force pushes earlier, I did not realize they break the incremental review, and I will keep fixes as follow up commits from now on. This round is commit c559963 on top of the reviewed one. It covers the four comments, and the tests and lint pass locally. |
The replay round leaves the watermark at the seed and reports no residual. A run that defers twice before completing keeps the watermark where the last poll left it and ends without a residual.
This adds an experimental Watch transform to the Python SDK. It addresses #21521.
Watch watches a growing set of outputs for each input element. It calls a user poll function on an interval and emits an unbounded PCollection of input and output pairs. Polling stops per input when the poll reports completion or when a termination condition fires. The transform is a splittable DoFn, so each process call performs one poll round and then self checkpoints with defer_remainder, which keeps the polling loop durable across bundles. New outputs are deduplicated with a stable 128 bit blake2b hash of the encoded output, and a manual watermark estimator advances once per poll.
This is an initial minimal version intended for review and iteration. It implements growth_of, PollResult, PollFn, the never and after_total_of termination conditions, the growth state restriction with its coder, and the polling splittable DoFn. It is validated end to end on the DirectRunner. The output coder must be deterministic for dedup to hold across workers, and the transform logs a warning when the resolved coder is not deterministic.
The following are out of scope for this initial change and are not included here: side input poll functions, the remaining termination conditions, a separate bounded stage for exploding large poll results, and bounded growth state eviction.
Testing
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.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.