You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemds.apache.org by GitBox <gi...@apache.org> on 2020/08/18 15:26:35 UTC
[GitHub] [systemds] kev-inn opened a new pull request #1027: [SYSTEMDS-2558][SYSTEMDS-2554][SYSTEMDS-2561] Fed frame recode transform (decode) support
kev-inn opened a new pull request #1027:
URL: https://github.com/apache/systemds/pull/1027
Adds decode support for recode, pass-through and composite (containing only those two).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] mboehm7 commented on pull request #1027: [SYSTEMDS-2558,2554,2561] Fed frame recode transform (decode) support
Posted by GitBox <gi...@apache.org>.
mboehm7 commented on pull request #1027:
URL: https://github.com/apache/systemds/pull/1027#issuecomment-677900762
LGTM - thanks for the patch @kev-inn. Overall this is nice, I just made some minor modifications: (1) fixed the export in `CacheableData` which did not handle federated objects (it might have worked due to previous transfer to the coordinator by chance), (2) fixed the removed replace handling in parsing of parameterized builtin instructions, (3) generalized the improved exception handling from UDFs to instruction execution and clear commands, and (4) fixed a javadoc issue I introduced in a previous commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on a change in pull request #1027: [SYSTEMDS-2558][SYSTEMDS-2554][SYSTEMDS-2561] Fed frame recode transform (decode) support
Posted by GitBox <gi...@apache.org>.
kev-inn commented on a change in pull request #1027:
URL: https://github.com/apache/systemds/pull/1027#discussion_r473958834
##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedWorkerHandler.java
##########
@@ -268,7 +268,8 @@ private FederatedResponse execUDF(FederatedRequest request) {
return udf.execute(ec, inputs);
}
catch(Exception ex) {
- return new FederatedResponse(ResponseType.ERROR, ex.getMessage());
+ return new FederatedResponse(ResponseType.ERROR, new FederatedWorkerHandlerException(
+ "Exception of type " + ex.getClass() + " thrown when processing request", ex));
Review comment:
`ex.getMessage()` would sometimes not suffice and instead be confusing. A `ArrayIndexOutOfBoundsException` would have for example just have the index as a message, therefore our response would just contain an arbitrary number and the problem would not be explained
##########
File path: src/main/java/org/apache/sysds/runtime/transform/encode/Encoder.java
##########
@@ -145,7 +145,7 @@ public Encoder subRangeEncoder(int colStart, int colEnd) {
*/
protected void mergeColumnInfo(Encoder other, int col) {
// update number of columns
- _clen = Math.max(_colList.length, col - 1 + other.getNumCols());
+ _clen = Math.max(_clen, col - 1 + other._clen);
Review comment:
This was my mistake in the last PR `_colList.length` is something different to `_clen` and `.getNumCols()` will be wrong once we add dummycoding (it gives us the cols after encoding).
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/fed/ParameterizedBuiltinFEDInstruction.java
##########
@@ -19,103 +19,217 @@
package org.apache.sysds.runtime.instructions.fed;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.LinkedHashMap;
+
+import org.apache.sysds.common.Types;
import org.apache.sysds.common.Types.DataType;
import org.apache.sysds.common.Types.ValueType;
+import org.apache.sysds.hops.OptimizerUtils;
import org.apache.sysds.lops.Lop;
import org.apache.sysds.runtime.DMLRuntimeException;
+import org.apache.sysds.runtime.controlprogram.caching.FrameObject;
import org.apache.sysds.runtime.controlprogram.caching.MatrixObject;
import org.apache.sysds.runtime.controlprogram.context.ExecutionContext;
import org.apache.sysds.runtime.controlprogram.federated.FederatedRequest;
-import org.apache.sysds.runtime.functionobjects.ParameterizedBuiltin;
-import org.apache.sysds.runtime.functionobjects.ValueFunction;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedResponse;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedUDF;
+import org.apache.sysds.runtime.controlprogram.federated.FederationMap;
import org.apache.sysds.runtime.controlprogram.federated.FederationUtils;
+import org.apache.sysds.runtime.functionobjects.ValueFunction;
import org.apache.sysds.runtime.instructions.InstructionUtils;
import org.apache.sysds.runtime.instructions.cp.CPOperand;
+import org.apache.sysds.runtime.instructions.cp.Data;
+import org.apache.sysds.runtime.matrix.data.FrameBlock;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
import org.apache.sysds.runtime.matrix.operators.Operator;
-import org.apache.sysds.runtime.matrix.operators.SimpleOperator;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.meta.MetaDataFormat;
+import org.apache.sysds.runtime.privacy.PrivacyMonitor;
+import org.apache.sysds.runtime.transform.decode.Decoder;
+import org.apache.sysds.runtime.transform.decode.DecoderFactory;
public class ParameterizedBuiltinFEDInstruction extends ComputationFEDInstruction {
-
protected final LinkedHashMap<String, String> params;
-
- protected ParameterizedBuiltinFEDInstruction(Operator op,
- LinkedHashMap<String, String> paramsMap, CPOperand out, String opcode, String istr)
- {
+
+ protected ParameterizedBuiltinFEDInstruction(Operator op, LinkedHashMap<String, String> paramsMap, CPOperand out,
+ String opcode, String istr) {
super(FEDType.ParameterizedBuiltin, op, null, null, out, opcode, istr);
params = paramsMap;
}
-
- public HashMap<String,String> getParameterMap() {
- return params;
+
+ public HashMap<String, String> getParameterMap() {
+ return params;
}
-
+
public String getParam(String key) {
return getParameterMap().get(key);
}
-
+
public static LinkedHashMap<String, String> constructParameterMap(String[] params) {
// process all elements in "params" except first(opcode) and last(output)
- LinkedHashMap<String,String> paramMap = new LinkedHashMap<>();
-
+ LinkedHashMap<String, String> paramMap = new LinkedHashMap<>();
+
// all parameters are of form <name=value>
String[] parts;
- for ( int i=1; i <= params.length-2; i++ ) {
+ for(int i = 1; i <= params.length - 2; i++) {
parts = params[i].split(Lop.NAME_VALUE_SEPARATOR);
paramMap.put(parts[0], parts[1]);
}
-
+
return paramMap;
}
-
- public static ParameterizedBuiltinFEDInstruction parseInstruction ( String str ) {
+
+ public static ParameterizedBuiltinFEDInstruction parseInstruction(String str) {
String[] parts = InstructionUtils.getInstructionPartsWithValueType(str);
// first part is always the opcode
String opcode = parts[0];
// last part is always the output
- CPOperand out = new CPOperand( parts[parts.length-1] );
-
+ CPOperand out = new CPOperand(parts[parts.length - 1]);
+
// process remaining parts and build a hash map
- LinkedHashMap<String,String> paramsMap = constructParameterMap(parts);
-
+ LinkedHashMap<String, String> paramsMap = constructParameterMap(parts);
+
// determine the appropriate value function
ValueFunction func = null;
- if( opcode.equalsIgnoreCase("replace") ) {
- func = ParameterizedBuiltin.getParameterizedBuiltinFnObject(opcode);
- return new ParameterizedBuiltinFEDInstruction(new SimpleOperator(func), paramsMap, out, opcode, str);
Review comment:
I formatted this file (sorry reviewer), but I think since those files are quite new we should start by abiding to our code style.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] mboehm7 commented on a change in pull request #1027: [SYSTEMDS-2558,2554,2561] Fed frame recode transform (decode) support
Posted by GitBox <gi...@apache.org>.
mboehm7 commented on a change in pull request #1027:
URL: https://github.com/apache/systemds/pull/1027#discussion_r474238261
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/fed/ParameterizedBuiltinFEDInstruction.java
##########
@@ -19,103 +19,217 @@
package org.apache.sysds.runtime.instructions.fed;
+import java.util.Arrays;
import java.util.HashMap;
import java.util.LinkedHashMap;
+
+import org.apache.sysds.common.Types;
import org.apache.sysds.common.Types.DataType;
import org.apache.sysds.common.Types.ValueType;
+import org.apache.sysds.hops.OptimizerUtils;
import org.apache.sysds.lops.Lop;
import org.apache.sysds.runtime.DMLRuntimeException;
+import org.apache.sysds.runtime.controlprogram.caching.FrameObject;
import org.apache.sysds.runtime.controlprogram.caching.MatrixObject;
import org.apache.sysds.runtime.controlprogram.context.ExecutionContext;
import org.apache.sysds.runtime.controlprogram.federated.FederatedRequest;
-import org.apache.sysds.runtime.functionobjects.ParameterizedBuiltin;
-import org.apache.sysds.runtime.functionobjects.ValueFunction;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedResponse;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedUDF;
+import org.apache.sysds.runtime.controlprogram.federated.FederationMap;
import org.apache.sysds.runtime.controlprogram.federated.FederationUtils;
+import org.apache.sysds.runtime.functionobjects.ValueFunction;
import org.apache.sysds.runtime.instructions.InstructionUtils;
import org.apache.sysds.runtime.instructions.cp.CPOperand;
+import org.apache.sysds.runtime.instructions.cp.Data;
+import org.apache.sysds.runtime.matrix.data.FrameBlock;
+import org.apache.sysds.runtime.matrix.data.MatrixBlock;
import org.apache.sysds.runtime.matrix.operators.Operator;
-import org.apache.sysds.runtime.matrix.operators.SimpleOperator;
+import org.apache.sysds.runtime.meta.MatrixCharacteristics;
+import org.apache.sysds.runtime.meta.MetaDataFormat;
+import org.apache.sysds.runtime.privacy.PrivacyMonitor;
+import org.apache.sysds.runtime.transform.decode.Decoder;
+import org.apache.sysds.runtime.transform.decode.DecoderFactory;
public class ParameterizedBuiltinFEDInstruction extends ComputationFEDInstruction {
-
protected final LinkedHashMap<String, String> params;
-
- protected ParameterizedBuiltinFEDInstruction(Operator op,
- LinkedHashMap<String, String> paramsMap, CPOperand out, String opcode, String istr)
- {
+
+ protected ParameterizedBuiltinFEDInstruction(Operator op, LinkedHashMap<String, String> paramsMap, CPOperand out,
+ String opcode, String istr) {
super(FEDType.ParameterizedBuiltin, op, null, null, out, opcode, istr);
params = paramsMap;
}
-
- public HashMap<String,String> getParameterMap() {
- return params;
+
+ public HashMap<String, String> getParameterMap() {
+ return params;
}
-
+
public String getParam(String key) {
return getParameterMap().get(key);
}
-
+
public static LinkedHashMap<String, String> constructParameterMap(String[] params) {
// process all elements in "params" except first(opcode) and last(output)
- LinkedHashMap<String,String> paramMap = new LinkedHashMap<>();
-
+ LinkedHashMap<String, String> paramMap = new LinkedHashMap<>();
+
// all parameters are of form <name=value>
String[] parts;
- for ( int i=1; i <= params.length-2; i++ ) {
+ for(int i = 1; i <= params.length - 2; i++) {
parts = params[i].split(Lop.NAME_VALUE_SEPARATOR);
paramMap.put(parts[0], parts[1]);
}
-
+
return paramMap;
}
-
- public static ParameterizedBuiltinFEDInstruction parseInstruction ( String str ) {
+
+ public static ParameterizedBuiltinFEDInstruction parseInstruction(String str) {
String[] parts = InstructionUtils.getInstructionPartsWithValueType(str);
// first part is always the opcode
String opcode = parts[0];
// last part is always the output
- CPOperand out = new CPOperand( parts[parts.length-1] );
-
+ CPOperand out = new CPOperand(parts[parts.length - 1]);
+
// process remaining parts and build a hash map
- LinkedHashMap<String,String> paramsMap = constructParameterMap(parts);
-
+ LinkedHashMap<String, String> paramsMap = constructParameterMap(parts);
+
// determine the appropriate value function
ValueFunction func = null;
- if( opcode.equalsIgnoreCase("replace") ) {
- func = ParameterizedBuiltin.getParameterizedBuiltinFnObject(opcode);
- return new ParameterizedBuiltinFEDInstruction(new SimpleOperator(func), paramsMap, out, opcode, str);
Review comment:
don't worry about the formatting - more severe was the above deletion of the recently added replace. ;-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] asfgit closed pull request #1027: [SYSTEMDS-2558,2554,2561] Fed frame recode transform (decode) support
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1027:
URL: https://github.com/apache/systemds/pull/1027
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org