Move CombineValues override to non portable overrides #25106

damccorm · 2023-01-20T20:02:04Z

Right now, running the following pipeline on Dataflow:

import logging

import apache_beam as beam
from apache_beam.options.pipeline_options import DebugOptions

def merge(vals):
    out = ""
    for v in vals:
        out += v
    return out

def run():
    pipeline_options = DebugOptions() 
    pipeline_options.dataflow_job_file = "path/to/dump/proto"

    with beam.Pipeline(options=pipeline_options) as p:
        (p
            | beam.Create([("key1", "foo"), ("key2", "bar"), ("key1", "baz")])
            | beam.GroupByKey()
            | beam.CombineValues(merge)
        )


if __name__ == '__main__':
    logging.getLogger().setLevel(logging.INFO)
    run()

succeeds when use_runner_v2 is not included as an experiment, but fails when it is. This is because the translation being done generates an invalid graph for the use_runner_v2 case: a portable pipeline shouldn't be using the overridden combine here.

This fixes the problem by moving the override to only occur when use_runner_v2 is not included

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.

codecov · 2023-01-20T20:20:18Z

Codecov Report

Merging #25106 (13c8538) into master (e379c23) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25106      +/-   ##
==========================================
+ Coverage   73.11%   73.13%   +0.01%     
==========================================
  Files         735      735              
  Lines       98161    98161              
==========================================
+ Hits        71774    71787      +13     
+ Misses      25024    25011      -13     
  Partials     1363     1363

Flag	Coverage Δ
python	`82.68% <ø> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...on/apache_beam/runners/dataflow/dataflow_runner.py	`81.88% <ø> (+0.14%)`	⬆️
sdks/python/apache_beam/transforms/combiners.py	`93.05% <0.00%> (-0.39%)`	⬇️
...ks/python/apache_beam/runners/worker/sdk_worker.py	`89.08% <0.00%> (-0.17%)`	⬇️
...s/python/apache_beam/io/gcp/bigquery_file_loads.py	`87.38% <0.00%> (+0.22%)`	⬆️
sdks/python/apache_beam/pipeline.py	`92.42% <0.00%> (+0.29%)`	⬆️
...hon/apache_beam/runners/worker/bundle_processor.py	`93.54% <0.00%> (+0.37%)`	⬆️
sdks/python/apache_beam/internal/dill_pickler.py	`84.89% <0.00%> (+0.71%)`	⬆️
...che_beam/runners/interactive/interactive_runner.py	`91.77% <0.00%> (+1.26%)`	⬆️
sdks/python/apache_beam/io/source_test_utils.py	`89.86% <0.00%> (+1.38%)`	⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

damccorm · 2023-01-20T20:24:31Z

Run Python 3.9 PostCommit

github-actions · 2023-01-20T21:18:27Z

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

R: @AnandInguva for label python.

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

damccorm · 2023-01-21T02:11:56Z

R: @lukecwik

github-actions · 2023-01-21T02:30:27Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

lukecwik

Can we add the described scenario as a new ValidatesRunner test to prevent a regression in the future?

damccorm · 2023-01-23T15:15:53Z

Run Python 3.8 PostCommit

damccorm · 2023-01-23T15:16:20Z

Run Python Dataflow V2 ValidatesRunner

damccorm · 2023-01-23T15:16:34Z

Run Python Dataflow ValidatesRunner

damccorm · 2023-01-23T15:22:20Z

Run Python 3.8 PostCommit

damccorm · 2023-01-23T15:22:28Z

Run Python Dataflow ValidatesRunner

damccorm · 2023-01-23T15:22:35Z

Run Python Dataflow V2 ValidatesRunner

damccorm · 2023-01-23T15:46:24Z

Run Python 3.8 PostCommit

damccorm · 2023-01-23T15:46:33Z

Run Python Dataflow V2 ValidatesRunner

damccorm · 2023-01-23T15:46:40Z

Run Python Dataflow ValidatesRunner

damccorm · 2023-01-23T15:58:29Z

Run Python 3.8 PostCommit

damccorm · 2023-01-23T15:58:37Z

Run Python Dataflow V2 ValidatesRunner

damccorm · 2023-01-23T15:58:44Z

Run Python Dataflow ValidatesRunner

AnandInguva · 2023-01-23T16:34:20Z

stop reviewer notifications

damccorm · 2023-01-23T16:46:59Z

Run Python 3.8 PostCommit

damccorm · 2023-01-23T16:47:08Z

Run Python Dataflow V2 ValidatesRunner

damccorm · 2023-01-23T16:47:17Z

Run Python Dataflow ValidatesRunner

lukecwik · 2023-01-23T17:02:40Z

sdks/python/apache_beam/examples/snippets/transforms/aggregation/combinevalues_it_test.py

+
+from . import combinevalues
+
+class CombineValuesIT(unittest.TestCase):


I would suggest making this a validates runner test like:

beam/sdks/python/apache_beam/transforms/ptransform_test.py

Line 702 in 897fffd

def test_flatten_pcollections(self):

in ptransform_test.py

damccorm · 2023-01-23T18:24:01Z

Run Python Dataflow V2 ValidatesRunner

damccorm · 2023-01-23T18:24:11Z

Run Python Dataflow ValidatesRunner

lukecwik

LGTM

sdks/python/apache_beam/transforms/combiners_test.py

Co-authored-by: Lukasz Cwik <[email protected]>

damccorm added 2 commits January 20, 2023 14:58

Move CombineValues override to non portable overrides

3d035aa

Fix extraneously added tab

32f216f

github-actions bot added the python label Jan 20, 2023

github-actions bot added Next Action: Reviewers and removed python labels Jan 20, 2023

lukecwik requested changes Jan 21, 2023

View reviewed changes

Create combinevalues_it_test.py

7168b2a

github-actions bot added examples python labels Jan 23, 2023

License

24a341a

Add backslashes

7da551d

Format

18cdb5f

Lint

c64f8c4

Fix imports

02e24a0

lukecwik requested changes Jan 23, 2023

View reviewed changes

damccorm added 2 commits January 23, 2023 13:12

Validates runner test

bd80c07

Spacing

8739f52

github-actions bot removed the examples label Jan 23, 2023

lukecwik approved these changes Jan 23, 2023

View reviewed changes

sdks/python/apache_beam/transforms/combiners_test.py Outdated Show resolved Hide resolved

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

13c8538

Co-authored-by: Lukasz Cwik <[email protected]>

damccorm merged commit 6029a57 into master Jan 23, 2023

damccorm deleted the users/damccorm/combine branch January 23, 2023 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move CombineValues override to non portable overrides #25106

Move CombineValues override to non portable overrides #25106

damccorm commented Jan 20, 2023

codecov bot commented Jan 20, 2023 •

edited

Loading

damccorm commented Jan 20, 2023

github-actions bot commented Jan 20, 2023

damccorm commented Jan 21, 2023

github-actions bot commented Jan 21, 2023

lukecwik left a comment

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

AnandInguva commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

lukecwik Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

lukecwik left a comment


		from . import combinevalues

		class CombineValuesIT(unittest.TestCase):

Move CombineValues override to non portable overrides #25106

Move CombineValues override to non portable overrides #25106

Conversation

damccorm commented Jan 20, 2023

GitHub Actions Tests Status (on master branch)

codecov bot commented Jan 20, 2023 • edited Loading

Codecov Report

damccorm commented Jan 20, 2023

github-actions bot commented Jan 20, 2023

damccorm commented Jan 21, 2023

github-actions bot commented Jan 21, 2023

lukecwik left a comment

Choose a reason for hiding this comment

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

AnandInguva commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

lukecwik Jan 23, 2023

Choose a reason for hiding this comment

damccorm commented Jan 23, 2023

damccorm commented Jan 23, 2023

lukecwik left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 20, 2023 •

edited

Loading