[NEMO-328] Refactor IRDAG #190

johnyangk · 2019-01-23T01:05:20Z

JIRA: NEMO-328: Refactor IRDAG

Major changes:

Modifies the Pass interface to use IRDAG, rather than directly using DAG
Modifies reshaping passes (e.g., LargeShuffle, Skew) to use the IRDAG reshaping methods

Minor changes to note:

Merges multiple annotating LargeShufflePasses into a single LargeShuffleAnnotatingPass

Tests for the changes:

Existing tests pass

Other comments:

N/A

Closes #190

sanha · 2019-01-24T04:45:09Z

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

- * 1) Task-level statistic collection is done via vertex with {@link MetricCollectTransform}
- * 2) Stage-level statistic aggregation is done via vertex with {@link AggregateMetricTransform}
- * inserted before shuffle edges.
+ * We insert a {@link MessageBarrierVertex for each shuffle edge}


We must optimize multiple shuffle edges at once if a vertex receives many shuffle edges. Please check 327.

wonook

I've taken a brief look throughout the PR. I think it doesn't make sense using both IRDAG and DAG<IRVertex, IREdge> now, so I think it should all be changed to IRDAG. Also, for the reshaping pass and annotating pass, it looks like apply and optimize methods actually do the same thing but have different names. I don't see a reason for this as well, feels like it would confuse our users. Could you justify this or make appropriate changes throughout your PR? I'll take a second look once these changes have been made.
Thanks 😄

wonook · 2019-01-24T04:56:42Z

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

+ * @param <V> of the input pair.
+ * @param <O> of the output aggregated message.
+ */
+public class MessageAggregationVertex<K, V, O> extends OperatorVertex {


MessageAggregatorVertex?

wonook · 2019-01-24T04:58:58Z

...ain/java/org/apache/nemo/compiler/optimizer/pass/compiletime/annotating/CompressionPass.java

@@ -48,11 +46,10 @@ public CompressionPass(final CompressionProperty.Value compression) {
  }

  @Override
-  public DAG<IRVertex, IREdge> apply(final DAG<IRVertex, IREdge> dag) {
+  public IRDAG optimize(final IRDAG dag) {


does changing the method name still override the super method?

wonook · 2019-01-24T05:01:30Z

...org/apache/nemo/compiler/optimizer/pass/compiletime/composite/LargeShuffleCompositePass.java

-        new LargeShuffleDataPersistencePass(),
-        new LargeShuffleResourceSlotPass()
+        new LargeShuffleReshapingPass(),
+        new LargeShuffleAnnotatingPass()


Should other composite passes refactored this way as well?

Sounds good. I filed a JIRA.
https://issues.apache.org/jira/browse/NEMO-329

wonook · 2019-01-24T05:07:39Z

common/src/main/java/org/apache/nemo/common/ir/IRDAG.java

+   * Get the current underlying DAG for direct access.
+   * @return underlying DAG.
+   */
+  public DAG<IRVertex, IREdge> getCurrentDAGSnapshot() {


How is this a 'snapshot'?

wonook · 2019-01-24T05:15:36Z

common/src/main/java/org/apache/nemo/common/ir/IRDAG.java

+   * "Unsafe" direct reshaping (semantic-preserving is not guaranteed).
+   * @param unsafeReshaping a function that directly reshapes the underlying DAG.
+   */
+  public void unSafeDirectReshaping(final Function<DAG<IRVertex, IREdge>, DAG<IRVertex, IREdge>> unsafeReshaping) {


Maybe something like 'reshapeUnsafely' or 'applyUnsafeDirectReshaping' or something like that? Unsafe is a single word by the way. I think it would be better to keep the names of method that perform a specific action as verbs. Could you double check on this throughout your PR?

DAG<IRVertex, IREdge> seems to be still used here. I don't see a reason for using both forms.

johnyangk · 2019-01-24T08:47:01Z

Thanks for the comments. I've updated the code.

@sanha
I've modified IRDAG#insert to receive multiple IREdges so the #189 continues to function as intended. I've run NetworkTraceAnalysisITCase#testDataSkew and verified the DAG topology.

@wonook
I've addressed your comments except for IRDAG#unSafeDirectReshaping, because the loop-related passes require direct access to the underlying DAG. It'd be good to handle this issue with a separate PR (JIRA). There also remains around 15 instances of DAG<IRVertex, IREdge> outside of IRDAG, mostly in loop-related passes and 2 in Spark-related classes for building new IRDAGs from RDD applications. Other than those instances, I've changed the type to IRDAG for consistency.

wonook

Looks good. Just a few naming suggestions. I'll merge after the change.

wonook · 2019-01-31T05:56:27Z

common/src/main/java/org/apache/nemo/common/dag/DAGQueryInterface.java

+ * @param <V> the vertex type
+ * @param <E> the edge type
+ */
+public interface DAGQueryInterface<V extends Vertex, E extends Edge<V>> extends Serializable {


maybe simply just a DAGInterface as its name?

wonook · 2019-01-31T05:59:23Z

common/src/main/java/org/apache/nemo/common/ir/IRDAG.java

+  private final AtomicInteger metricCollectionId;
+
+  private DAG<IRVertex, IREdge> lastSnapshotDAG; // the DAG that was saved most recently.
+  private DAG<IRVertex, IREdge> updatingDAG; // the DAG that is being updated.


dagSnapshot andmodifiedDAG?

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

@sanha Anything else you want to check?

outdated

johnyangk · 2019-01-31T07:04:13Z

@wonook Thanks. I've renamed as per your suggestions.

sanha

@johnyangk Thanks for the work! Please check my comment.

sanha · 2019-01-31T07:28:57Z

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

-            final IREdge edgeToOriginalDstV =
-              new IREdge(edge.getPropertyValue(CommunicationPatternProperty.class).get(), edge.getSrc(), v);
-            edge.copyExecutionPropertiesTo(edgeToOriginalDstV);
-            edgeToOriginalDstV.setPropertyPermanently(MetricCollectionProperty.of(metricCollectionId));


Don't we need to annotate this MetricCollectionProperty elsewhere?

@sanha This has been moved to IRDAG#insert

https://github.com/apache/incubator-nemo/pull/190/files#diff-969693370fcffe6720d48d175502c466R210

sanha

@johnyangk Thanks for your explanation! I'll merge this.

John Yang added 26 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

johnyangk self-assigned this Jan 23, 2019

johnyangk requested review from jeongyooneo, wonook and seojangho January 23, 2019 01:05

johnyangk requested a review from sanha January 23, 2019 01:05

minor change

2e03c47

sanha previously requested changes Jan 24, 2019

View reviewed changes

wonook suggested changes Jan 24, 2019

View reviewed changes

John Yang added 7 commits January 24, 2019 15:14

sanha's comment

296bd71

nit

8b9837d

revert optimize() to apply()

82102df

use irdag consistently

a4cabaa

checkstyle

95b7328

clean up

544e423

fix boolean error

276d899

John Yang and others added 3 commits January 24, 2019 17:52

fix CompilerTestUtil to use IRDAG

73108db

checkstyle

3cab078

fix unit test

32f3f49

johnyangk mentioned this pull request Jan 30, 2019

[NEMO-332] Refactor RunTimePass #191

Merged

wonook suggested changes Jan 31, 2019

View reviewed changes

address comment

11b0d36

wonook approved these changes Jan 31, 2019

View reviewed changes

sanha suggested changes Jan 31, 2019

View reviewed changes

sanha approved these changes Feb 1, 2019

View reviewed changes

sanha merged commit a49b050 into apache:master Feb 1, 2019

johnyangk deleted the 328-refactor-irdag branch February 1, 2019 02:17

[NEMO-328] Refactor IRDAG #190

[NEMO-328] Refactor IRDAG #190

Uh oh!

Conversation

johnyangk commented Jan 23, 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!

wonook left a comment • 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!

johnyangk commented Jan 24, 2019

Uh oh!

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

wonook commented Jan 31, 2019

Uh oh!

johnyangk commented Jan 31, 2019

Uh oh!

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

sanha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnyangk commented Jan 23, 2019 •

edited

Loading

wonook left a comment •

edited

Loading