[BEAM-8351] Support passing in arbitrary KV pairs to sdk worker via external environment config by violalyu · Pull Request #9730 · apache/beam

violalyu · 2019-10-04T16:32:25Z

Originally, the environment config for environment type of EXTERNAL only support passing in an url for the external worker pool; We want to support passing in arbitrary KV pairs to sdk worker via external environment config, so that the when starting the sdk harness we could get the values from StartWorkerRequest.params.
Jira issue: https://issues.apache.org/jira/browse/BEAM-8351

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	SDK	Apex	Dataflow	Gearpump	Samza	Spark
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.

violalyu · 2019-10-04T16:33:29Z

R: @robertwb
R: @chadrik

chadrik · 2019-10-04T16:45:39Z

R: @mxm

chadrik · 2019-10-04T16:49:13Z

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

    elif environment_urn == common_urns.environments.EXTERNAL.urn:
+      def _looks_like_json(environment_config):
+        import re
+        return re.match(r'\{.+\}', environment_config)


use re.search(). also, I don't think this needs to be private, so remove the leading underscore.

maybe add a comment like: "we don't use json.loads to test validity because we don't want to propagate json syntax errors downstream to the runner"

Thanks! For the re.search() part, do we want it to be not searching from the start? I was thinking that if it is valid json string it should start and end with '{}', maybe re.match(r'\{.+\}$') or re.search(r'^\{.+\}$')?

There's also whitespace to consider. So if we're using re.match we have to do re.match(r'\s*\{.*\}\s*$').

We just need a simple heuristic that tells us that it looks json and not a url. The most important thing is that we don't end up with a regex that incorrectly returns False for something that is actually json, so I suggested the re.search option because I think it's sufficient at detecting something that's not a url and thus probably json. That said, I think the re.match regex above looks pretty safe. Obviously, we're only talking about json objects (i.e. dict) and not arrays and other scalars.

Just to reiterate, we want json syntax errors to occur here, at submission time, and not later on, so we want to answer "did the user attempt to pass a json map or a url?" So I was even considering return '{' in config or '}' in config, since curly braces should not be in a url.

I'll defer the final answer on this to the reviewer.

Thanks Chad! Updated for now in 7fed22b!

chadrik · 2019-10-04T20:06:13Z

11:58:18 ************* Module apache_beam.runners.portability.portable_runner_test
11:58:18 W:328, 0: Bad indentation. Found 8 spaces, expected 6 (bad-indentation)
11:58:18 W:333, 0: Bad indentation. Found 8 spaces, expected 6 (bad-indentation)
11:58:18 C:337, 0: Line too long (82/80) (line-too-long)

chadrik · 2019-10-04T21:44:13Z

14:36:52 apache_beam/runners/portability/portable_runner_test.py:340:1: E302 expected 2 blank lines, found 1

to run the lint tests locally you do something like:

python -m virtualenv .venv
. .venv/bin/activate
pip install tox
cd sdks/python
tox -e py27-lint

chadrik · 2019-10-05T01:38:05Z

Run Python2_PVR_Flink PreCommit

mxm

Thanks @violalyu. Looks good to me. Just some minor comments.

mxm · 2019-10-05T10:35:54Z

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

+
+      if looks_like_json(portable_options.environment_config):
+        config = json.loads(portable_options.environment_config)
+        url = config.pop('url', None)


Suggested change

url = config.pop('url', None)

url = config.get('url', None)

Why pop?

Thanks Maximilian! I've updated in this commit: 9578559

mxm · 2019-10-05T10:39:02Z

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

+    self.assertEqual(
+        PortableRunner._create_environment(PipelineOptions.from_dictionary({
+            'environment_type': "EXTERNAL",
+            'environment_config': ' {"url":"localhost:50000", '


Should we also be testing the case without any space at the beginning?

I updated the test for this part in this commit: 916e170

mxm · 2019-10-07T08:26:40Z

Thanks!

mxm · 2019-10-07T08:28:06Z

@violalyu One minor note for future PRs, please also include the JIRA issue in the commit subject, just like in the GitHub issue title.

chadrik suggested changes Oct 4, 2019

View reviewed changes

violalyu force-pushed the external_payload_params branch from d239472 to 7fed22b Compare October 4, 2019 18:17

violalyu force-pushed the external_payload_params branch from 77db088 to a0679ef Compare October 4, 2019 21:33

violalyu force-pushed the external_payload_params branch 2 times, most recently from e822ff7 to 3a4b335 Compare October 5, 2019 00:01

mxm approved these changes Oct 5, 2019

View reviewed changes

violalyu added 2 commits October 5, 2019 16:18

Add support to pass params along to external worker pool.

9578559

Add unittest for external environment.

916e170

violalyu force-pushed the external_payload_params branch from 3a4b335 to 916e170 Compare October 5, 2019 20:18

mxm merged commit 64e5a83 into apache:master Oct 7, 2019

Conversation

violalyu commented Oct 4, 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!

violalyu commented Oct 4, 2019

Uh oh!

chadrik commented Oct 4, 2019

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!

violalyu Oct 4, 2019 • 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!

chadrik commented Oct 4, 2019

Uh oh!

chadrik commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chadrik commented Oct 5, 2019

Uh oh!

mxm 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!

mxm commented Oct 7, 2019

Uh oh!

mxm commented Oct 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

violalyu commented Oct 4, 2019 •

edited

Loading

violalyu Oct 4, 2019 •

edited

Loading

chadrik commented Oct 4, 2019 •

edited

Loading