[BEAM-4059] Reduce number of ValidatesRunner tests and reorganize them for better parallelization #5193

swegner · 2018-04-20T17:04:23Z

Dataflow ValidatesRunner test suite has gotten obscenely slow due to the number of ValidatesRunner tests. There are currently over 250, and each one takes at least 3 minutes to run on Dataflow. Many of them don't actually need to be run on every worker, and this test converts them to @NeedsRunner tests instead.

Gradle also parallelizes tests differently, parallelizing at the test class level rather than per test case. As a result, the largest test class will be a bottleneck for the overall execution. This PR splits up large @ValidatesRunner test classes into scenario-based subclasses.

Follow this checklist to help us incorporate your contribution quickly and easily:

swegner · 2018-04-20T17:05:00Z

Run Dataflow ValidatesRunner

swegner · 2018-04-20T17:30:25Z

Run Dataflow ValidatesRunner

swegner · 2018-04-20T17:30:35Z

R: @kennknowles

kennknowles

Extraordinarily helpful. Great. I had a couple things - PAssert and Flatten - that really need to be VR in my opinion. Then Combine is a gray area worth a follow-up discussion or factoring, and Create is similar but less important. And those TestPipeline test methods just need fixing.

kennknowles · 2018-04-20T18:19:04Z

runners/direct-java/build.gradle

@@ -75,6 +76,8 @@ dependencies {
  needsRunner project(path: ":beam-sdks-java-core", configuration: "shadowTest")
  needsRunner project(path: project.path, configuration: "shadow")
  needsRunner project(path: project.path, configuration: "shadowTest")
+  validatesRunner project(path: ":beam-sdks-java-core", configuration: "shadowTest")


This will duplicate them, I think. Did you confirm? Because ValidatesRunner extends NeedsRunner, all the VR tests should be pulled in by the other execution. I think your version is cleaner, so I would agree to break this relationship. We could do NR minus VR for the needsRunner gradle suite, or just solve it with documentation/rename.

Added a comment; the intention is to have a DirectRunner version of the @ValidatesRunner suite so that I can run the same set of tests but in a faster in-memory runner. It's not being used for Jenkins.

kennknowles · 2018-04-20T18:21:44Z

sdks/java/core/src/test/java/org/apache/beam/sdk/io/AvroIOTest.java

@@ -672,13 +672,13 @@ public void populateDisplayData(DisplayData.Builder builder) {
  }

  @Test
-  @Category({ValidatesRunner.class, UsesTestStream.class})
+  @Category({NeedsRunner.class, UsesTestStream.class})


+1 to this and all tests that are not one of the core primitives. It is true that complex transforms sometimes exercise primitives in exciting ways, but we can and should run integration tests separately from VR which are basically compliance unit tests.

I left a few that seemed like they may be exercising per-runner functionality. I agree they probably shouldn't be necessary, but I'm not ready to go through and validate that we have sufficient integration test coverage before removing.

kennknowles · 2018-04-20T18:23:38Z

sdks/java/core/src/test/java/org/apache/beam/sdk/testing/PAssertTest.java

@@ -173,7 +173,7 @@ public void testSuccessEncodedDecoded() throws IOException {
   * serializable.
   */
  @Test
-  @Category(ValidatesRunner.class)
+  @Category(NeedsRunner.class)


I actually think that PAssertTest might be an exception - even though it is a composite transform, it is important to run alongside the VR runner suite to make sure the VR suite is meaningful. At least the particular pipelines that make sure suites are vacuous.

kennknowles · 2018-04-20T18:24:58Z

sdks/java/core/src/test/java/org/apache/beam/sdk/testing/TestPipelineTest.java

@@ -208,21 +208,21 @@ public String apply(final String input) {
      @Rule
      public final transient RuleChain chain = RuleChain.outerRule(exception).around(pipeline);

-      @Category(ValidatesRunner.class)
+      @Category(NeedsRunner.class)


This one is also iffy, maybe? But really these tests are also violating the core tenet of tests which is straight-line readability. So I can't actually tell at a glance what kind of test they really are. So go ahead and leave this as you have it, I guess.

kennknowles · 2018-04-20T18:26:19Z

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/CombineTest.java

@@ -184,13 +184,13 @@ public void testSimpleCombineWithContext() {
  }

  @Test
-  @Category(ValidatesRunner.class)
+  @Category(NeedsRunner.class)


For Combine there's an argument to bring some of them back since this is almost always implemented as a primitive, even though it is not one, technically. I would favor a JIRA TODO for factoring Combine into two suites - one for the composite's various configs but one that is focused on testing a runner's basic primitive implementation.

Ok, I've reverted these changes and instead subdivided the tests into subclasses.

kennknowles · 2018-04-20T18:27:41Z

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/CreateTest.java

@@ -224,7 +224,7 @@ public UnserializableRecord decode(
  }

  @Test
-  @Category(ValidatesRunner.class)
+  @Category(NeedsRunner.class)


How many runners are currently overriding Create as a primitive? If they aren't, then done! If they are, then (and this is a new thought that I thought of after my prior comments) another place for the tests to move is into ITs for just that runner, via some organization of modules. This is after-this-PR suggestion. I'm OK with this as-is unless runners are all turning it back into a primitive.

Ack, I'll leave as-is.

kennknowles · 2018-04-20T18:28:25Z

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/FlattenTest.java

@@ -138,7 +138,7 @@ public void testFlattenPCollectionsEmpty() {
  }

  @Test
-  @Category(ValidatesRunner.class)
+  @Category(NeedsRunner.class)


Flatten.pCollections() is a primitive that should be ValidatesRunner.

Flatten.iterables() is unrelated.

Ack, reverted.

swegner · 2018-04-20T19:03:15Z

Thanks for the quick review! All feedback is addressed; PTAL @kennknowles

kennknowles · 2018-04-24T22:07:56Z

run java precommit

swegner · 2018-04-24T22:10:48Z

It seems the @Enclosed test runner doesn't like non-static inner classes. I'll work on converting new subclasses to static: https://scans.gradle.com/s/npfiidsczvefo/tests/failed

swegner · 2018-05-02T16:10:43Z

Run Dataflow ValidatesRunner

swegner · 2018-05-02T18:22:53Z

Java precommit now fails on only a single test case, which is unrelated to this change.

swegner · 2018-05-02T18:23:00Z

Run Dataflow ValidatesRunner

swegner · 2018-05-02T21:26:20Z

@kennknowles please take a look.

Still waiting on Dataflow ValidatesRunner results, but if the current state is a clear improvement we should merge it since this test suite is currently executing at around 5 hours.

swegner · 2018-05-03T15:31:52Z

FYI, current Dataflow ValidatesRunner failure is due to the fact that current Gradle is set to max 2 workers. This is being addressed in #5218.

swegner · 2018-05-03T15:56:04Z

Run Flink ValidatesRunner

kennknowles · 2018-05-03T22:43:12Z

I think someone told me that the failure which you encountered is on master and fixed. Can you git rebase -i to resolve the fixups anyhow?

This is useful for local validation. It is not yet run via Jenkins.

This is an effort to reduce the overhead of our ValidatesRunner test suite. By manual analysis of the @ValidatesRunner test, I've identified a number of tests which don't seem to validate runner-specific functionality and are better suited as @NeedsRunner tests. As a result, these tests will only run on a single runner (DirectRunner), which executes efficiently in-memory.

echauchot

@swegner thanks for that !
I only quickly looked at the ValidatesRunner tests that I wrote (you modified none) and the ones that impact my ongoing work (metrics).

echauchot · 2018-05-04T09:25:26Z

sdks/java/core/src/test/java/org/apache/beam/sdk/metrics/MetricsTest.java

+    }
+
+    @Test
+    @Category({NeedsRunner.class, UsesAttemptedMetrics.class, UsesCounterMetrics.class})


I think this one is a ValidatesRunner because PipelineResults.metrics() actually uses MetricsContainerStepMap which is a runner-core componant and which is set by the different runners with their own aggregators to support metrics.

For MetricsTest, I tried to reduce what I felt was redundant validation of the runner-provided pieces. I kept a few @ValidatesRunner tests exercise different parts of the MetricsContainerStepMap; in particular:

testAllAttemptedMetrics()

testAllCommittedMetrics()

testAttempted[Counter|Distribution|Gauge]Metrics()

testCommitted[Counter|Distribution|Gauge]Metrics()

This test, testBoundedSourceMetrics() is validating that metrics from BoundedSources get added into the container. I don't believe this exercises any new runner-behavior not already covered by the other tests. Which is why I converted it.

Let me know if you disagree. If there is runner-behavior that needs to be validated we shouldn't be shy about keeping it as @ValidatesRunner. I think it's better to have long test runs than gaps in our validation.

A bit late for this comment but +1:
indeed the tests you pointed actually test the containers and you left them annotated with ValidatesRunner, so it is ok. You're right to say that the other tests that use PipelineResults.metrics() test other parts that the metrics aggregation with the runners.
Also I agree to keep validatesRunner tests as short as possible and remove any redundancy; only one validatesRunner test is enough to test the metrics aggregation.

echauchot · 2018-05-04T09:25:46Z

sdks/java/core/src/test/java/org/apache/beam/sdk/metrics/MetricsTest.java

+
+      MetricQueryResults metrics =
+          pipelineResult
+              .metrics()


same as above. Actually, I think this is true for all the tests using PepelineResults.metrics()

See comment above. If the tests are exercising different behaviors of PipelineResults.metrics() that are runner-provided functionality, we should keep them all as @ValidatesRunner. Otherwise, having at least one as @ValidatesRunner is sufficient and the rest can be @NeedsRunner

swegner · 2018-05-07T15:33:36Z

Ping @echauchot; take a look at my comments above and let me know what you think.

swegner · 2018-05-08T15:31:18Z

Ping @echauchot @kennknowles I believe all feedback has been addressed. Can you let me know if this is ready to merge?

kennknowles · 2018-05-08T17:47:45Z

Well, it seems to have sat for 4 days. So I think we could move forward and then could change those two test cases back if decided.

swegner force-pushed the optimize_validatesrunner branch from 4b2facc to 7d9cea8 Compare April 20, 2018 17:30

kennknowles requested changes Apr 20, 2018

View reviewed changes

swegner force-pushed the optimize_validatesrunner branch from 7d9cea8 to c91697e Compare April 20, 2018 19:01

kennknowles approved these changes Apr 24, 2018

View reviewed changes

swegner force-pushed the optimize_validatesrunner branch 2 times, most recently from c32d444 to dfbdc02 Compare April 24, 2018 22:13

swegner force-pushed the optimize_validatesrunner branch from dfbdc02 to 5242334 Compare May 1, 2018 22:52

swegner force-pushed the optimize_validatesrunner branch from 82b8456 to 2157a66 Compare May 3, 2018 15:31

swegner added 2 commits May 3, 2018 16:05

Create ValidatesRunner task for DirectRunner.

37ebee3

This is useful for local validation. It is not yet run via Jenkins.

swegner force-pushed the optimize_validatesrunner branch from 2157a66 to 4167138 Compare May 3, 2018 23:05

echauchot requested changes May 4, 2018

View reviewed changes

kennknowles merged commit cd92c5e into apache:master May 8, 2018

swegner deleted the optimize_validatesrunner branch May 23, 2018 23:16

[BEAM-4059] Reduce number of ValidatesRunner tests and reorganize them for better parallelization #5193

[BEAM-4059] Reduce number of ValidatesRunner tests and reorganize them for better parallelization #5193

Conversation

swegner commented Apr 20, 2018

swegner commented Apr 20, 2018

swegner commented Apr 20, 2018

swegner commented Apr 20, 2018

kennknowles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swegner commented Apr 20, 2018

kennknowles commented Apr 24, 2018

swegner commented Apr 24, 2018

swegner commented May 2, 2018

swegner commented May 2, 2018

swegner commented May 2, 2018

swegner commented May 2, 2018

swegner commented May 3, 2018

swegner commented May 3, 2018

kennknowles commented May 3, 2018

echauchot left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swegner commented May 7, 2018

swegner commented May 8, 2018

kennknowles commented May 8, 2018

echauchot left a comment •

edited