[BEAM-7850] Makes environment ID a top level attribute of PTransform. by chamikaramj · Pull Request #10183 · apache/beam

chamikaramj · 2019-11-21T00:09:00Z

Removes SDKFunctionSpec and replaces all usages of it with FunctionSpec.

Maintains the current behavior as much as possible. For example, added a list of known transform for which we'll be setting the environment (will leave the default value, empty string, for others). If needed, this behavior can be changed in a future change.

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.

See the Contributor Guide for more tips on how to make review process smoother.

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.

chamikaramj · 2019-11-21T00:10:57Z

cc: @robertwb and @lukecwik

I'm adding the rest of the refactoring needed for this but can you take a quick look to see if the proto changes look good ?

I did not preserve tags since I think we do not worry about backwards compatibility at this point but lemme know if I should. Thanks.

robertwb · 2019-11-21T01:51:38Z

The proto changes makes sense to me. The one bit I'm not sure of is how this will look for cross-language UDFs, but I think that will still look very different than the current SdfFunctionSpecs, and possibly will be modeled as "side" PTransforms which this would be in line with.

chamikaramj · 2019-11-21T01:59:21Z

Thanks. Yeah, design for cross-language UDFs should be done separately and SdkFunctionSpec is inadequate for this.

lukecwik · 2019-11-21T20:52:26Z

model/pipeline/src/main/proto/beam_runner_api.proto

All other uses of SdkFunctionSpec are part of a transform but finding the envrionment for the windowing strategy that the window_fn is part of when looking at a PCollection is difficult since one needs to find the environment that the upstream assign windows transform is part of. It is possible but annoying. Also this is different then coders which don't have an environment since both the upstream and downstream transforms need to understand the encoding and runners have the length prefixing technique to convert coders from unknown ones to known ones.

For now I'm for not adding an environment id here and forcing that graph traversal for the assign windows transform to find the environment and in the future to truly support custom window fns its likely we will have to come up with techniques like the coder length prefixing to make it so that they can be treated opaquely. (note that rest of the change looks good)

@chamikaramj @robertwb what do you think?

That makes sense and I'm actually running into this issue while updating SDKs. We probably have to add environment_id to WindowingStrategy since it's not directly attached to PTransform.

The windowing strategy cannot be coerced into an environment-agnostic one the same way a coder can be, as it involves actually executing user code as oppose to specifying constraints on its behavior. (In some sense, it is in a very real sense a cross-language UDF.)

Having to traverse the graph is ugly, but I suppose possible for now.

After hearing about some of the difficulties that Cham is running into. I would go either way on whether we add an environment id here or remove it completely.

Added an environment_id to WindowingStrategy for now (retaining the current behavior). This can be further simplified in the future if needed.

chamikaramj · 2019-12-12T21:46:08Z

R: @robertwb

I'm trying to get the last set of failing tests to work but I think this is ready to be reviewed now.

robertwb

Thanks for this cleanup.

robertwb · 2019-12-16T23:45:08Z

...-construction-java/src/main/java/org/apache/beam/runners/core/construction/Environments.java

Nit: for consistency, order this as above.

robertwb · 2019-12-16T23:51:23Z

...tion-java/src/main/java/org/apache/beam/runners/core/construction/PTransformTranslation.java

Seems it'd be better to have a prohibited list. Or possibly call these sdkTransformsWithEnvironment as "known" means lots of different things, and Flatten, GBK, ... would certainly seem to belong to a known this list.

Renamed to sdkTransformsWithEnvironment since we are trying to maintain current behavior in this PR. In the future I think we should set environment ID in all transforms by default and let runners override for naively implemented transforms. Created https://issues.apache.org/jira/browse/BEAM-9001 for this.

robertwb · 2019-12-16T23:56:19Z

sdks/python/apache_beam/pipeline.py

As for Java, known is probably not the right adjective here.

This is not needed anymore. Removed.

robertwb · 2019-12-16T23:57:12Z

sdks/python/apache_beam/pipeline.py

(On that note, a set of environment-less ones probably makes more sense, as it's more likely to be a fixed set.)

Ditto. Renamed the variable and created https://issues.apache.org/jira/browse/BEAM-9001.

robertwb · 2019-12-16T23:59:12Z

sdks/python/apache_beam/pipeline.py

Why is this needed given the above?

(Also, if it is needed, should it be nested for clarity, i.e.

if not environment_id: if transform_urn and transform_urn in KNOWN_SDK_TRANSFORMS: environment_id = context.default_environment_id() elif ...:

Only one is needed. Removed the other.

robertwb · 2019-12-17T00:06:42Z

sdks/python/apache_beam/transforms/ptransform.py

These are specific to a runner, shouldn't be here.

Moved back to original location.

robertwb · 2019-12-17T00:07:33Z

sdks/python/apache_beam/transforms/ptransform.py

unneeded now?

Moved back to original location.

robertwb · 2019-12-17T00:08:17Z

sdks/python/apache_beam/transforms/ptransform.py

This feels a bit out of place here, I'd move it back to where it was.

Moved back to original location.

youngoli · 2019-12-17T23:01:21Z

Heads up, in #10393 I updated the Go proto compiler version and rebuilt the generated code.You'll probably need to update the generated Go code before merging this (and make sure you update the protoc-gen-go version to the most recent one this time). If it says the generated code is using ProtoVersion3 instead of 2, then your change is good to go.

chamikaramj · 2019-12-19T08:08:27Z

Run Python2_PVR_Flink PreCommit

chamikaramj · 2019-12-19T08:09:51Z

Run Flink ValidatesRunner

chamikaramj · 2019-12-19T08:14:31Z

Run Python PreCommit

chamikaramj

Thanks. PTAL.

chamikaramj · 2019-12-19T21:07:48Z

model/pipeline/src/main/proto/beam_runner_api.proto

Added an environment_id to WindowingStrategy for now (retaining the current behavior). This can be further simplified in the future if needed.

chamikaramj · 2019-12-19T21:12:19Z

...-construction-java/src/main/java/org/apache/beam/runners/core/construction/Environments.java

chamikaramj · 2019-12-19T21:23:37Z

...tion-java/src/main/java/org/apache/beam/runners/core/construction/PTransformTranslation.java

Renamed to sdkTransformsWithEnvironment since we are trying to maintain current behavior in this PR. In the future I think we should set environment ID in all transforms by default and let runners override for naively implemented transforms. Created https://issues.apache.org/jira/browse/BEAM-9001 for this.

chamikaramj · 2019-12-19T21:25:42Z

sdks/python/apache_beam/pipeline.py

This is not needed anymore. Removed.

chamikaramj · 2019-12-19T21:25:51Z

sdks/python/apache_beam/pipeline.py

Ditto. Renamed the variable and created https://issues.apache.org/jira/browse/BEAM-9001.

chamikaramj · 2019-12-19T21:31:05Z

sdks/python/apache_beam/pipeline.py

Ditto. Renamed the variable and created https://issues.apache.org/jira/browse/BEAM-9001.

chamikaramj · 2019-12-19T21:31:35Z

sdks/python/apache_beam/pipeline.py

Only one is needed. Removed the other.

chamikaramj · 2019-12-19T21:44:11Z

sdks/python/apache_beam/transforms/ptransform.py

Moved back to original location.

chamikaramj · 2019-12-19T21:44:15Z

sdks/python/apache_beam/transforms/ptransform.py

Moved back to original location.

chamikaramj · 2019-12-19T21:44:20Z

sdks/python/apache_beam/transforms/ptransform.py

Moved back to original location.

chamikaramj · 2019-12-19T23:46:56Z

Run PythonLint PreCommit

chamikaramj · 2019-12-19T23:48:34Z

Run Flink ValidatesRunner

chamikaramj · 2019-12-19T23:48:43Z

Run Spark ValidatesRunner

chamikaramj · 2019-12-19T23:48:52Z

Run Dataflow ValidatesRunner

robertwb

Looks good to me, thanks!

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

Removes SDKFunctionSpec and replaces all usages of it with FunctionSpec.

chamikaramj

Thanks!

lukecwik reviewed Nov 21, 2019

View reviewed changes

chamikaramj force-pushed the mv_environment_to_ptransform branch 5 times, most recently from 16d484b to 6f57355 Compare December 10, 2019 01:38

chamikaramj force-pushed the mv_environment_to_ptransform branch from 66d8153 to a636ea0 Compare December 12, 2019 19:42

robertwb reviewed Dec 17, 2019

View reviewed changes

chamikaramj force-pushed the mv_environment_to_ptransform branch from dedbe59 to 31eddd1 Compare December 17, 2019 22:05

chamikaramj force-pushed the mv_environment_to_ptransform branch from 31eddd1 to dc3fd94 Compare December 19, 2019 04:04

chamikaramj commented Dec 19, 2019

View reviewed changes

robertwb approved these changes Dec 19, 2019

View reviewed changes

sdks/python/apache_beam/runners/portability/fn_api_runner_transforms.py Outdated Show resolved Hide resolved

Makes environment ID a top level attribute of PTransform.

edff83f

Removes SDKFunctionSpec and replaces all usages of it with FunctionSpec.

chamikaramj force-pushed the mv_environment_to_ptransform branch from 084d4d9 to edff83f Compare December 20, 2019 00:05

chamikaramj commented Dec 20, 2019

View reviewed changes

chamikaramj merged commit 3174445 into apache:master Dec 20, 2019

lostluck self-assigned this Dec 23, 2019

Conversation

chamikaramj commented Nov 21, 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!

chamikaramj commented Nov 21, 2019

Uh oh!

robertwb commented Nov 21, 2019

Uh oh!

chamikaramj commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

chamikaramj commented Dec 12, 2019

Uh oh!

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

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!

youngoli commented Dec 17, 2019

Uh oh!

chamikaramj commented Dec 19, 2019

Uh oh!

chamikaramj commented Dec 19, 2019

Uh oh!

chamikaramj commented Dec 19, 2019

Uh oh!

chamikaramj 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

chamikaramj commented Nov 21, 2019 •

edited

Loading

chamikaramj commented Nov 21, 2019 •

edited

Loading