[BEAM-9487] Disable allowing unsafe triggers by default #15340

zhoufek · 2021-08-17T14:07:18Z

In the release notes for Beam 2.31, there were two deprecations mentioned that affect GroupByKey in the Python SDK:

* Python GBK will stop supporting unbounded PCollections that have global windowing and a default trigger in Beam 2.33. This can be overriden with `--allow_unsafe_triggers`. ([BEAM-9487](https://issues.apache.org/jira/browse/BEAM-9487)).
* Python GBK will start requiring safe triggers or the `--allow_unsafe_triggers` flag starting with Beam 2.33. ([BEAM-9487](https://issues.apache.org/jira/browse/BEAM-9487)).

This PR making that change in anticipation of the upcoming Beam 2.33 release.

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

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

zhoufek · 2021-08-17T17:38:02Z

All test failures are from a known problem related to Panda and are unrelated to this change. Here's the test for reference, and it has a skip message calling this out:

beam/sdks/python/apache_beam/dataframe/frames_test.py

Line 1684 in 3fb6aad

def test_df_agg_method_invalid_kwarg_raises(self):

zhoufek · 2021-08-17T17:40:47Z

Should CHANGES.md be updated to mark this as deprecated in 2.32, or was marking it as deprecated in 2.31 good enough?

zhoufek · 2021-08-17T17:43:06Z

R: @aaltay

aaltay · 2021-08-20T01:29:25Z

sdks/python/apache_beam/transforms/core.py

@@ -2332,22 +2332,19 @@ def expand(self, pcoll):

    unsafe_reason = trigger.may_lose_data(windowing)
    if unsafe_reason != DataLossReason.NO_POTENTIAL_LOSS:
+      reason_msg = str(unsafe_reason).replace('DataLossReason.', '')
      if pcoll.pipeline.allow_unsafe_triggers:


Just double checking. Is this (pcoll.pipeline.allow_unsafe_triggers) correct? Would not it be something like pcoll.pipeline.options.allow_unsafe_triggers ?

options is marked as deprecated with the message, "References to <pipeline>.options will not be supported."

For that reason, I added a property to access the option:

beam/sdks/python/apache_beam/pipeline.py

Line 243 in b0e9f26

def allow_unsafe_triggers(self):

aaltay · 2021-08-20T01:30:14Z

Should CHANGES.md be updated to mark this as deprecated in 2.32, or was marking it as deprecated in 2.31 good enough?

Yes, please also update the changes.md file for 2.33.0 (or whichever the release this will be part of).

zhoufek · 2021-08-20T13:13:05Z

Updated CHANGES.md to both call out the deprecation in 2.32 and note the breaking change for 2.33.

codecov · 2021-08-20T13:15:03Z

Codecov Report

Merging #15340 (706e20f) into master (b0e9f26) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #15340      +/-   ##
==========================================
- Coverage   83.81%   83.68%   -0.13%     
==========================================
  Files         440      440              
  Lines       59795    60292     +497     
==========================================
+ Hits        50116    50458     +342     
- Misses       9679     9834     +155

Impacted Files	Coverage Δ
...dks/python/apache_beam/options/pipeline_options.py	`95.16% <ø> (ø)`
sdks/python/apache_beam/transforms/core.py	`92.02% <100.00%> (+<0.01%)`	⬆️
sdks/python/apache_beam/io/gcp/bigquery.py	`73.98% <0.00%> (-2.40%)`	⬇️
...apache_beam/runners/portability/portable_runner.py	`76.14% <0.00%> (-1.76%)`	⬇️
sdks/python/apache_beam/io/source_test_utils.py	`88.47% <0.00%> (-1.39%)`	⬇️
...hon/apache_beam/runners/worker/bundle_processor.py	`93.51% <0.00%> (-0.13%)`	⬇️
setup.py	`0.00% <0.00%> (ø)`
sdks/python/apache_beam/transforms/trigger.py	`89.61% <0.00%> (+0.34%)`	⬆️
sdks/python/apache_beam/runners/direct/executor.py	`96.79% <0.00%> (+0.53%)`	⬆️
sdks/python/apache_beam/io/localfilesystem.py	`92.24% <0.00%> (+0.77%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0e9f26...706e20f. Read the comment docs.

udim

Thanks!

Failing test seems unrelated:
https://ci-beam.apache.org/job/beam_PreCommit_Python_Commit/19571/testReport/junit/apache_beam.runners.portability.fn_api_runner.fn_runner_test/FnApiRunnerTestWithGrpcAndMultiWorkers/test_multimap_multiside_input_2/

aaltay · 2021-08-20T19:41:29Z

Thanks!

Failing test seems unrelated:
ci-beam.apache.org/job/beam_PreCommit_Python_Commit/19571/testReport/junit/apache_beam.runners.portability.fn_api_runner.fn_runner_test/FnApiRunnerTestWithGrpcAndMultiWorkers/test_multimap_multiside_input_2

I agree. But let's try one more time. Feel free to merge after the next run.

aaltay · 2021-08-20T19:41:36Z

Run Python PreCommit

zhoufek marked this pull request as ready for review August 17, 2021 17:41

aaltay requested a review from udim August 20, 2021 01:26

aaltay reviewed Aug 20, 2021

View reviewed changes

[BEAM-9487] Disable allowing unsafe triggers by default

706e20f

zhoufek force-pushed the gbk_233 branch from de09bb9 to 706e20f Compare August 20, 2021 12:58

udim approved these changes Aug 20, 2021

View reviewed changes

aaltay approved these changes Aug 20, 2021

View reviewed changes

udim merged commit cf8e08f into apache:master Aug 20, 2021

zhoufek deleted the gbk_233 branch September 3, 2021 01:05

zhoufek restored the gbk_233 branch September 28, 2021 01:12

zhoufek deleted the gbk_233 branch September 28, 2021 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-9487] Disable allowing unsafe triggers by default #15340

[BEAM-9487] Disable allowing unsafe triggers by default #15340

zhoufek commented Aug 17, 2021 •

edited

zhoufek commented Aug 17, 2021 •

edited

zhoufek commented Aug 17, 2021

zhoufek commented Aug 17, 2021

aaltay Aug 20, 2021

zhoufek Aug 20, 2021 •

edited

aaltay commented Aug 20, 2021 •

edited

zhoufek commented Aug 20, 2021

codecov bot commented Aug 20, 2021 •

edited

udim left a comment

aaltay commented Aug 20, 2021

aaltay commented Aug 20, 2021

[BEAM-9487] Disable allowing unsafe triggers by default #15340

[BEAM-9487] Disable allowing unsafe triggers by default #15340

Conversation

zhoufek commented Aug 17, 2021 • edited

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

zhoufek commented Aug 17, 2021 • edited

zhoufek commented Aug 17, 2021

zhoufek commented Aug 17, 2021

aaltay Aug 20, 2021

Choose a reason for hiding this comment

zhoufek Aug 20, 2021 • edited

Choose a reason for hiding this comment

aaltay commented Aug 20, 2021 • edited

zhoufek commented Aug 20, 2021

codecov bot commented Aug 20, 2021 • edited

Codecov Report

udim left a comment

Choose a reason for hiding this comment

aaltay commented Aug 20, 2021

aaltay commented Aug 20, 2021

zhoufek commented Aug 17, 2021 •

edited

`ValidatesRunner` compliance status (on master branch)

zhoufek commented Aug 17, 2021 •

edited

zhoufek Aug 20, 2021 •

edited

aaltay commented Aug 20, 2021 •

edited

codecov bot commented Aug 20, 2021 •

edited