[BEAM-115] Runner API representation of windowing strategies for Python by robertwb · Pull Request #2190 · apache/beam

robertwb · 2017-03-08T01:21:15Z

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify. (Even better, enable
Travis-CI on your fork and ensure the whole test matrix passes).
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

robertwb · 2017-03-08T01:24:14Z

asfbot · 2017-03-08T02:23:17Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8192/
--none--

kennknowles · 2017-03-08T02:39:29Z

Is the plan to check in the generated code?

tibkiss · 2017-03-08T12:28:20Z

sdks/python/apache_beam/pipeline.py

+
+  def _unique_ref(self):
+    self._counter += 1
+    return "ref_%s_%s" % (obj_type.__name__, self._counter)


obj_type is not available in this function, please use self._obj_type

Done. I'm going to add tests for this.

tibkiss · 2017-03-08T12:31:43Z

sdks/python/apache_beam/pipeline.py

+
+  def to_runner_api(self):
+    context_proto = beam_runner_api_pb2.Components()
+    for name, cls in self.__COMPONENT_TYEPS:


typo in self.__COMPONENT_TYEPS

Thanks for addressing my comments!
There is still a typo here: *TYEPS vs. *TYPES

Oh. Fixed. I'll add some tests to make sure this actually works an push again. (Oh, for a statically typechecked Python...)

tibkiss · 2017-03-08T12:33:10Z

sdks/python/apache_beam/transforms/core.py

    return self._is_default

+  def to_runner_api(self, context):
+    raise NotImplementedError


This probably not intentional as you are returning a WindowingStrategy the exception is raised

tibkiss · 2017-03-08T12:50:35Z

sdks/python/apache_beam/pipeline.py

+    context_proto = beam_runner_api_pb2.Components()
+    for name, cls in self.__COMPONENT_TYEPS:
+      getattr(self, name).populate_map(getattr(context_proto, name))
+    return components


Seems like components is undefined at this point

tibkiss · 2017-03-08T12:53:44Z

sdks/python/apache_beam/transforms/core.py

+
+  @staticmethod
+  def from_runner_api(self, proto, context):
+    return Windowing(


Windowing class has different named parameters: windowfn and triggerfn is used there (without underscore).

tibkiss · 2017-03-08T12:55:02Z

sdks/python/apache_beam/transforms/core.py

+  def from_runner_api(self, proto, context):
+    return Windowing(
+        window_fn=WindowFn.from_runner_api(proto.window_fn),
+        trigger_fn=TriggerFn.from_runner_api(proto.trigger),


TriggerFn was not imported in this file

Yes, there was a circular import. Fixed.

tibkiss · 2017-03-08T12:56:03Z

sdks/python/apache_beam/transforms/core.py

+        allowed_lateness=0)
+
+  @staticmethod
+  def from_runner_api(self, proto, context):


A staticmethod shall not need 'self' parameter

tibkiss · 2017-03-08T12:59:06Z

sdks/python/apache_beam/transforms/trigger.py

+    return beam_runner_api_pb2.Trigger(
+        after_each=beam_runner_api_pb2.Trigger.AfterEach(
+            subtriggers=[
+                subtrigger.to_runner_api(context) for subtrigger in triggers]))


typo: please replace triggers by self.triggers

Done and test added.

tibkiss · 2017-03-08T13:09:03Z

sdks/python/apache_beam/transforms/trigger.py

+    return beam_runner_api_pb2.Trigger(
+        or_finally=beam_runner_api_pb2.Trigger.OrFinally(
+            main=self.triggers[0].to_runner_api(context),
+            **{'finally': self.triggers[1].to_runner_api(context)}))


Maybe it's just me, but I needed some time to figure out what it does.
Wouldn't be simpler just passing finally=self.triggers[1].to_runner_api(context)?

Yeah, if only finally wasn't a keyword in Python, so I can't put it literally.

robertwb · 2017-03-08T16:57:41Z

Thanks for taking a look, @tibkiss. I though I had added tests for Windowing, but clearly not.

robertwb · 2017-03-08T17:06:35Z

@kennknowles As for checking in generated code, I poked around a bit, and asked around, and that's what we're currently doing for the other protos and apitools generated code. It'd be nice to avoid a protoc/mvn dependency for contributors, but I figured this was better than generating the json manually.

asfbot · 2017-03-08T17:23:07Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8211/
--none--

asfbot · 2017-03-08T17:23:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8212/
--none--

asfbot · 2017-03-08T20:41:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8216/
--none--

asfbot · 2017-03-08T20:49:45Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8218/
--none--

robertwb · 2017-03-08T21:48:53Z

R: @kennknowles

kennknowles · 2017-03-08T22:31:26Z

@robertwb with regard to checking it in, I actually don't have much of an objection. It seems like a potentially annoying problem, and this unblocks other work. Perhaps we'll get to a point where it is autogenerated later, and it won't be hard to adjust other things.

I'll take a little bit to get to this review in more depth, though. I trust the LGTM of someone involved in the Python SDK more deeply.

vikkyrk · 2017-03-08T23:54:59Z

sdks/python/apache_beam/utils/urns.py

+SLIDING_WINDOWS_FN = "beam:window_fn:sliding_windows:v0.1"
+SESSION_WINDOWS_FN = "beam:window_fn:session_windows:v0.1"
+
+PICKLED_CODER = "dataflow:coder:pickled_python:v0.1"


/s/dataflow/beam

vikkyrk · 2017-03-09T00:13:33Z

sdks/python/apache_beam/transforms/window.py

+  OUTPUT_AT_EOW = beam_runner_api_pb2.END_OF_WINDOW
+  OUTPUT_AT_EARLIEST = beam_runner_api_pb2.EARLIEST_IN_PANE
+  OUTPUT_AT_LATEST = beam_runner_api_pb2.LATEST_IN_PANE
  OUTPUT_AT_EARLIEST_TRANSFORMED = 'OUTPUT_AT_EARLIEST_TRANSFORMED'


This will fail when converting to_runner_api as it is not defined in the proto, maybe comment it out with a TODO?

vikkyrk · 2017-03-09T00:25:18Z

sdks/python/apache_beam/transforms/window.py

+    return pickler.loads(fn_parameter.value)
+
+  def to_runner_api_parameter(self, context):
+    raise TypeError(self)


vikkyrk · 2017-03-09T01:04:00Z

sdks/python/apache_beam/transforms/core.py

+        trigger=self.triggerfn.to_runner_api(context),
+        accumulation_mode=self.accumulation_mode,
+        output_time=self.output_time_fn,
+        closing_behavior=beam_runner_api_pb2.EMIT_ALWAYS,


Add a TODO to support EMIT_IF_NONEMPTY?

vikkyrk · 2017-03-09T01:15:32Z

sdks/python/apache_beam/pipeline.py

          visitor.visit_value(v, self)
+
+
+class PipelineContextMap(object):


Tests for PipelineContextMap and PipelineContext would be great.

vikkyrk · 2017-03-09T01:24:54Z

sdks/python/apache_beam/pipeline.py

+    return self._id_to_obj[id]
+
+
+class PipelineContext(object):


Is this a user facing class or should it it be in runners module.

asfbot · 2017-03-09T18:14:19Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8268/

Build result: FAILURE

[...truncated 1.96 MB...] at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoExecutionException: Command execution failed. at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:302) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 moreCaused by: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) at org.apache.commons.exec.DefaultExecutor.executeInternal(DefaultExecutor.java:404) at org.apache.commons.exec.DefaultExecutor.execute(DefaultExecutor.java:166) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:764) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:711) at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:289) ... 33 more2017-03-09T18:10:22.481 [ERROR] 2017-03-09T18:10:22.481 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-09T18:10:22.481 [ERROR] 2017-03-09T18:10:22.481 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-09T18:10:22.481 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-03-09T18:10:22.481 [ERROR] 2017-03-09T18:10:22.481 [ERROR] After correcting the problems, you can resume the build with the command2017-03-09T18:10:22.481 [ERROR] mvn -rf :beam-sdks-pythonchannel stoppedSetting status of d2f6d5b to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8268/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

asfbot · 2017-03-09T20:07:09Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8272/

Build result: FAILURE

[...truncated 1.69 MB...] at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoExecutionException: Command execution failed. at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:302) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 moreCaused by: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) at org.apache.commons.exec.DefaultExecutor.executeInternal(DefaultExecutor.java:404) at org.apache.commons.exec.DefaultExecutor.execute(DefaultExecutor.java:166) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:764) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:711) at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:289) ... 33 more2017-03-09T20:03:54.653 [ERROR] 2017-03-09T20:03:54.653 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-09T20:03:54.653 [ERROR] 2017-03-09T20:03:54.653 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-09T20:03:54.653 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-03-09T20:03:54.654 [ERROR] 2017-03-09T20:03:54.654 [ERROR] After correcting the problems, you can resume the build with the command2017-03-09T20:03:54.654 [ERROR] mvn -rf :beam-sdks-pythonchannel stoppedSetting status of 29cfcbe to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8272/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

coveralls · 2017-03-09T21:21:25Z

Coverage increased (+0.02%) to 70.175% when pulling 8a0db87 on robertwb:py-runner-api into 29d9bd3 on apache:master.

asfbot · 2017-03-09T21:31:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8278/
--none--

vikkyrk · 2017-03-09T21:59:38Z

LGTM.

asfbot · 2017-03-10T00:44:01Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8289/

Build result: FAILURE

[...truncated 1.96 MB...] at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoExecutionException: Command execution failed. at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:302) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 moreCaused by: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) at org.apache.commons.exec.DefaultExecutor.executeInternal(DefaultExecutor.java:404) at org.apache.commons.exec.DefaultExecutor.execute(DefaultExecutor.java:166) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:764) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:711) at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:289) ... 33 more2017-03-10T00:36:53.061 [ERROR] 2017-03-10T00:36:53.061 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-10T00:36:53.061 [ERROR] 2017-03-10T00:36:53.061 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-10T00:36:53.061 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-03-10T00:36:53.062 [ERROR] 2017-03-10T00:36:53.062 [ERROR] After correcting the problems, you can resume the build with the command2017-03-10T00:36:53.062 [ERROR] mvn -rf :beam-sdks-pythonchannel stoppedSetting status of 3bc8d8d to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8289/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

asfbot · 2017-03-10T01:55:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8293/

Build result: FAILURE

[...truncated 1.96 MB...] at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoExecutionException: Command execution failed. at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:302) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 moreCaused by: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) at org.apache.commons.exec.DefaultExecutor.executeInternal(DefaultExecutor.java:404) at org.apache.commons.exec.DefaultExecutor.execute(DefaultExecutor.java:166) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:764) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:711) at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:289) ... 33 more2017-03-10T01:50:03.593 [ERROR] 2017-03-10T01:50:03.593 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-10T01:50:03.593 [ERROR] 2017-03-10T01:50:03.593 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-10T01:50:03.593 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-03-10T01:50:03.593 [ERROR] 2017-03-10T01:50:03.594 [ERROR] After correcting the problems, you can resume the build with the command2017-03-10T01:50:03.594 [ERROR] mvn -rf :beam-sdks-pythonchannel stoppedSetting status of 0cc50dc to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8293/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

coveralls · 2017-03-10T03:11:52Z

Changes Unknown when pulling 86e1754 on robertwb:py-runner-api into ** on apache:master**.

asfbot · 2017-03-10T03:19:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8294/
--none--

tibkiss reviewed Mar 8, 2017

View reviewed changes

robertwb force-pushed the py-runner-api branch from 0e5abcf to 2a9d95a Compare March 8, 2017 16:55

robertwb force-pushed the py-runner-api branch 2 times, most recently from f077d5e to 36269d6 Compare March 8, 2017 18:57

vikkyrk reviewed Mar 9, 2017

View reviewed changes

robertwb force-pushed the py-runner-api branch from 8a0db87 to 3bc8d8d Compare March 9, 2017 23:25

Auto-generated runner api proto bindings.

69c2790

robertwb force-pushed the py-runner-api branch from 3bc8d8d to 0cc50dc Compare March 10, 2017 00:43

robertwb added 5 commits March 9, 2017 18:07

Runner API context helper classes.

d9e53cc

Runner API encoding of WindowFns.

269463c

Runner API translation of triggers and windowing strategies.

9dfa9b6

Add license to new files.

4cd2d8c

Move pipeline context and add more tests.

86e1754

robertwb force-pushed the py-runner-api branch from 0cc50dc to 86e1754 Compare March 10, 2017 02:08

asfgit closed this in 2c2424c Mar 10, 2017

		visitor.visit_value(v, self)


		class PipelineContextMap(object):

Conversation

robertwb commented Mar 8, 2017

Uh oh!

robertwb commented Mar 8, 2017

Uh oh!

asfbot commented Mar 8, 2017

Uh oh!

kennknowles commented Mar 8, 2017

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!

robertwb Mar 8, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertwb commented Mar 8, 2017

Uh oh!

robertwb commented Mar 8, 2017

Uh oh!

asfbot commented Mar 8, 2017

Uh oh!

asfbot commented Mar 8, 2017

Uh oh!

asfbot commented Mar 8, 2017

Uh oh!

asfbot commented Mar 8, 2017

Uh oh!

robertwb commented Mar 8, 2017

Uh oh!

kennknowles commented Mar 8, 2017

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

robertwb Mar 8, 2017 •

edited

Loading