[BEAM-5878] add tests for kwonly args in python 3 #8505

Juta · 2019-05-06T15:15:21Z

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---			---	---	---

Pre-Commit Tests Status (on master branch)

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

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

Juta · 2019-05-06T15:44:18Z

R: @tvalentyn

tvalentyn · 2019-05-06T16:11:38Z

sdks/python/apache_beam/transforms/tranforms_keyword_only_args_test.py

+
+    result3 = pcol | 'compute3' >> beam.FlatMap(
+      sort_with_side_inputs)
+    assert_that(result3, equal_to([]), label='assert3')


(didn't look into the logic in detail yet) is the result supposed to be empty here and in line 64?

yes because it joins with the side input and in this case there is no side input

Juta · 2019-05-07T09:28:32Z

@tvalentyn The test currently fails on pylint 2.7 because the syntax of keyword only args gives a syntax-error in python 2. Should I add a pylint disable syntax-error comment or is there an other way to skip this file for python 2?

tvalentyn · 2019-05-07T13:17:36Z

Let's try to disable a particular pylint warning and add a comment.
# Keyword-only arguments are not available on Python 2
# pylint: disable=wrong-....

Juta · 2019-05-07T17:46:56Z

@tvalentyn Apparently # pylint disable=syntax-error is not possible: pylint-dev/pylint#986. I can only think of fixing it by adding it as an exception in pylint2.7: https://github.com/apache/beam/blob/master/sdks/python/scripts/run_pylint.sh#L73. What do you think?

tvalentyn · 2019-05-07T19:49:16Z

Adding as exception sounds good to me. Is it possible to add and exception on Python 2 lint only? If that's hard, you can file a Jira and add a comment to clean this up eventually.

tvalentyn · 2019-05-13T21:09:13Z

It looks like the suite still fails on Py2 due to unsupported syntax.

Juta · 2019-05-20T13:51:00Z

@tvalentyn PTAL

tvalentyn

Thanks a lot, this is great! Left a few minor comments.

tvalentyn · 2019-05-20T17:30:27Z

sdks/python/apache_beam/transforms/transforms_keyword_only_args_py3_test.py

+    pipeline = TestPipeline()
+
+    # Keyword-only arguments are not available on Python 2
+    # pylint: disable=syntax-error


Leftover comment?

tvalentyn · 2019-05-20T17:39:17Z

sdks/python/apache_beam/transforms/transforms_keyword_only_args_py3_test.py

+    result1 = pcoll | 'sum1' >> beam.ParDo(MyDoFn(), 5, 8, bound=15)
+    result2 = pcoll | 'sum2' >> beam.ParDo(MyDoFn(), 5, 8)
+    result3 = pcoll | 'sum3' >> beam.ParDo(MyDoFn())
+    result4 = pcoll | 'sum4' >> beam.ParDo(MyDoFn(), bound=10)


You could select a smaller bound (5) here for result4 to be different.

tvalentyn · 2019-05-20T17:40:57Z

sdks/python/apache_beam/transforms/transforms_keyword_only_args_py3_test.py

+    result1 = pcoll | 'sum1' >> beam.CombineGlobally(bounded_sum, 5, 8, bound=20)
+    result2 = pcoll | 'sum2' >> beam.CombineGlobally(bounded_sum, 5, 8)
+    result3 = pcoll | 'sum3' >> beam.CombineGlobally(bounded_sum)
+    result4 = pcoll | 'sum4' >> beam.CombineGlobally(bounded_sum, bound=12)


You could select a smaller bound here for result4 to be different.

tvalentyn · 2019-05-20T17:51:05Z

sdks/python/scripts/run_pylint.sh

@@ -60,8 +60,15 @@ EXCLUDED_GENERATED_FILES=(
 apache_beam/portability/api/*pb2*.py
 )

+# Following files contain python 3 syntax and are excluded from pylint in python 2
+EXCLUDED_PYTHON3_FILES=(
+"apache_beam/transforms/transforms_keyword_only_args_py3_test.py"


Let's add # TODO(BEAM-7372): Remove this list once Python 2 is no longer supported and drop _py3_test suffix.

tvalentyn · 2019-05-20T18:01:26Z

sdks/python/scripts/run_pylint.sh

@@ -60,8 +60,15 @@ EXCLUDED_GENERATED_FILES=(
 apache_beam/portability/api/*pb2*.py
 )

+# Following files contain python 3 syntax and are excluded from pylint in python 2
+EXCLUDED_PYTHON3_FILES=(
+"apache_beam/transforms/transforms_keyword_only_args_py3_test.py"


I think _py3_only_test may be a more explicit suffix, how about we use that?
Also could this be a regex pattern .*_py3_only_test.py?

Juta · 2019-05-21T13:22:30Z

Run Python PreCommit

Juta · 2019-05-27T13:42:40Z

@tvalentyn PTAL

tvalentyn

Thanks, @Juta.
@aaltay could you please help with the merge?

apilloud · 2019-06-03T21:55:44Z

After this change beam_PostCommit_Python_Verify :sdks:python:postCommitIT and other tests are failing due to a syntax error. I filed an issue: https://issues.apache.org/jira/browse/BEAM-7481

This reverts commit 15d0341.

…test added in apache#8505

* Also add back the unit tests introduced in #8505 with minor changes.

* Also add back the unit tests introduced in apache#8505 with minor changes.

tvalentyn reviewed May 6, 2019

View reviewed changes

Juta force-pushed the keywords branch from f6b4c01 to 9adb2c6 Compare May 7, 2019 16:18

Juta force-pushed the keywords branch 3 times, most recently from a4796f6 to 7a169c2 Compare May 10, 2019 14:42

Juta force-pushed the keywords branch 4 times, most recently from 1960b43 to 403b5ed Compare May 20, 2019 12:09

tvalentyn requested changes May 20, 2019

View reviewed changes

Juta added 2 commits May 21, 2019 10:48

[BEAM-5878] add tests for kwonly args in python 3

72f2703

[BEAM-5878] skip python3 sytntax tests in pylint and tox on python 2

4cbb120

Juta force-pushed the keywords branch 2 times, most recently from e113e64 to ae5da58 Compare May 21, 2019 11:32

Juta force-pushed the keywords branch 2 times, most recently from c0c5abd to 5f0c245 Compare May 21, 2019 16:19

fixup: rename suffix py3 only test

76337bf

Juta force-pushed the keywords branch from 5f0c245 to 76337bf Compare May 27, 2019 12:51

tvalentyn approved these changes May 31, 2019

View reviewed changes

aaltay approved these changes May 31, 2019

View reviewed changes

aaltay merged commit 15d0341 into apache:master May 31, 2019

tvalentyn added a commit to tvalentyn/beam that referenced this pull request Jun 3, 2019

Revert "[BEAM-5878] add tests for kwonly args in python 3 (apache#8505)"

81813c0

This reverts commit 15d0341.

tvalentyn mentioned this pull request Jun 3, 2019

[BEAM-7481] Revert "[BEAM-5878] add tests for kwonly args in python 3" #8750

Merged

lazylynx added a commit to lazylynx/beam that referenced this pull request Aug 2, 2019

[BEAM-5878] DoFns with Keyword-only arguments supported and restored …

60caf81

…test added in apache#8505

tvalentyn mentioned this pull request Aug 2, 2019

[BEAM-7060] Introduce Python3-only test modules #9223

Merged

3 tasks

lazylynx added a commit to lazylynx/beam that referenced this pull request Aug 3, 2019

[BEAM-5878] DoFns with Keyword-only arguments supported and restored …

26d4a04

…test added in apache#8505

lazylynx added a commit to lazylynx/beam that referenced this pull request Aug 3, 2019

[BEAM-5878] DoFns with Keyword-only arguments supported and restored …

0e5bd93

…test added in apache#8505

tvalentyn pushed a commit that referenced this pull request Sep 3, 2019

[BEAM-5878] support DoFns with Keyword-only arguments (#9237)

0c2d8be

* Also add back the unit tests introduced in #8505 with minor changes.

soyrice pushed a commit to soyrice/beam that referenced this pull request Sep 19, 2019

[BEAM-5878] support DoFns with Keyword-only arguments (apache#9237)

ab3eeb3

* Also add back the unit tests introduced in apache#8505 with minor changes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-5878] add tests for kwonly args in python 3 #8505

[BEAM-5878] add tests for kwonly args in python 3 #8505

Juta commented May 6, 2019

Juta commented May 6, 2019

tvalentyn May 6, 2019

Juta May 7, 2019

Juta commented May 7, 2019

tvalentyn commented May 7, 2019

Juta commented May 7, 2019

tvalentyn commented May 7, 2019

tvalentyn commented May 13, 2019

Juta commented May 20, 2019

tvalentyn left a comment

tvalentyn May 20, 2019

tvalentyn May 20, 2019

tvalentyn May 20, 2019

tvalentyn May 20, 2019

tvalentyn May 20, 2019

Juta commented May 21, 2019

Juta commented May 27, 2019

tvalentyn left a comment

apilloud commented Jun 3, 2019

[BEAM-5878] add tests for kwonly args in python 3 #8505

[BEAM-5878] add tests for kwonly args in python 3 #8505

Conversation

Juta commented May 6, 2019

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Juta commented May 6, 2019

tvalentyn May 6, 2019

Choose a reason for hiding this comment

Juta May 7, 2019

Choose a reason for hiding this comment

Juta commented May 7, 2019

tvalentyn commented May 7, 2019

Juta commented May 7, 2019

tvalentyn commented May 7, 2019

tvalentyn commented May 13, 2019

Juta commented May 20, 2019

tvalentyn left a comment

Choose a reason for hiding this comment

tvalentyn May 20, 2019

Choose a reason for hiding this comment

tvalentyn May 20, 2019

Choose a reason for hiding this comment

tvalentyn May 20, 2019

Choose a reason for hiding this comment

tvalentyn May 20, 2019

Choose a reason for hiding this comment

tvalentyn May 20, 2019

Choose a reason for hiding this comment

Juta commented May 21, 2019

Juta commented May 27, 2019

tvalentyn left a comment

Choose a reason for hiding this comment

apilloud commented Jun 3, 2019