[BEAM-115] Unify Java and Python WindowingStrategy representations. by robertwb · Pull Request #3222 · apache/beam

robertwb · 2017-05-25T00:28:51Z

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.
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-05-25T00:30:10Z

kennknowles · 2017-05-25T01:35:59Z

+  // This URN says that the WindowFn is just a UDF blob the Java SDK understands
  // TODO: standardize such things
-  public static final String CUSTOM_WINDOWFN_URN = "urn:beam:windowfn:javasdk:0.1";
+  public static final String SERIALIZED_JAVA_WINDOWFN_URN = "beam:windowfn:javasdk:v0.1";


Just curious - does the v add anything in particular?

Makes it clearer that this number is a version field.

I'd say drop it

We use 'v' elsewhere (e.g. in the package name itself). Otherwise it looks like a ratio or something.

It is in the package name to make it a valid package name. Not a huge deal. If you feel strongly about it I'm fine with it.

kennknowles · 2017-05-25T01:36:48Z

+package org.apache.beam.runner_api.v1;
+
+option java_package = "org.apache.beam.sdk.common.runner.v1";
+option java_outer_classname = "RunnerApiPayloads";


Let's name the classes and files consistently so they are easy to find.

I'm open for suggestions (haven't come up with names that feel right yet).

Here, how about StandardWindowFns in standard_window_fns.proto?

Sounds good. Done.

kennknowles · 2017-05-25T01:37:24Z

R: @kennknowles

kennknowles · 2017-05-25T01:46:25Z

-SLIDING_WINDOWS_FN = "beam:window_fn:sliding_windows:v0.1"
-SESSION_WINDOWS_FN = "beam:window_fn:session_windows:v0.1"
+PICKLED_WINDOW_FN = "beam:windowfn:pickled_python:v0.1"
+GLOBAL_WINDOWS_FN = "beam:windowfn:global_windows:v0.1"


We had discussed using a proto definition like

message WellKnownUrns { string global_windows = 1 [default = "urn:beam:windowfn:global_windows:0.1"] ... }

But I guess this is proto 2 only. For normal proto use, I agree with this change, but it is too bad we don't have that hack available. Any other ideas? A data file seems mostly pointless because whatever you look up the URN by is effectively just another unique name anyhow (it would probably prevent some errors).

Not that I think there is a huge danger of these getting out of sync, since they can never change once they are really used. But it would be nice for new SDKs, etc.

We could have a script that goes through the proto file (or something else) and pulls the name out and generates a .java, .py, .go, ... file of constants. Perhaps overkill though. Ideally this'd be co-located with the semantic definition of the urn as well, if needed.

kennknowles · 2017-05-25T01:52:16Z

+  public static final String SESSION_WINDOWS_FN = "beam:windowfn:session_windows:v0.1";
+  // This URN says that the WindowFn is just a UDF blob the Java SDK understands
  // TODO: standardize such things
-  public static final String CUSTOM_WINDOWFN_URN = "urn:beam:windowfn:javasdk:0.1";


FWIW the urn: at the beginning is the URI scheme that makes it a URN. The official syntax is "urn:" Namespace : Namespace-specific format

(https://tools.ietf.org/html/rfc8141#section-2)

I agree the scheme is mostly pointless but wanted to just note the departure.

(in particular, top level schemes and the remainder of the URI have exactly the same relationship as URN namespaces and the NSS, as far as I can tell)

I didn't know urns were an official thing with a spec... We can prefix everything with urn: if you want.

No, I just wanted to note it. I find the prefixing a bit tedious. I've been doing it because following internet standards is my first instinct, but I don't think it is worthwhile. We should lay out the namespaces in a section of the runner guide on the website.

kennknowles · 2017-05-25T02:12:11Z

(btw in commit message s/Stragegy/Strategy/)

tgroh

It seems that it would be worth adding the serialization as well here and testing the round tripping.

robertwb · 2017-05-25T19:38:25Z

We have serialization and round-tripping tests.

tgroh · 2017-05-25T19:47:18Z

What I mean is my understanding is that we do not have any logic to serialize known types (so this new deserialization code is not exercised). I'm happy to be wrong though.

robertwb · 2017-05-25T19:51:03Z

Ah, yes, the serialization logic is only available in Python.

robertwb · 2017-05-25T20:26:39Z

PTAL

tgroh · 2017-05-25T20:48:28Z

-import org.apache.beam.sdk.transforms.windowing.FixedWindows;
-import org.apache.beam.sdk.transforms.windowing.TimestampCombiner;
-import org.apache.beam.sdk.transforms.windowing.Trigger;
+import org.apache.beam.sdk.transforms.windowing.*;


No * imports

Yeah, just disabled that setting in intellej...

tgroh · 2017-05-25T20:56:49Z

+            windowFnSpec.getSpec().getParameter().unpack(
+                RunnerApiPayloads.SlidingWindowsPayload.class);
+        return SlidingWindows.of(
+            Duration.millis(Durations.toMillis(slidingParams.getSize())))


The formatting here is pretty funky.

These are long lines, you have to break somewhere. I generally abide by the rectangle rule when I have to break up large expressions.

FWIW since you mentioned IntelliJ there's google-java-format plugin which is stellar.

tgroh · 2017-05-25T20:58:07Z

-                            .build())))
-        .build();
+    // TODO: Set environment IDs
+    if (windowFn instanceof GlobalWindows) {


These should probably translated similarly to how we're translating the PTransform Payloads (i.e. with a registrar and some interface that can translate to and from a known type in some meaningful format. We probably want to wait until we've determined that API, though.)

Yeah, I'll defer on that.

robertwb · 2017-05-26T15:22:26Z

Jenkins: retest this please

tgroh · 2017-05-26T15:32:03Z

retest this please

robertwb · 2017-05-26T16:59:49Z

Jenkins: retest this please

coveralls · 2017-05-26T18:29:11Z

Coverage increased (+0.1%) to 70.836% when pulling f5ff727 on robertwb:runner-windows into 3643676 on apache:master.

robertwb · 2017-05-30T17:28:55Z

Connect to conjars.org:80 [conjars.org/107.22.169.25] failed: Connection timed out

retest this please

robertwb · 2017-05-30T17:29:19Z

retest this please

asfbot · 2017-05-30T18:09:48Z

--none--

asfbot · 2017-05-30T18:10:23Z

--none--

robertwb · 2017-05-31T19:56:09Z

Retest this please

robertwb · 2017-05-31T21:08:25Z

Jenkins: retest this please (Apache Beam :: SDKs :: Java :: IO :: HBase failed downloading dependencies.)

coveralls · 2017-06-01T00:15:27Z

Coverage increased (+0.01%) to 70.679% when pulling 97e72a0 on robertwb:runner-windows into 2df9dbd on apache:master.

robertwb · 2017-06-01T00:23:58Z

Jenkins is happy, PTAL.

kennknowles · 2017-06-01T03:03:31Z

Jenkins appears sad due to a typo in an attribute name.

kennknowles

LGTM once you get Jenkins happy.

kennknowles · 2017-06-01T03:07:08Z

-    return (WindowFn<?, ?>) deserializedWindowFn;
+      default:
+        throw new IllegalArgumentException(
+            "Unknown or unsupported WindowFn: " + windowFnSpec.getSpec().getUrn());


nit: use WindowFn.class.getSimpleName()

kennknowles · 2017-06-01T03:08:20Z

+    self.assertEqual(
+        strategy,
+        DataflowRunner.deserialize_windowing_strategy(
+            DataflowRunner.serialize_windwoing_strategy(strategy)))


Here's the typo.

kennknowles · 2017-06-01T03:11:50Z

+                offset=proto_utils.from_micros(
+                    timestamp_pb2.Timestamp, self.offset.micros)))
+
+  @urns.RunnerApiFn.register_urn(


One design decision on the Java side (which I don't feel wedded to) is that URNs and payloads are entirely divorced from their Java native classes. I had imagined adding PTransform.getUrn() but instead we have a registrar from Java class to URN and payload which lives entirely outside the core SDK to keep proto off the API surface.

I don't entirely understand the ramifications of the Python SDK making both of these decisions separately. (PTransform having their URN, proto messages being right there too). I'd love your thoughts on it sometime, and whether we can easily make Java more natural.

There are certainly pros and cons to each, but we should be consistent. Certainly going from the urn to object requires some kind of registration (even if the other way around can be handled by polymorphism).

Probably wouldn't hurt to put together a doc detailing these design choices, which wouldn't have to all be resolved now (but before the Runner/Fn Api is public we would make sure all SDKs are consistent).

I generally think that two-way translation is better via separate registered translators than dynamic dispatch. So that is where the design comes from. But actually, we don't really do it, for a specific reason. When we are parsing a proto that has URN "beam:transforms:pardo" we don't try to make a Java ParDo object because the types wouldn't work out, etc. Instead all transforms just become RawPTransform that can vend its URN and language-agnostic bits of the payload.

I am currently fleshing out the kinds of access runners will need and will then add all this to the runner guide.

Just notes, nothing particularly needed here.

asfbot · 2017-06-01T17:03:20Z

Build finished.
--none--

tgroh

LGTM, with a nit.

tgroh · 2017-06-01T17:46:52Z

+                                      SerializableUtils.serializeToByteArray(windowFn)))
+                              .build())))
+          .build();
+    } else if (windowFn instanceof GlobalWindows) {


We should have some sort of registration scheme for WindowFns in the medium-long term, even if only to simplify this method. File a JIRA?

Created https://issues.apache.org/jira/browse/BEAM-2399.

coveralls · 2017-06-02T09:13:09Z

Coverage decreased (-0.003%) to 70.678% when pulling 3b23198 on robertwb:runner-windows into 217f085 on apache:master.

kennknowles reviewed May 25, 2017

View reviewed changes

robertwb force-pushed the runner-windows branch from 395e33c to 84971c7 Compare May 25, 2017 15:03

robertwb changed the title ~~[BEAM-115] Unify Java and Python WindowingStragegy representations.~~ [BEAM-115] Unify Java and Python WindowingStrategy representations. May 25, 2017

robertwb force-pushed the runner-windows branch from 84971c7 to 6b928b9 Compare May 25, 2017 17:09

tgroh reviewed May 25, 2017

View reviewed changes

robertwb force-pushed the runner-windows branch from 14c5d18 to d078715 Compare May 31, 2017 20:39

robertwb force-pushed the runner-windows branch from 97e72a0 to 22be749 Compare June 1, 2017 00:23

kennknowles approved these changes Jun 1, 2017

View reviewed changes

tgroh approved these changes Jun 1, 2017

View reviewed changes

robertwb force-pushed the runner-windows branch from 2c28ade to 9754415 Compare June 1, 2017 23:40

robertwb and others added 4 commits June 2, 2017 00:46

Unify Java and Python WindowingStrategy representations.

8d75359

Add known window serialization for Java.

25d4672

Ignore all proto generated files.

a393cb6

Increase dataflow worker version.

3b23198

robertwb force-pushed the runner-windows branch from 89c73a9 to 3b23198 Compare June 2, 2017 07:46

asfgit closed this in 2f9428c Jun 2, 2017

Conversation

robertwb commented May 25, 2017

Uh oh!

robertwb commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertwb May 25, 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!

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!

kennknowles commented May 25, 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!

kennknowles commented May 25, 2017

Uh oh!

tgroh left a comment

Choose a reason for hiding this comment

Uh oh!

robertwb commented May 25, 2017

Uh oh!

tgroh commented May 25, 2017

Uh oh!

robertwb commented May 25, 2017

Uh oh!

robertwb commented May 25, 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!

robertwb commented May 26, 2017

Uh oh!

tgroh commented May 26, 2017

Uh oh!

robertwb commented May 26, 2017

Uh oh!

coveralls commented May 26, 2017

Uh oh!

robertwb commented May 30, 2017

Uh oh!

robertwb commented May 30, 2017

Uh oh!

asfbot commented May 30, 2017

Uh oh!

robertwb May 25, 2017 •

edited

Loading

kennknowles Jun 1, 2017 •

edited

Loading