[NEMO-332] Refactor RunTimePass #191

johnyangk · 2019-01-30T01:58:13Z

JIRA: NEMO-332: Refactor RunTimePass

Major changes:

Refactors the RunTimePass API
Refactors the run-time plan rewriting logic into the NemoPlanRewriter

Minor changes to note:

Replaces ResourceSkewedDataProperty with ResourceAntiAffinityProperty (generalizing the directive)

Tests for the changes:

Existing tests continue to pass

Closes #191

johnyangk · 2019-01-30T01:59:23Z

Please review this PR after #190 is merged, as this PR builds on top of #190. Thanks!

jeongyooneo

Thanks! I've left some comments.

jeongyooneo · 2019-01-30T06:05:57Z

common/src/main/java/org/apache/nemo/common/ir/edge/executionproperty/PartitionSetProperty.java

 /**
- * DataSkewMetric ExecutionProperty.
+ * Number of partitions.


Could you elaborate on where this 'number of partitions' info used?
Maybe mentioning some use cases(ex. skew handling) would be helpful.

jeongyooneo · 2019-01-30T06:08:26Z

common/src/main/java/org/apache/nemo/common/ir/edge/executionproperty/PartitionerProperty.java

-public final class PartitionerProperty extends EdgeExecutionProperty<PartitionerProperty.Value> {
+public final class PartitionerProperty
+  extends EdgeExecutionProperty<Pair<PartitionerProperty.Type, Integer>> {
+  public static final int AUTO_NUMBER_OF_PARTITIONS = 0;


What does 'auto' mean here? How about DESTINATION_PARALLELISM?

jeongyooneo · 2019-01-30T06:08:36Z

common/src/main/java/org/apache/nemo/common/ir/edge/executionproperty/PartitionerProperty.java

-   *
-   * @param value value of the new execution property.
-   * @return the newly created execution property.
+   * Use the automatic number of partitions.


jeongyooneo · 2019-01-30T06:11:31Z

...src/main/java/org/apache/nemo/common/ir/vertex/executionproperty/MinParallelismProperty.java

@@ -23,13 +23,13 @@
 /**
 * Parallelism ExecutionProperty.
 */
-public final class ParallelismProperty extends VertexExecutionProperty<Integer> {
+public final class MinParallelismProperty extends VertexExecutionProperty<Integer> {


What differs 'MinParallelism' and 'Parallelism'?

I've added class-level comments.

Do you think that this property would be meaningful even when it doesn't show the accurate values of the parallelism of each vertex? Why not update the value of the ParallelismProperty instead when the cases described above occur?

jeongyooneo · 2019-01-30T06:27:24Z

common/src/main/java/org/apache/nemo/common/ir/vertex/system/MessageAggregatorVertex.java

@@ -16,28 +16,26 @@
 * specific language governing permissions and limitations
 * under the License.
 */
-package org.apache.nemo.common;
+package org.apache.nemo.common.ir.vertex.system;


How about having basic, optimization(or specialized?) 2-package structure?

optimization package for MessageAggregatorVertex and MessageBarrierVertex, as they're used in DAG optimization

basic package for StreamVertex, OperatorVertex, ... etc other than IRVertex, since they all extend IRVertex

I've explicitly added the SystemIRVertex abstract class to make it clearer that Stream/MessageBarrier/MessageAggregate vertices are provided by Nemo and allowed to be inserted into an IRDAG to optimize execution. (hence the "system" vertices)

jeongyooneo · 2019-01-30T06:39:18Z

common/src/main/java/org/apache/nemo/common/ir/vertex/transform/StreamTransform.java

@@ -28,14 +28,14 @@
 * This transform can be used for merging input data into the {@link OutputCollector}.
 * @param <T> input/output type.
 */
-public final class RelayTransform<T> implements Transform<T, T> {
+public final class StreamTransform<T> implements Transform<T, T> {


StreamTransform intuitively sounds like transform used in stream operation(other than batch).
How about ShuffleDataStreamTransform or something like that to keep the nuance of 'relaying promptly'?

I think this vertex can be decoupled from Shuffle, as it can be inserted regardless of the communication pattern, but I can't think of a better name at the moment. I do think that Stream is better than Relay in capturing the behavior of "immediately forwarding the input element without modifying it".

How about we revisit this later, after we add more system vertices and streaming functionalities?

jeongyooneo · 2019-01-30T06:41:46Z

common/src/main/java/org/apache/nemo/common/partitioner/DedicatedKeyPerElementPartitioner.java

@@ -33,7 +33,7 @@
   * Constructor.
   */
  public DedicatedKeyPerElementPartitioner() {
-    // TODO #288: DedicatedKeyPerElementPartitioner should not always return 0
+    // TODO #288: DedicatedKeyPerElement should not always return 0


DedicatedKeyPerElement type Partitioner should not always return 0?

jeongyooneo · 2019-01-30T06:46:12Z

common/src/main/java/org/apache/nemo/common/partitioner/Partitioner.java

        partitioner = new DedicatedKeyPerElementPartitioner();
        break;
+      case Hash:
+        final int numOfPartitions = edgeProperties.get(PartitionerProperty.class).get().right();
+        // If AUTO, use the number of destination parallelism to minimize the number of partitions


Maybe copy this comment to the definition of AUTO_NUMBER_OF_PARTITIONS?

wonook · 2019-01-31T06:05:58Z

i'll take a look after #190

johnyangk · 2019-02-01T01:25:12Z

Thanks @jeongyooneo, I've addressed the comments.

common/src/main/java/org/apache/nemo/common/ir/vertex/system/SystemIRVertex.java

...src/main/java/org/apache/nemo/common/ir/vertex/executionproperty/MinParallelismProperty.java

...in/java/org/apache/nemo/compiler/optimizer/pass/compiletime/reshaping/SkewReshapingPass.java

johnyangk · 2019-02-01T04:49:12Z

Thanks @wonook, I've addressed the comments.

johnyangk · 2019-02-06T23:57:51Z

@jeongyooneo @wonook Ping. 😄

wonook · 2019-02-07T05:00:10Z

LGTM on my side. @jeongyooneo anything else you want to take a look?

jeongyooneo · 2019-02-07T07:04:59Z

Thanks! I'll merge this.

John Yang added 30 commits January 21, 2019 16:14

init

d245546

use the irdag class

2a8e8d8

chkpt

d22fc1d

optimize imports

fea8405

largeshuffle done

1257851

chkpt

ba034cc

handle loops

4319fb1

skew restructuring

7a80134

chkpt

c393d58

done

55a5e6f

chkpt

0c08d8b

refactor largeshuffle

f894b97

save

1ea942d

message done

4690d80

aggr done

35bf7e9

src code compiles

17d8202

return an irdag

290be9f

tests almost

182b887

all compiles

9cba450

largeshuffle debugging done

37b51d3

skew debugged

409fc67

relay to stream

cd38af6

unit tests pass

a5f22f0

fix tests

4ee3346

merge

d409b9b

checkstyle

66cc885

minor change

2e03c47

runtime

8524111

chkpt

17d4e04

remove

38961fe

John Yang added 3 commits January 30, 2019 09:32

unit tests

b16ddbc

checkstyle

0b61bb9

tests pass

679685c

johnyangk self-assigned this Jan 30, 2019

johnyangk changed the title ~~Refactor RunTimePass~~ [NEMO-332] Refactor RunTimePass Jan 30, 2019

itcase

acf375f

jeongyooneo requested changes Jan 30, 2019

View reviewed changes

John Yang added 3 commits January 31, 2019 16:03

address comment

11b0d36

merge

5681ba4

address comments

b365c9a

John Yang added 2 commits February 1, 2019 10:26

nit

6ccd9c9

merge

5409e42

wonook reviewed Feb 1, 2019

View reviewed changes

common/src/main/java/org/apache/nemo/common/ir/vertex/system/SystemIRVertex.java Outdated Show resolved Hide resolved

wonook reviewed Feb 1, 2019

View reviewed changes

...src/main/java/org/apache/nemo/common/ir/vertex/executionproperty/MinParallelismProperty.java Outdated Show resolved Hide resolved

wonook reviewed Feb 1, 2019

View reviewed changes

...in/java/org/apache/nemo/compiler/optimizer/pass/compiletime/reshaping/SkewReshapingPass.java Outdated Show resolved Hide resolved

John Yang added 2 commits February 1, 2019 13:19

remove min from parallelism

0352023

address comments

02db05a

John Yang added 4 commits February 1, 2019 14:38

chkpt

9de6d91

fix stageEdge bug

a6f678f

provided to utility

ba35ee2

nit

bf806a3

jeongyooneo approved these changes Feb 7, 2019

View reviewed changes

jeongyooneo merged commit 8ec2469 into apache:master Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEMO-332] Refactor RunTimePass #191

[NEMO-332] Refactor RunTimePass #191

johnyangk commented Jan 30, 2019 •

edited

Loading

johnyangk commented Jan 30, 2019

jeongyooneo left a comment

jeongyooneo Jan 30, 2019

jeongyooneo Jan 30, 2019

jeongyooneo Jan 30, 2019

jeongyooneo Jan 30, 2019

johnyangk Feb 1, 2019

wonook Feb 1, 2019

jeongyooneo Jan 30, 2019

johnyangk Feb 1, 2019

jeongyooneo Jan 30, 2019

johnyangk Feb 1, 2019

jeongyooneo Jan 30, 2019

jeongyooneo Jan 30, 2019

wonook commented Jan 31, 2019

johnyangk commented Feb 1, 2019

johnyangk commented Feb 1, 2019

johnyangk commented Feb 6, 2019

wonook commented Feb 7, 2019

jeongyooneo commented Feb 7, 2019

[NEMO-332] Refactor RunTimePass #191

[NEMO-332] Refactor RunTimePass #191

Conversation

johnyangk commented Jan 30, 2019 • edited Loading

johnyangk commented Jan 30, 2019

jeongyooneo 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

wonook commented Jan 31, 2019

johnyangk commented Feb 1, 2019

johnyangk commented Feb 1, 2019

johnyangk commented Feb 6, 2019

wonook commented Feb 7, 2019

jeongyooneo commented Feb 7, 2019

johnyangk commented Jan 30, 2019 •

edited

Loading