[BEAM-6853] Make sdkWorkerParallelism option consistent #8286

angoenka · 2019-04-12T00:00:09Z

sdkWorkerParallelism behavior was not consistent in the previous implementation and was not easy to describe.
The new behavior of sdkWorkerParallelism is as follows.

A) User input sdkWorkerParallelism = null -> Default value of 0 is set by PortablePipelineOptions -> Jobserver Overwrite(default value 1) to 1 -> 1 is used as the final value and sdkWorkerParallelism is dynamically decided.

B) User input sdkWorkerParallelism = 5 -> No default overwrite by PortablePipelineOptions and 5 is passed along -> Jobserver Overwrite not used -> 5 is used as the final value.

C) User input sdkWorkerParallelism = null -> Default value of 0 is set by PortablePipelineOptions -> Jobserver Overwrite (2) used -> 2 is used as the final value.

D) User input sdkWorkerParallelism = 0 -> No default overwrite by PortablePipelineOptions and 0 is passed along -> Jobserver Overwrite(default value 0) to 0 -> 0 is used as the final value and actual worker parallelism is determined based on CPU.

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.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

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.

angoenka · 2019-04-12T21:07:23Z

R: @tvalentyn

angoenka · 2019-04-13T01:20:13Z

Run Java PreCommit

angoenka · 2019-04-13T01:30:07Z

Run Java PreCommit

angoenka · 2019-04-15T21:16:25Z

Run Java PreCommit

tvalentyn

Thanks, @angoenka

tvalentyn · 2019-04-15T20:34:01Z

sdks/python/apache_beam/options/pipeline_options.py


 class PortableOptions(PipelineOptions):
-
+  """Portable options are common options expected to be understood by most of


@angoenka What do you think about removing current RunnerOptions in Python SDK, and renaming PortableOptions to either RunnerOptions or PortableRunnerOptions in all SDKs? See also: #8225 (comment)
cc: @mxm, @robertwb .

Fine with me to remove RunnerOptions because it is just a stub. Not sure about PortableOptions => PortableRunnerOptions because we currently do not include Runner in any option name, e.g. FlinkPipelineOptions, not FlinkRunnerPipelineOptions.

I agree that we can remove the RunnerOptions as its not used anywhere.
I would like to stick with the current convention of not adding "Runner" to PortableOptions.

I think PortableOptions is understandable by developers but might be less user-friendly, since users might not know what portability refers to.

tvalentyn · 2019-04-15T21:08:11Z

...ecution/src/main/java/org/apache/beam/runners/fnexecution/jobsubmission/JobServerDriver.java

-        usage = "Default parallelism for SDK worker processes (see portable pipeline options)")
-    private Long sdkWorkerParallelism = 1L;
+        usage =
+            "Default parallelism for SDK worker processes (see portable pipeline options). "


Consider slightly different order: Parallelism level for SDK worker processes. This value is used only when the pipeline option sdkWorkerParallelism is set to 0, otherwise the value provided in pipeline options takes precedence. Default is 0, which means worker parallelism will be dynamically decided by a runner. See also: sdkWorkerParallelism Pipeline Option. Also, should last sentence be a Javadoc reference?

Makes sense. I will update the wording.

tvalentyn · 2019-04-15T21:22:24Z

sdks/python/apache_beam/options/pipeline_options.py

              'command.'))
    parser.add_argument(
-        '--sdk_worker_parallelism', default=None,
+        '--sdk_worker_parallelism', default=1,


Do we have strong reasons not to make 0 be a default value? Folks working on portability probably have an informed opinion on this. cc: @mxm, @robertwb.

I agree, we probably want 0.

I have seen machine lock down when running python processes equal to the number of cores.
We can still go with 0 here as long as we set the default to be 1 in JobServerDriver.java.
And I think this is reasonable.

angoenka · 2019-04-17T18:04:51Z

Run Java PreCommit

angoenka · 2019-04-17T18:05:02Z

Run Java_Examples_Dataflow PreCommit

angoenka · 2019-04-18T20:46:28Z

Ping @tvalentyn

tvalentyn

Thanks. Please update the PR description, looks like it does not reflect changes in last iteration.

angoenka · 2019-04-18T23:14:10Z

Thanks @tvalentyn
Updated the description and Merging the PR.

angoenka force-pushed the portable_options branch from b85fa32 to a583c98 Compare April 12, 2019 19:16

angoenka force-pushed the portable_options branch 2 times, most recently from 8bbe1a1 to 0a5c1af Compare April 12, 2019 21:24

angoenka changed the title ~~Make sdkWorkerParallelism option consistent~~ [BEAM-6853] Make sdkWorkerParallelism option consistent Apr 12, 2019

angoenka force-pushed the portable_options branch from 0a5c1af to 05a0472 Compare April 12, 2019 22:48

angoenka force-pushed the portable_options branch from 05a0472 to 44741d0 Compare April 13, 2019 01:29

tvalentyn reviewed Apr 15, 2019

View reviewed changes

angoenka force-pushed the portable_options branch from 44741d0 to c1225de Compare April 16, 2019 20:50

[BEAM-6853] Make sdkWorkerParallelism option consistent

f3623e8

angoenka force-pushed the portable_options branch from c1225de to f3623e8 Compare April 17, 2019 18:10

tvalentyn approved these changes Apr 18, 2019

View reviewed changes

angoenka merged commit fe676c5 into apache:master Apr 18, 2019


		class PortableOptions(PipelineOptions):

		"""Portable options are common options expected to be understood by most of

[BEAM-6853] Make sdkWorkerParallelism option consistent #8286

[BEAM-6853] Make sdkWorkerParallelism option consistent #8286

Uh oh!

Conversation

angoenka commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

angoenka commented Apr 12, 2019

Uh oh!

angoenka commented Apr 13, 2019

Uh oh!

angoenka commented Apr 13, 2019

Uh oh!

angoenka commented Apr 15, 2019

Uh oh!

tvalentyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angoenka commented Apr 17, 2019

Uh oh!

angoenka commented Apr 17, 2019

Uh oh!

angoenka commented Apr 18, 2019

Uh oh!

tvalentyn left a comment

Choose a reason for hiding this comment

Uh oh!

angoenka commented Apr 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

angoenka commented Apr 12, 2019 •

edited

Loading