[BEAM-10527] Migrate Flink and Spark tests to pytest. by ibzib · Pull Request #12385 · apache/beam

ibzib · 2020-07-28T01:52:47Z

Motivation

The main goals of migrating to pytest are:

Get Junit structured test output (BEAM-10527).
Replace PortableRunnerTest's bespoke timeout mechanism with pytest's (BEAM-9011). ~~This also implicitly raises the timeout for all these tests from 60s to 600s, which should reduce the likelihood of timeout flakes (BEAM-8912).~~

Implementation

I originally wanted to do this in incremental changes, but I gradually realized a complete overhaul of these tests' configuration was needed. The main challenge was that flink_runner_test.py expected to be run as __main__, which is impossible with pytest. I basically reworked everything except the tests themselves; the tests themselves are unchanged.

Test parametrization

I left worker configuration in Gradle because it changes the test dependencies. I moved optimization and streaming into flink_runner_test.py because it removes the need to set up separate tox tasks, separate test result files, etc.
Since flink_job_server_driver, environment_type, and environment_config are all pipeline options, I decided to pass them to flink_runner_test.py by introducing a global pytest option, --test-pipeline-options. nose uses --test-pipeline-options for integration tests, so I figured this would be generally useful beyond just these tests in the future.

Bonus trivia

Prior to this change, we were running the exact same streaming test suite four times per Jenkins run.

Every flinkCompatibilityMatrix task ran the entirety of flink_runner_test.py which contained two classes: FlinkRunnerTest and FlinkRunnerTestOptimized. FlinkRunnerTestOptimized was basically the same thing as FlinkRunnerTest, but it added the pre_optimize=all experiment and skipped external transform tests, since the Python optimizer breaks external transforms (BEAM-7252). But we were also adding pre_optimize=all in Gradle, redundantly.

The old configuration looks like this:

dependsOn flinkCompatibilityMatrix(streaming: false, workerType: CompatibilityMatrixConfig.SDK_WORKER_TYPE.LOOPBACK)
dependsOn flinkCompatibilityMatrix(streaming: true, workerType: CompatibilityMatrixConfig.SDK_WORKER_TYPE.LOOPBACK)
dependsOn flinkCompatibilityMatrix(streaming: true, workerType: CompatibilityMatrixConfig.SDK_WORKER_TYPE.LOOPBACK, preOptimize: true)

Notice that pre-optimized batch is missing. This is because flinkCompatibilityMatrixBatchPreOptimize* would run FlinkRunnerTest with pre_optimize=all but without skipping the external transform tests, causing failure.

What about streaming, then? Well, the optimizer doesn't affect streaming pipelines at all:

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

Line 319 in 489cf2c

if not options.view_as(StandardOptions).streaming:

So in one invocation of flinkValidatesRunner, flinkCompatibilityMatrixStreamingLoopback would run FlinkRunnerTest (without pre_optimize=all) and FlinkRunnerTestOptimized, then flinkCompatibilityMatrixStreamingPreOptimizeLoopback would run FlinkRunnerTest (with pre_optimize=all) and FlinkRunnerTestOptimized (with pre_optimize=all twice). Besides the skips in FlinkRunnerTestOptimized, all four tests would be doing the exact same thing.

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

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang	---	---	---

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.

ibzib · 2020-07-28T17:53:53Z

Run Portable_Python PreCommit

ibzib · 2020-07-28T19:06:06Z

R: @udim @mxm

mxm

Thanks for the PR @ibzib! Great work on finding the test duplications. Having structured access to the test results via Jenkins will be very useful.

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

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

mxm · 2020-07-30T16:57:59Z

Run Java PreCommit

mxm

I couldn't find the published test results in Jenkins. Do we have to add this separately? https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Commit/6126/

ibzib · 2020-07-31T00:47:55Z

I couldn't find the published test results in Jenkins. Do we have to add this separately? https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Commit/6126/

This is because publishing the test results relies on a change to the Jenkins job config, and I didn't want to run seed job because Flink PVR is a precommit and if I made a mistake, run seed job could break it for others. I tried testing locally on dockerized Jenkins, but I couldn't get it to work.

ibzib · 2020-07-31T00:56:37Z

I got flinkCompatibilityMatrixPROCESS to pass on my machine by escaping the arguments via ${1@Q}. Apparently whatever shell Jenkins is using does not support this. I will have to find a better solution.

I have already spent a long time trying to fix quotes, so I can't help but wondering: why do we need flinkCompatibilityMatrixPROCESS in the first place, when it is not being run anywhere? If it's important, shouldn't we add it to some postcommit?

ibzib · 2020-07-31T01:00:52Z

I have already spent a long time trying to fix quotes, so I can't help but wondering: why do we need flinkCompatibilityMatrixPROCESS in the first place, when it is not being run anywhere? If it's important, shouldn't we add it to some postcommit?

Another solution I had in mind was reworking the --environment_config option. JSON blobs are unwieldy, and overloading the --environment_config option is confusing to the user. We could make each field in the PROCESS --environment_config blob its own argument, and then reject these arguments when environment_type != PROCESS.

udim

The approach to using pytest looks good. One comment.

sdks/python/scripts/run_pytest.sh

mxm · 2020-08-03T08:48:16Z

Thanks for fixing the quoting issue!

I got flinkCompatibilityMatrixPROCESS to pass on my machine by escaping the arguments via ${1@Q}. Apparently whatever shell Jenkins is using does not support this. I will have to find a better solution.

I think that feature only works in newer versions of bash.

why do we need flinkCompatibilityMatrixPROCESS in the first place, when it is not being run anywhere? If it's important, shouldn't we add it to some postcommit?

flinkCompatibilityMatrixPROCESS was how we ran the PVR tests to avoid a dependency on the container build (for speed). I was under the assumption that we are still doing that. That probably changed when we added the external transform tests which require the Java container. I agree that we should at least have a post commit.

ibzib · 2020-08-10T21:48:07Z

I have already spent a long time trying to fix quotes, so I can't help but wondering: why do we need flinkCompatibilityMatrixPROCESS in the first place, when it is not being run anywhere? If it's important, shouldn't we add it to some postcommit?

Another solution I had in mind was reworking the --environment_config option. JSON blobs are unwieldy, and overloading the --environment_config option is confusing to the user. We could make each field in the PROCESS --environment_config blob its own argument, and then reject these arguments when environment_type != PROCESS.

@mxm what do you think about https://issues.apache.org/jira/browse/BEAM-10671?

mxm · 2020-08-11T16:57:46Z

Another solution I had in mind was reworking the --environment_config option. JSON blobs are unwieldy, and overloading the --environment_config option is confusing to the user. We could make each field in the PROCESS --environment_config blob its own argument, and then reject these arguments when environment_type != PROCESS.

@mxm what do you think about https://issues.apache.org/jira/browse/BEAM-10671?

I think it makes sense. Especially for error reporting by having dedicated argument parsers for all environments parameters.

The default pytest timeout is 600s. Once all portable runner tests are migrated to pytest, we can use the pytest timeout instead of portable_runner_test's bespoke implementation. See apache#12385.

codecov · 2020-10-01T02:07:09Z

Codecov Report

Merging #12385 into master will increase coverage by 0.11%.
The diff coverage is 79.48%.

@@            Coverage Diff             @@
##           master   #12385      +/-   ##
==========================================
+ Coverage   82.39%   82.50%   +0.11%     
==========================================
  Files         453      453              
  Lines       54623    54612      -11     
==========================================
+ Hits        45005    45059      +54     
+ Misses       9618     9553      -65

Impacted Files	Coverage Δ
sdks/python/apache_beam/io/fileio.py	`95.80% <ø> (ø)`
sdks/python/apache_beam/io/gcp/bigquery.py	`80.23% <0.00%> (+0.14%)`	⬆️
sdks/python/apache_beam/io/gcp/bigquery_tools.py	`88.35% <0.00%> (ø)`
...eam/testing/benchmarks/nexmark/nexmark_launcher.py	`0.00% <0.00%> (ø)`
...pache_beam/runners/interactive/interactive_beam.py	`79.53% <66.66%> (ø)`
...dks/python/apache_beam/options/pipeline_options.py	`93.76% <70.58%> (ø)`
sdks/python/apache_beam/transforms/environments.py	`83.73% <83.33%> (ø)`
.../apache_beam/options/pipeline_options_validator.py	`98.69% <100.00%> (ø)`
...ache_beam/runners/interactive/recording_manager.py	`98.90% <100.00%> (ø)`
... and 15 more

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 4161140...7219836. Read the comment docs.

ibzib · 2020-10-01T16:05:02Z

Run Python Spark ValidatesRunner

ibzib · 2020-10-01T16:06:27Z

@udim @mxm I rebased this to use environment_options instead of environment_config, bypassing previous issues with string parsing. PTAL

mxm · 2020-10-01T18:04:49Z

This looks good to me after rebasing. Thanks for porting this to pytest!

boyuanzz · 2020-10-06T00:20:17Z

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

+# Run as
+#
+# pytest flink_runner_test.py \
+#     [--test_pipeline_options "--flink_job_server_jar=/path/to/job_server.jar \


--test-pipeline-options

Oh yeah, --test_pipeline_options is now required (even though it should be possible to leave it empty). Boyuan, would you mind filing a PR to fix this?

boyuanzz · 2020-10-06T00:39:31Z

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

+# pytest flink_runner_test.py \
+#     [--test_pipeline_options "--flink_job_server_jar=/path/to/job_server.jar \
+#                               --environment_type=DOCKER"] \
+#     [FlinkRunnerTest.test_method, ...]


And the test filter here doesn't work for me properly. The working version for me is

pytest flink_runner_test.py::TestClass:test_case --test-pipeline-options "--flink_job_server_jar=XXX --environment_type=XXX "

probot-autolabeler bot added build python labels Jul 28, 2020

ibzib force-pushed the BEAM-10527 branch 2 times, most recently from 8ae28f6 to a51fed8 Compare July 28, 2020 02:00

probot-autolabeler bot added the infra label Jul 28, 2020

mxm approved these changes Jul 30, 2020

View reviewed changes

mxm reviewed Jul 30, 2020

View reviewed changes

ibzib force-pushed the BEAM-10527 branch from f0f2ec0 to 5cf9720 Compare July 31, 2020 00:40

udim requested changes Jul 31, 2020

View reviewed changes

sdks/python/scripts/run_pytest.sh Outdated Show resolved Hide resolved

mxm approved these changes Aug 3, 2020

View reviewed changes

ibzib force-pushed the BEAM-10527 branch from 25713d8 to 266c9c6 Compare August 10, 2020 22:17

ibzib mentioned this pull request Aug 19, 2020

[BEAM-9118] Increase portable_runner_test timeout. #12633

Merged

4 tasks

ibzib mentioned this pull request Sep 29, 2020

[BEAM-10671] Add environment configuration fields as first-class pipeline options. #12576

Merged

4 tasks

ibzib force-pushed the BEAM-10527 branch 2 times, most recently from 6c233c2 to 2a0ad5d Compare October 1, 2020 01:46

[BEAM-10527] Migrate Flink and Spark tests to pytest.

134f177

Publish pytest Junit results.

7219836

ibzib force-pushed the BEAM-10527 branch from 2a0ad5d to 7219836 Compare October 1, 2020 01:49

mxm merged commit bd56002 into apache:master Oct 1, 2020

boyuanzz reviewed Oct 6, 2020

View reviewed changes

Conversation

ibzib commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Implementation

Test parametrization

Bonus trivia

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

ibzib commented Jul 28, 2020

Uh oh!

ibzib commented Jul 28, 2020

Uh oh!

mxm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxm commented Jul 30, 2020

Uh oh!

mxm left a comment

Choose a reason for hiding this comment

Uh oh!

ibzib commented Jul 31, 2020

Uh oh!

ibzib commented Jul 31, 2020

Uh oh!

ibzib commented Jul 31, 2020

Uh oh!

udim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mxm commented Aug 3, 2020

Uh oh!

ibzib commented Aug 10, 2020

Uh oh!

mxm commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ibzib commented Oct 1, 2020

Uh oh!

ibzib commented Oct 1, 2020

Uh oh!

mxm commented Oct 1, 2020

Uh oh!

boyuanzz Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

ibzib Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

boyuanzz Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ibzib commented Jul 28, 2020 •

edited

Loading

mxm commented Aug 11, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading

boyuanzz Oct 6, 2020 •

edited

Loading