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/09/22 15:13:36 UTC
[GitHub] [systemds] kev-inn opened a new pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
kev-inn opened a new pull request #1062:
URL: https://github.com/apache/systemds/pull/1062
Adds `FederatedLocalData` so that we can use local data without the necessity to send it to a worker. This allows reusing a lot of code, but might lead to overhead. Other options to handle this scenario exist, please let me know what you think about this specific implementation.
----------------------------------------------------------------
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] Baunsgaard closed pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
Baunsgaard closed pull request #1062:
URL: https://github.com/apache/systemds/pull/1062
----------------------------------------------------------------
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] Baunsgaard commented on a change in pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on a change in pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#discussion_r492843396
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/InstructionUtils.java
##########
@@ -140,6 +141,19 @@ public static int checkNumFields( String[] parts, int expected1, int expected2 )
return numFields;
}
+ public static int checkNumFields( String[] parts, int... expected ) {
+ int numParts = parts.length;
+ int numFields = numParts - 1; //account for opcode
+
+ if (Arrays.stream(expected).noneMatch((i) -> numFields == i))
+ throw new DMLRuntimeException(
+ "checkNumFields() -- expected number ("
+ + Arrays.stream(expected).mapToObj(Integer::toString).reduce((a, b) -> a + ", " + b).orElse("")
+ + ") != is not equal to actual number (" + numFields + ").");
Review comment:
this looks overkill, maybe use a string builder
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/fed/AppendFEDInstruction.java
##########
@@ -33,71 +32,74 @@
import org.apache.sysds.runtime.meta.DataCharacteristics;
public class AppendFEDInstruction extends BinaryFEDInstruction {
- protected boolean _cbind; //otherwise rbind
-
- protected AppendFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out,
- boolean cbind, String opcode, String istr) {
+ protected boolean _cbind; // otherwise rbind
+
+ protected AppendFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out, boolean cbind,
+ String opcode, String istr) {
super(FEDType.Append, op, in1, in2, out, opcode, istr);
_cbind = cbind;
}
-
+
public static AppendFEDInstruction parseInstruction(String str) {
String[] parts = InstructionUtils.getInstructionPartsWithValueType(str);
- InstructionUtils.checkNumFields(parts, 5, 4);
-
+ InstructionUtils.checkNumFields(parts, 6, 5, 4);
+
String opcode = parts[0];
CPOperand in1 = new CPOperand(parts[1]);
CPOperand in2 = new CPOperand(parts[2]);
CPOperand out = new CPOperand(parts[parts.length - 2]);
boolean cbind = Boolean.parseBoolean(parts[parts.length - 1]);
-
+
Operator op = new ReorgOperator(OffsetColumnIndex.getOffsetColumnIndexFnObject(-1));
return new AppendFEDInstruction(op, in1, in2, out, cbind, opcode, str);
}
-
+
@Override
public void processInstruction(ExecutionContext ec) {
- //get inputs
+ // get inputs
MatrixObject mo1 = ec.getMatrixObject(input1.getName());
MatrixObject mo2 = ec.getMatrixObject(input2.getName());
DataCharacteristics dc1 = mo1.getDataCharacteristics();
DataCharacteristics dc2 = mo1.getDataCharacteristics();
-
- //check input dimensions
- if (_cbind && mo1.getNumRows() != mo2.getNumRows()) {
- throw new DMLRuntimeException(
- "Append-cbind is not possible for federated input matrices " + input1.getName() + " and "
- + input2.getName() + " with different number of rows: " + mo1.getNumRows() + " vs "
- + mo2.getNumRows());
- }
- else if (!_cbind && mo1.getNumColumns() != mo2.getNumColumns()) {
- throw new DMLRuntimeException(
- "Append-rbind is not possible for federated input matrices " + input1.getName() + " and "
- + input2.getName() + " with different number of columns: " + mo1.getNumColumns()
- + " vs " + mo2.getNumColumns());
+
+ // check input dimensions
+ if(_cbind && mo1.getNumRows() != mo2.getNumRows()) {
+ throw new DMLRuntimeException("Append-cbind is not possible for federated input matrices "
+ + input1.getName() + " and " + input2.getName() + " with different number of rows: " + mo1.getNumRows()
+ + " vs " + mo2.getNumRows());
Review comment:
again, maybe string builder.
----------------------------------------------------------------
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] Baunsgaard commented on pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#issuecomment-700673332
used closing instead of closes... in the merge...
but this is merged to master therefore i'm closing it.
----------------------------------------------------------------
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 edited a comment on pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
kev-inn edited a comment on pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#issuecomment-698890649
@sebwrede could you take a look at the new r/cbind tests? I believe you used it to check transfer privacy constraints, but the fixed rbind and cbind should not transfer (except when writing result to disk), so maybe a simple write test would suffice.
Edit: `FederatedWorkerHandlerTest` fails due to changes
----------------------------------------------------------------
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 pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
kev-inn commented on pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#issuecomment-696839679
@Baunsgaard I improved the `FederatedRCBindTest.java`, which actually didn't properly execute the federated instructions, that contains a test case for federated-federated, federated-local and local-federated bindings (both rbind and cbind). I will add a comment that clarifies that.
----------------------------------------------------------------
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 pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
kev-inn commented on pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#issuecomment-698890649
@sebwrede could you take a look at the new r/cbind tests? I believe you used it to check transfer privacy constraints, but the fixed rbind and cbind should not transfer (except when writing result to disk), so maybe a simple write test would suffice.
----------------------------------------------------------------
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] sebwrede commented on pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
sebwrede commented on pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#issuecomment-698947363
> @sebwrede could you take a look at the new r/cbind tests? I believe you used it to check transfer privacy constraints, but the fixed rbind and cbind should not transfer (except when writing result to disk), so maybe a simple write test would suffice.
> Edit: `FederatedWorkerHandlerTest` fails due to changes
Yes, the tests are failing right now because the arguments given are not correct anymore. As you say, they do not even transfer anything anymore, so they are not really relevant to put in a FederatedWorkerHandler test. The test was more relevant when we had the different instructions handled separately on the worker side. Furthermore, I will soon make a PR that changes the way privacy constraints are propagated for RBind and CBind.
With this in mind, I think we could actually remove or at least ignore the privacy tests that are using CBind and RBind in FederatedWorkerHandlerTest.java.
----------------------------------------------------------------
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 edited a comment on pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
kev-inn edited a comment on pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#issuecomment-698890649
@sebwrede could you take a look at the new r/cbind tests? I believe you used it to check transfer privacy constraints, but the fixed rbind and cbind should not transfer (except when writing result to disk), so maybe a simple write test would suffice.
Edit: `FederatedWorkerHandlerTest` fails due to changes
----------------------------------------------------------------
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 pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
kev-inn commented on pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#issuecomment-698890649
@sebwrede could you take a look at the new r/cbind tests? I believe you used it to check transfer privacy constraints, but the fixed rbind and cbind should not transfer (except when writing result to disk), so maybe a simple write test would suffice.
----------------------------------------------------------------
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] Baunsgaard commented on a change in pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on a change in pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#discussion_r492843396
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/InstructionUtils.java
##########
@@ -140,6 +141,19 @@ public static int checkNumFields( String[] parts, int expected1, int expected2 )
return numFields;
}
+ public static int checkNumFields( String[] parts, int... expected ) {
+ int numParts = parts.length;
+ int numFields = numParts - 1; //account for opcode
+
+ if (Arrays.stream(expected).noneMatch((i) -> numFields == i))
+ throw new DMLRuntimeException(
+ "checkNumFields() -- expected number ("
+ + Arrays.stream(expected).mapToObj(Integer::toString).reduce((a, b) -> a + ", " + b).orElse("")
+ + ") != is not equal to actual number (" + numFields + ").");
Review comment:
this looks overkill, maybe use a string builder
##########
File path: src/main/java/org/apache/sysds/runtime/instructions/fed/AppendFEDInstruction.java
##########
@@ -33,71 +32,74 @@
import org.apache.sysds.runtime.meta.DataCharacteristics;
public class AppendFEDInstruction extends BinaryFEDInstruction {
- protected boolean _cbind; //otherwise rbind
-
- protected AppendFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out,
- boolean cbind, String opcode, String istr) {
+ protected boolean _cbind; // otherwise rbind
+
+ protected AppendFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out, boolean cbind,
+ String opcode, String istr) {
super(FEDType.Append, op, in1, in2, out, opcode, istr);
_cbind = cbind;
}
-
+
public static AppendFEDInstruction parseInstruction(String str) {
String[] parts = InstructionUtils.getInstructionPartsWithValueType(str);
- InstructionUtils.checkNumFields(parts, 5, 4);
-
+ InstructionUtils.checkNumFields(parts, 6, 5, 4);
+
String opcode = parts[0];
CPOperand in1 = new CPOperand(parts[1]);
CPOperand in2 = new CPOperand(parts[2]);
CPOperand out = new CPOperand(parts[parts.length - 2]);
boolean cbind = Boolean.parseBoolean(parts[parts.length - 1]);
-
+
Operator op = new ReorgOperator(OffsetColumnIndex.getOffsetColumnIndexFnObject(-1));
return new AppendFEDInstruction(op, in1, in2, out, cbind, opcode, str);
}
-
+
@Override
public void processInstruction(ExecutionContext ec) {
- //get inputs
+ // get inputs
MatrixObject mo1 = ec.getMatrixObject(input1.getName());
MatrixObject mo2 = ec.getMatrixObject(input2.getName());
DataCharacteristics dc1 = mo1.getDataCharacteristics();
DataCharacteristics dc2 = mo1.getDataCharacteristics();
-
- //check input dimensions
- if (_cbind && mo1.getNumRows() != mo2.getNumRows()) {
- throw new DMLRuntimeException(
- "Append-cbind is not possible for federated input matrices " + input1.getName() + " and "
- + input2.getName() + " with different number of rows: " + mo1.getNumRows() + " vs "
- + mo2.getNumRows());
- }
- else if (!_cbind && mo1.getNumColumns() != mo2.getNumColumns()) {
- throw new DMLRuntimeException(
- "Append-rbind is not possible for federated input matrices " + input1.getName() + " and "
- + input2.getName() + " with different number of columns: " + mo1.getNumColumns()
- + " vs " + mo2.getNumColumns());
+
+ // check input dimensions
+ if(_cbind && mo1.getNumRows() != mo2.getNumRows()) {
+ throw new DMLRuntimeException("Append-cbind is not possible for federated input matrices "
+ + input1.getName() + " and " + input2.getName() + " with different number of rows: " + mo1.getNumRows()
+ + " vs " + mo2.getNumRows());
Review comment:
again, maybe string builder.
----------------------------------------------------------------
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 pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data
Posted by GitBox <gi...@apache.org>.
kev-inn commented on pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#issuecomment-696839679
@Baunsgaard I improved the `FederatedRCBindTest.java`, which actually didn't properly execute the federated instructions, that contains a test case for federated-federated, federated-local and local-federated bindings (both rbind and cbind). I will add a comment that clarifies that.
----------------------------------------------------------------
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