Repository navigation
[Prism] Finalize bundles of DoFns fused after another transform - #40640
Conversation
Problem:
In finalizeStage (sdks/go/pkg/beam/runners/prism/internal/preprocess.go), Prism sets
stage.finalize only while it inspects the consumers of the stage's input PCollections.
The loop skips every PCollection that a transform of the same stage produces:
for pid, plinks := range stageFacts.PcolConsumers {
if _, ok := stageFacts.PcolProducers[pid]; ok {
continue
}
...
if pardo.GetRequestsFinalization() {
stg.finalize = true
}
So a ParDo that sets requests_finalization but is fused after another transform (a Map
after Create, the expand step of a Reshuffle, any element-wise chain) never marks its
stage. The SDK harness registers the callbacks and returns requires_finalization, but Prism
never sends FinalizeBundleRequest, and the callbacks never run. This affects every SDK. The
existing TestParDoBundleFinalizer cases apply the finalizing DoFn directly to Impulse, so the
DoFn always consumes the stage input and the tests pass.
Fix:
Set stage.finalize from every ParDo-like transform of the stage (ParDo and the splittable
DoFn stage URNs), not only from the consumers of the stage inputs. Statefulness, the
on-window-expiration timer and side inputs still come from the input consumers. Add a
TestParDoBundleFinalizer case with the finalizing DoFn fused after another ParDo.
Reproduction (Python SDK on PrismRunner):
```python
import os
import tempfile
import apache_beam as beam
from apache_beam.options.pipeline_options import PipelineOptions
class MarkFinalization(beam.DoFn):
"""Registers a bundle finalization callback that creates the file `path`."""
def __init__(self, path):
self._path = path
def process(self, element, bundle_finalizer=beam.DoFn.BundleFinalizerParam):
path = self._path
bundle_finalizer.register(lambda: open(path, "w").close())
yield element
if __name__ == "__main__":
markers = tempfile.mkdtemp()
at_input = os.path.join(markers, "at_stage_input")
fused = os.path.join(markers, "after_fused_map")
options = PipelineOptions(["--runner=PrismRunner", "--environment_type=LOOPBACK"])
with beam.Pipeline(options=options) as p:
impulse = p | beam.Impulse()
# Consumes the input of its stage: Prism finalizes the bundle.
_ = impulse | "AtStageInput" >> beam.ParDo(MarkFinalization(at_input))
# Fused after a Map in the same stage: Prism never finalizes the bundle.
_ = (
impulse
| "Map" >> beam.Map(lambda x: x)
| "AfterFusedMap" >> beam.ParDo(MarkFinalization(fused))
)
print("finalized:", sorted(os.listdir(markers)))
assert os.path.exists(at_input), "the DoFn at the stage input was not finalized"
assert os.path.exists(fused), "the DoFn fused after Map was not finalized"
```
Before the fix: `finalized: ['at_stage_input']`, and the second assertion fails.
After the fix: `finalized: ['after_fused_map', 'at_stage_input']`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40640 +/- ##
============================================
+ Coverage 56.16% 56.22% +0.05%
============================================
Files 1124 868 -256
Lines 178336 153546 -24790
Branches 1489 0 -1489
============================================
- Hits 100170 86327 -13843
+ Misses 75645 65382 -10263
+ Partials 2521 1837 -684
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:
|
…e error Move the check of requests_finalization into requestsFinalization. finalizeStage runs computeFacts first, which already rejects a ParDo payload that does not decode, so the error return of the earlier loop could not run. Add TestFinalizeStage_RequestsFinalization: the flag stays false when no transform asks for it, and is set when the transform at the stage input, a ParDo fused after another transform, or a fused splittable DoFn process stage asks for it.
There was a problem hiding this comment.
📋 Review Summary
This PR resolves a bug in Prism where bundle finalization callbacks were skipped for ParDo transforms fused downstream of another transform within the same stage. The fix properly inspects all transforms in the stage for requests_finalization rather than only the transforms consuming the stage's external inputs, and includes both unit and integration tests.
🔍 General Feedback
- The root cause analysis and fix in
finalizeStageare clean, accurate, and properly account for both standardParDoand expanded SplittableDoFntransform URNs. - Unit test coverage in
preprocess_test.goand integration test coverage inprimitives/pardo_test.gothoroughly verify the fix.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @lostluck for label go. 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). |
|
Good find! Looking presently. |
Problem:
In finalizeStage (sdks/go/pkg/beam/runners/prism/internal/preprocess.go), Prism sets
stage.finalizeonly while it inspects the consumers of the stage's input PCollections. The loop skips every PCollection that a transform of the same stage produces:So a
ParDothat setsrequests_finalizationbut is fused after another transform (aMapafterCreate, the expand step of aReshuffle, any element-wise chain) never marks its stage. The SDK harness registers the callbacks and returnsrequires_finalization, but Prism never sendsFinalizeBundleRequest, and the callbacks never run. This affects every SDK. The existingTestParDoBundleFinalizercases apply the finalizingDoFndirectly to Impulse, so theDoFnalways consumes the stage input and the tests pass.Fix:
Set
stage.finalizefrom every ParDo-like transform of the stage (ParDoand the splittableDoFnstage URNs), not only from the consumers of the stage inputs. Statefulness, the on-window-expiration timer and side inputs still come from the input consumers. Add aTestParDoBundleFinalizercase with the finalizingDoFnfused after anotherParDo.Reproduction (Python SDK on PrismRunner):
Before the fix:
finalized: ['at_stage_input'], and the second assertion fails.After the fix:
finalized: ['after_fused_map', 'at_stage_input'].