-
Notifications
You must be signed in to change notification settings - Fork 479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redesign MemoTable, CostEstimator, PlanEnumerator for Optimal Plan Selection #2147
Conversation
# Conflicts: # src/main/java/org/apache/sysds/hops/fedplanner/MemoTable.java
Thanks for the contribution @min-guk - as we just discussed please clean up the enumeration logic for arbitrary many inputs, and in a subsequent PR add a debugging print out of the memo table and its federated plans. |
Updates
Unchanged Features
Discussion Points
|
└──Hop 1 [LOUT] (Total: 2.000, Self: 1.000, Net: -0.000)
└─Hop 0 [FOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
└──Hop 12 [LOUT] (Total: 2.000, Self: 1.000, Net: -0.000)
└─Hop 11 [FOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
└──Hop 23 [LOUT] (Total: 2.000, Self: 1.000, Net: -0.000)
└─Hop 22 [FOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
└──Hop 42 [LOUT] (Total: 28.000, Self: 10.000, Net: -0.000)
└─Hop 41 [FOUT] (Total: 18.000, Self: 10.000, Net: -0.000)
├─Hop 33 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 34 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 35 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 36 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 37 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 38 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 39 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
└─Hop 40 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
└──Hop 65 [LOUT] (Total: 28.000, Self: 10.000, Net: -0.000)
└─Hop 64 [FOUT] (Total: 18.000, Self: 10.000, Net: -0.000)
├─Hop 56 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 57 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 58 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 59 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 60 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 61 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
├─Hop 62 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
└─Hop 63 [LOUT] (Total: 1.000, Self: 1.000, Net: -0.000)
Process finished with exit code 0 Here is the output of the DAG from the TestCode for your reference. |
UpdatesAs discussed in the meeting, I have updated the following components:
Updated Output of
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2147 +/- ##
============================================
+ Coverage 72.03% 72.05% +0.01%
- Complexity 43937 43961 +24
============================================
Files 1441 1443 +2
Lines 166106 166239 +133
Branches 32428 32453 +25
============================================
+ Hits 119655 119776 +121
- Misses 37199 37212 +13
+ Partials 9252 9251 -1 ☔ View full report in Codecov by Sentry. |
LGTM - Thanks for the patch @min-guk. During the merge I fixed the formatting (tabs over spaces in Java), added missing licenses, and fixed remaining javadoc issues. |
I have implemented FederatedPlanCostEstimator, FederatedPlanCostEnumerator, and FederatedMemoTable.
However, this implementation differs significantly from the coding direction we discussed in the meeting, so we need detailed discussion about the implementation direction in the next meeting.
Please review my thoughts and advise if my understanding is correct.
1. FederatedMemoTable (MemoTable)
The previous FedPlan class structure had several issues:
The key points of the redesigned FederatedMemoTable are as follows:
2. CostEstimator
The previous CostEstimator also had several issues:
The key points of the redesigned CostEstimator are as follows:
However, the current CostEstimator may cause two problems because it selects child plans based only on the cost of a single current plan and child plan:
3. FederatedPlanCostEnumerator