[BEAM-7407] Adds portable wordcount test for Python 3 with Flink runner. #8745

tvalentyn · 2019-06-03T18:37:47Z

This PR extends the portable python precommit test suite with a Python 3 scenario.

The PR adds portable wordcount test for Python 3 with Flink running in-process runner and SDK harness running in Docker container.
To run Python 2 and Python 3 test scenarios in parallel, we add the Py3 scenario to a new project project sdks/python/test-suites/portable/py35/build.gradle inline with the proposal discussed in https://lists.apache.org/thread.html/7c48200678c570c6e42d4933f8ea4f9324d82f7ae720b591a93ffcda@%3Cdev.beam.apache.org%3E.
To reduce the duplication of Gradle code between Python 2 and Python 3 suites, we move the common the definition of Portable Wordcount task to to Beam Module Plugin.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	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.

tvalentyn · 2019-06-04T00:32:10Z

R: @markflyhigh @angoenka
cc: @fredo838, @Juta

tvalentyn · 2019-06-06T16:35:34Z

Ping please, @markflyhigh @angoenka. CC: @aaltay .

angoenka

Thanks @tvalentyn !
Some minor comments.

angoenka · 2019-06-06T23:06:17Z

sdks/python/apache_beam/runners/portability/portable_runner.py

+ 'has Python %d.%d interpreter. See also: BEAM-7474.' % (
+ sys.version_info[0], sys.version_info[1]))
+
+ return ('{user}-docker-apache.bintray.io/beam/python'


Shall we log the image URL

angoenka · 2019-06-06T23:08:34Z

sdks/python/build.gradle

@@ -77,11 +77,14 @@ task preCommitPy2() {
 dependsOn "lint"
 }

-task portablePreCommit() {
+addPortableWordCountTask('BatchPy2', false)


We can infer batch or streaming from the flag

We can, however, the portableWordCountExample definition would be more complicated. Do we meed that task at all? how is it used?
I can remove it if you prefer and remove one of the prefix parameter.

We can certainly remove portableWordCountExample but we would like to keep portableWordCount

angoenka · 2019-06-06T23:12:46Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

@@ -1815,6 +1815,58 @@ class BeamModulePlugin implements Plugin<Project> {
 }
 }
 }
+
+ project.ext.addPortableWordCountTask = { String nameSuffix, boolean isStreaming ->


We can use implicit argument to get nameSuffix and isStreaming as we do in https://github.com/apache/beam/blob/master/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L1649

Good to know, thanks. I find explicit way more reader-friendly. Unless there is a reason (long list of optional parameters, etc), I think explicit is better than implicit.

I have similar feeling but I think in groovy land, implicit is more popular because we don't have to change method signature to add new arguments or remove arguments.

PTAL. I couldn't find such recommendation in Groovy style guide https://groovy-lang.org/style-guide.html. I moved the streaming parameter into an internal helper function, and it only has 1 parameter right now. Making it implicit requires adding another "Configuration" class, which at this point seems unnecessary boilerplate. I agree that passing maps is better when we have large number of parameters, similar how service requests/responses are JSON / ProtoBufs, but I don't think we should follow this everywhere.

markflyhigh

Thank you @tvalentyn! General lgtm. Some minor thoughts.

markflyhigh · 2019-06-06T20:21:08Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

+
+ project.task('portableWordCount' + nameSuffix) {
+ dependsOn 'installGcpTest'
+ dependsOn = ['installGcpTest']


good catch.

markflyhigh · 2019-06-06T22:35:51Z

sdks/python/apache_beam/runners/portability/portable_runner.py

+ else:
+ version_suffix = '3'
+ # TODO(BEAM-7474): Use an image which has correct Python minor version.
+ logging.warning('Make sure that locally built Python SDK docker image '


Does it have to be locally built?

We don't have a release process for this image yet, so we build it locally when we run the test.

markflyhigh · 2019-06-07T00:15:17Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

@@ -1815,6 +1815,58 @@ class BeamModulePlugin implements Plugin<Project> {
 }
 }
 }
+
+ project.ext.addPortableWordCountTask = { String nameSuffix, boolean isStreaming ->


I'm okay to have this task in BeamModulePlugin for now. But my concern is that this may not be scalable if we want to add more variants (e.g. pipelines, configurations) in the future. We probably can make it more configurable or put it in separate groovy if more variants we are end up with.

Yes, it is my concern as well, this is being discussed on the mailing list, let's continue the discussion on https://lists.apache.org/thread.html/7c48200678c570c6e42d4933f8ea4f9324d82f7ae720b591a93ffcda@%3Cdev.beam.apache.org%3E.

tvalentyn

thanks, @angoenka @markflyhigh . PTAL.

tvalentyn · 2019-06-07T17:10:21Z

sdks/python/apache_beam/runners/portability/portable_runner.py

+ else:
+ version_suffix = '3'
+ # TODO(BEAM-7474): Use an image which has correct Python minor version.
+ logging.warning('Make sure that locally built Python SDK docker image '


We don't have a release process for this image yet, so we build it locally when we run the test.

tvalentyn · 2019-06-07T17:16:54Z

sdks/python/apache_beam/runners/portability/portable_runner.py

+ 'has Python %d.%d interpreter. See also: BEAM-7474.' % (
+ sys.version_info[0], sys.version_info[1]))
+
+ return ('{user}-docker-apache.bintray.io/beam/python'


tvalentyn · 2019-06-07T17:17:23Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

+
+ project.task('portableWordCount' + nameSuffix) {
+ dependsOn 'installGcpTest'
+ dependsOn = ['installGcpTest']


good catch.

tvalentyn · 2019-06-07T17:29:44Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

@@ -1815,6 +1815,58 @@ class BeamModulePlugin implements Plugin<Project> {
 }
 }
 }
+
+ project.ext.addPortableWordCountTask = { String nameSuffix, boolean isStreaming ->


Good to know, thanks. I find explicit way more reader-friendly. Unless there is a reason (long list of optional parameters, etc), I think explicit is better than implicit.

tvalentyn · 2019-06-07T17:31:31Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

@@ -1815,6 +1815,58 @@ class BeamModulePlugin implements Plugin<Project> {
 }
 }
 }
+
+ project.ext.addPortableWordCountTask = { String nameSuffix, boolean isStreaming ->


Yes, it is my concern as well, this is being discussed on the mailing list, let's continue the discussion on https://lists.apache.org/thread.html/7c48200678c570c6e42d4933f8ea4f9324d82f7ae720b591a93ffcda@%3Cdev.beam.apache.org%3E.

tvalentyn · 2019-06-07T17:38:11Z

sdks/python/build.gradle

@@ -77,11 +77,14 @@ task preCommitPy2() {
 dependsOn "lint"
 }

-task portablePreCommit() {
+addPortableWordCountTask('BatchPy2', false)


We can, however, the portableWordCountExample definition would be more complicated. Do we meed that task at all? how is it used?
I can remove it if you prefer and remove one of the prefix parameter.

angoenka · 2019-06-07T20:57:14Z

Thanks @tvalentyn
Added my comments.

…WC tasks.

tvalentyn · 2019-06-12T17:18:36Z

Thanks, @angoenka, PTAL.

tvalentyn · 2019-06-12T19:02:47Z

Run Java_Examples_Dataflow PreCommit

tvalentyn · 2019-06-12T22:55:23Z

Run Python PreCommit

tvalentyn · 2019-06-12T22:58:16Z

Run Java PreCommit

tvalentyn force-pushed the flink_wordcount branch 2 times, most recently from a58d1e9 to b0a58b8 Compare June 3, 2019 20:20

angoenka reviewed Jun 6, 2019

View reviewed changes

markflyhigh reviewed Jun 7, 2019

View reviewed changes

tvalentyn commented Jun 7, 2019

View reviewed changes

tvalentyn force-pushed the flink_wordcount branch from 0a346bd to 1e96741 Compare June 12, 2019 17:11

tvalentyn added 4 commits June 12, 2019 10:18

Adds portable wordcount test for Python 3 with Flink runner.

9654de4

fixup: Address reviewers' comments.

eadfb4e

fixup: Lint.

6af742d

Remove portableWordCountExample tasks and simplify the helper to add …

9f2419b

…WC tasks.

tvalentyn force-pushed the flink_wordcount branch from 1e96741 to 9f2419b Compare June 12, 2019 17:18

fixup: Extra newline

4133709

angoenka merged commit af7ad3f into apache:master Jun 12, 2019

tvalentyn deleted the flink_wordcount branch October 23, 2019 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-7407] Adds portable wordcount test for Python 3 with Flink runner. #8745

[BEAM-7407] Adds portable wordcount test for Python 3 with Flink runner. #8745

tvalentyn commented Jun 3, 2019 •

edited

Loading

tvalentyn commented Jun 4, 2019

tvalentyn commented Jun 6, 2019

angoenka left a comment

angoenka Jun 6, 2019

tvalentyn Jun 7, 2019

angoenka Jun 6, 2019

tvalentyn Jun 7, 2019

angoenka Jun 7, 2019

tvalentyn Jun 12, 2019

angoenka Jun 6, 2019

tvalentyn Jun 7, 2019

angoenka Jun 7, 2019

tvalentyn Jun 12, 2019

markflyhigh left a comment

markflyhigh Jun 6, 2019

tvalentyn Jun 7, 2019

markflyhigh Jun 6, 2019

tvalentyn Jun 7, 2019

markflyhigh Jun 7, 2019

tvalentyn Jun 7, 2019

tvalentyn left a comment

tvalentyn Jun 7, 2019

tvalentyn Jun 7, 2019

tvalentyn Jun 7, 2019

tvalentyn Jun 7, 2019

tvalentyn Jun 7, 2019

tvalentyn Jun 7, 2019

angoenka commented Jun 7, 2019

tvalentyn commented Jun 12, 2019

tvalentyn commented Jun 12, 2019

tvalentyn commented Jun 12, 2019

tvalentyn commented Jun 12, 2019

[BEAM-7407] Adds portable wordcount test for Python 3 with Flink runner. #8745

[BEAM-7407] Adds portable wordcount test for Python 3 with Flink runner. #8745

Conversation

tvalentyn commented Jun 3, 2019 • edited Loading

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

tvalentyn commented Jun 4, 2019

tvalentyn commented Jun 6, 2019

angoenka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markflyhigh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvalentyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angoenka commented Jun 7, 2019

tvalentyn commented Jun 12, 2019

tvalentyn commented Jun 12, 2019

tvalentyn commented Jun 12, 2019

tvalentyn commented Jun 12, 2019

tvalentyn commented Jun 3, 2019 •

edited

Loading