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 2022/06/07 14:03:10 UTC
[GitHub] [systemds] kev-inn opened a new pull request, #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
kev-inn opened a new pull request, #1628:
URL: https://github.com/apache/systemds/pull/1628
This reworks our runtime replacement of CP instructions with FED versions when appropriate.
SP replacement will be done in another PR.
This implementation was my second approach, the first one consisted of static methods in the FED instructions. I much prefer this one as it is quite clear which instructions are replaced, which constraints the CP imposes and we don't have to lead every check with an `instanceof` check. The downside of this approach is that it can get a bit confusing with inheritance, but it is---and will probably stay---manageable.
Note that not everything is replaced yet, I will work on it while the general approach can already be reviewed.
During the testing I found myself requiring to check if actual FED instructions are executed, is there a way to ensure this in the test framework, or is this already ensured in some way via privacy? Tests checking the heavy hitters do it implicitly.
NOTE: I deleted a fix in `QuantilePickCPInstruction` (bf19244c909ae8354bc8b3da0985eaf448aa5f0c), as the common layout is no longer necessary and null is cleaner. This is intentional and the fix is no longer necessary.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1149797403
> You have a point there, I also realized this recently when trying to also replace SP Instructions without duplicating code. I will weigh the options, if I come up with more, but will most likely opt to replace it already during parsing.
>
> Edit: this actually is not that simple, as we create our instructions before we get the runtime information if an object is federated AFAIK. Note that federated instruction compilation exists by now, but this is about a runtime decision. I will search for a smooth approach, currently only interfaces come to mind, but would like to find something better.
You are right, it is not that simple but try to prioritize a good extensible design and then we can expand upon it.
If you cannot find a better way, then i still can live with the design proposed already.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1148791380
As a side note, when I ran all the tests the following testcases fail, but work when executed in isolation:
- `FederatedWorkerIntegrationCRUDTest.testWorkerAddedForMonitoring`
- `FederatedColAggregateTest.testTernaryColSumDenseMatrix`
- `FederatedCtableTest.federatedCtableSinglenode` (configuration [3])
- `FederatedFullAggregateTest.testTernarySumDenseMatrixCP` (configuration [0] and [1])
- `FederatedWeightedCrossEntropy` (configuration [0] and [1])
- `federatedWeightedCrossEntropySpark`
- `federatedWeightedCrossEntropySparkEpsilon`
- `federatedWeightedCrossEntropySparkNode`
- `federatedWeightedCrossEntropySparkNodeEpsilon`
- `FederatedWeightedDivMatrixMultTest` all or almost all
- `FederatedWeightedSigmoidTest` all
- `FederatedWeightedSquaredLossTest` all
- `FederatedWeightedUnaryMatrixMultTest` all (both configurations)
I remember that this is a known problem.
`FederatedReuseSlicesTest.testElementWisePlusCP` fails, because `checkForReuses()` expects `"LinCache writes (Mem/FS/Del): 8/"`, but the actual value is `"LinCache writes (Mem/FS/Del): 12/"`. I am not sure what the reason could be here.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1150238963
After a lot of consideration I propose the following:
1. Switch to the first implementation approach. The refactor makes the code more maintainable, because of less indentation and implementation in the corresponding `FEDInstruction`. The static methods perform checks for both `CPInstruction` and `SPInstruction`, which will necessarily have large overlaps. A constructor with the `SPInstruction` as input (like the ones I already added with `CPInstruction`) will be added where necessary.
2. Add a method to most `SPInstruction`s to convert it to a `CPInstruction`. This will be used to allow the federated worker to choose which execution type he wants to use. Most `SPInstruction` can be converted to `CPInstruction` (and vice versa) by changing the execution type in the instruction string before parsing, those we could simply tag with an interface. This would reduce code quite a bit, but is harder to maintain. Opinions are appreciated.
3. Some `SPInstruction`s like `ReblockSPInstruction` don't have an equivalent `CPInstruction`, those will be sent to the worker as is. A problem arises when the federated worker opts to use the `CPInstruction` for inputs instead, this would require us to ignore the `SPInstruction`. Not sure if checking for RDD handles on the inputs is enough(?).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1150993877
> Thanks for getting started on this issue @kev-inn. Unfortunately, I think the PR goes a bit in the wrong direction. We first want to do a basic cleanup of the federated instructions, without modifying the CP or Spark instructions. The replace in `FEDInstructionUtils` should stay mostly as it is, and all federated instructions should have overloaded `parseInstruction` methods to consume either CP or Spark instructions (objects) directly and construct the FED instruction like [1] without replicating the string parsing logic. For the parseInstruction with strings, we can call the parse on the respective CP instruction (for consistency) and use again the above methods.
>
> In a second step, we would remove the replicated instructions like `MMFEDInstruction` (redundant to `AggregateBinaryFEDInstruction`) and let all instruction send CP instruction strings to the federated workers.
>
> In a third step, we then extend the federated workers to (under certain conditions) create a hop that represents the CP instruction and generate lops and instructions as needed (e.g., CP, Spark, GPU).
>
> Sorry, if I was not clear during our offline discussions.
>
> [1] https://github.com/apache/systemds/blob/main/src/main/java/org/apache/sysds/runtime/instructions/fed/CentralMomentFEDInstruction.java#L77
No worries, as @Baunsgaard stated, I think the majority of this PR is still a nice addition and it fits your first step nicely. I would like to have 3 methods:
1. Interface method named `toFedInstruction` for CP and SP Instructions
2. Static "constructor" method for `FEDInstructions` `tryReplace()`
3. FED constructors taking `CPInstruction` and `SPInstruction`. I propose a constructor instead of `parseInstruction()`, because it always returns an instance (the checks happen in `tryReplace()`) and `parseInstruction()` can be kept for actual parsing logic.
The second step I might combine with this PR, but the third one will definitely be seperate.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1219543262
Closing to begin the merging phase, Thanks for the contribution @kev-inn
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1149666292
You have a point there, I also realized this recently when trying to also replace SP Instructions without duplicating code. I will weigh the options, if I come up with more, but will most likely opt to replace it already during parsing.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1149653353
> This implementation was my second approach, the first one consisted of static methods in the FED instructions. I much prefer this one as it is quite clear which instructions are replaced, which constraints the CP imposes and we don't have to lead every check with an instanceof check. The downside of this approach is that it can get a bit confusing with inheritance, but it is---and will probably stay---manageable.
I am not sure if this is the best location to edit.
Would it not make more sense to change it when we construct the CP vs SP vs GPU instructions from the string versions of instructions themselves?
This way if we for instance in the future add FPGA instructions it naturally also support changing these into federated instructions, and we do not have to maintain both all instances for each federated instruction which would lead to excessive testing requirements where we need to hit each instruction as either cases.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1149805822
Sure, that is the number one priority of this rework, that is why I also would like to remove the dependency for the string representations of those instructions to be similar/matching.
In the future we want to send only CP instructions to the workers and locally choose CP, SP or GPU (maybe even FED?). Keeping this in mind, we could add conversions for all kinds of instructions to CP (and require constructors like the ones I added for FED) or add interfaces with getters for specialized instructions like `BinaryAggregationInstruction`.
The approach, which should be the nicest to work with, would be a complete rework of the hierarchy of instructions, adding the type of execution only at the leaf specializations. Instead of `ComputationCPInstruction` and `ComputationSPInstruction` we would have `ComputationInstruction. This would require a very intrusive change of a lot of core implementations and seems infeasable.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on a diff in pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on code in PR #1628:
URL: https://github.com/apache/systemds/pull/1628#discussion_r891282601
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/CovarianceCPInstruction.java:
##########
@@ -82,4 +85,28 @@ public void processInstruction(ExecutionContext ec)
double val = covobj.getRequiredResult(_optr);
ec.setScalarOutput(output_name, new DoubleObject(val));
}
+
+ @Override
+ protected FEDInstruction tryReplaceWithFederated(ExecutionContext ec) {
+ /* NOTE: the requirement before was:
+ if( (instruction.input1.isMatrix() && ec.getMatrixObject(instruction.input1).isFederatedExcept(FTypes.FType.BROADCAST))
+ || (instruction.input2.isMatrix() && ec.getMatrixObject(instruction.input2).isFederatedExcept(FTypes.FType.BROADCAST))) {
+ if(instruction.getOpcode().equals(...) )
+ ...
+ else if("cov".equals(instruction.getOpcode()) && (ec.getMatrixObject(instruction.input1).isFederated(
+ FTypes.FType.ROW) ||
+ ec.getMatrixObject(instruction.input2).isFederated(FTypes.FType.ROW)))
+ ...
+ else
+ ...
+
+ This allowed `input1(Broadcast & Row) & input2(!Broadcast & ...)`, I am not sure if that is intentional.
+ */
+ if((input1.isMatrix() && ec.getMatrixObject(input1).isFederated(FTypes.FType.ROW) &&
+ ec.getMatrixObject(input1).isFederatedExcept(FTypes.FType.BROADCAST)) ||
+ (input2.isMatrix() && ec.getMatrixObject(input2).isFederated(FTypes.FType.ROW) &&
+ ec.getMatrixObject(input2).isFederatedExcept(FTypes.FType.BROADCAST)))
Review Comment:
I am not sure about this, no testcase covers this I think.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1155170877
`ConcurrentModificationException` seems to be more frequent with this PR, I do not know what the reason is for that, but that is anyway a fix for the future.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard closed pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
Baunsgaard closed pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
URL: https://github.com/apache/systemds/pull/1628
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on a diff in pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on code in PR #1628:
URL: https://github.com/apache/systemds/pull/1628#discussion_r891289159
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/QuantileSortCPInstruction.java:
##########
@@ -119,4 +123,23 @@ public void processInstruction(ExecutionContext ec) {
//set and release output
ec.setMatrixOutput(output.getName(), resultBlock);
}
+
+ public int getNumThreads() {
+ return _numThreads;
+ }
+
+ @Override
+ protected FEDInstruction tryReplaceWithFederated(ExecutionContext ec) {
+ if(!input1.isMatrix())
+ return null;
+
+ MatrixObject mo1 = ec.getMatrixObject(input1);
+ if(!mo1.isFederatedExcept(FType.BROADCAST))
+ return null;
+
+ if(mo1.isFederated(FType.ROW) ||
+ (mo1.getFedMapping().getFederatedRanges().length == 1 && mo1.isFederated(FType.COL)))
+ return new QuantileSortFEDInstruction(this);
+ return null;
Review Comment:
As you can notice in all implementations I opted to use early return checks to reduce nesting. The last if on the other hand checks if replacement is appropriate. This is inconsistent to the checks before, which checked for failure, but is much more readable/maintainable, than confusing inverted conditions in my experience.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on a diff in pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on code in PR #1628:
URL: https://github.com/apache/systemds/pull/1628#discussion_r891282601
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/CovarianceCPInstruction.java:
##########
@@ -82,4 +85,28 @@ public void processInstruction(ExecutionContext ec)
double val = covobj.getRequiredResult(_optr);
ec.setScalarOutput(output_name, new DoubleObject(val));
}
+
+ @Override
+ protected FEDInstruction tryReplaceWithFederated(ExecutionContext ec) {
+ /* NOTE: the requirement before was:
+ if( (instruction.input1.isMatrix() && ec.getMatrixObject(instruction.input1).isFederatedExcept(FTypes.FType.BROADCAST))
+ || (instruction.input2.isMatrix() && ec.getMatrixObject(instruction.input2).isFederatedExcept(FTypes.FType.BROADCAST))) {
+ if(instruction.getOpcode().equals(...) )
+ ...
+ else if("cov".equals(instruction.getOpcode()) && (ec.getMatrixObject(instruction.input1).isFederated(
+ FTypes.FType.ROW) ||
+ ec.getMatrixObject(instruction.input2).isFederated(FTypes.FType.ROW)))
+ ...
+ else
+ ...
+
+ This allowed `input1(Broadcast & Row) & input2(!Broadcast & ...)`, I am not sure if that is intentional.
+ */
+ if((input1.isMatrix() && ec.getMatrixObject(input1).isFederated(FTypes.FType.ROW) &&
+ ec.getMatrixObject(input1).isFederatedExcept(FTypes.FType.BROADCAST)) ||
+ (input2.isMatrix() && ec.getMatrixObject(input2).isFederated(FTypes.FType.ROW) &&
+ ec.getMatrixObject(input2).isFederatedExcept(FTypes.FType.BROADCAST)))
Review Comment:
I am not sure about this.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1150415515
(Just because i regret deleting it -- i wrote this while you commented Matthias)
I suggest that you make the methods inside CP and SP instructions part of a interface that allows any instruction to change to FED, and then make the footprint of these functions as small as possible inside each of the CP and SP instructions. All of these instructions should call into the fitting FEDInstructions class that then handle the logic of converting the instructions to FEDInstructions though static constructors that can return null.
With this we simply try to call this interface function on the instructions to change them into FEDInstructions when they can before execution. In the static construction it is easy to change the SP instructions (or GPU etc) to CP if possible, otherwise the static constructor would construct SP type.
This design addresses all 3 points with (i think) the least amount of code.
-- Comment to Matthias's point
We are missing one step where we can avoid the whole FEDInstructionUtils code with casting that Kevin i think correctly is addressing in some of in his changes, where we can remove the entire if else tree if we add a interface method that simply map to the static constructors in your step 1.
If we set default for all methods to return null for this interface "toFedInstruction" then we can just overwrite all instances of instructions that can change into FEDInstructions and get much cleaner code from it, --- with the downside of having small overloaded functions that map to the static constructors for FEDInstructions in each CP and SP instruction---
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1150407311
> After a lot of consideration I propose the following:
>
> 1. Switch...
> 2. Add a method to most `SPInstruction`s ...
> 3. Some `SPInstruction`s like `ReblockSPInstruction` ...
I suggest that you make the methods inside CP and SP instructions part of a interface that allows any instruction to change to FED, and then make the footprint of these functions as small as possible inside each of the CP and SP instructions. All of these instructions should call into the fitting FEDInstructions class that then handle the logic of converting the instructions to FEDInstructions though static constructors that can return null.
With this we simply try to call this interface function on the instructions to change them into FEDInstructions when they can before execution. In the static construction it is easy to change the SP instructions (or GPU etc) to CP if possible, otherwise the static constructor would construct SP type.
This design addresses all 3 points with (i think) the least amount of code.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] mboehm7 commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
mboehm7 commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1150390257
Thanks for getting started on this issue @kev-inn. Unfortunately, I think the PR goes a bit in the wrong direction. We first want to do a basic cleanup of the federated instructions, without modifying the CP or Spark instructions. The replace in `FEDInstructionUtils` should stay mostly as it is, and all federated instructions should have overloaded `parseInstruction` methods to consume either CP or Spark instructions (objects) directly and construct the FED instruction like [1] without replicating the string parsing logic. For the parseInstruction with strings, we can call the parse on the respective CP instruction (for consistency) and use again the above methods.
In a second step, we would remove the replicated instructions like `MMFEDInstruction` (redundant to `AggregateBinaryFEDInstruction`) and let all instruction send CP instruction strings to the federated workers.
In a third step, we then extend the federated workers to (under certain conditions) create a hop that represents the CP instruction and generate lops and instructions as needed (e.g., CP, Spark, GPU).
Sorry, if I was not clear during our offline discussions.
[1] https://github.com/apache/systemds/blob/main/src/main/java/org/apache/sysds/runtime/instructions/fed/CentralMomentFEDInstruction.java#L77
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on a diff in pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on code in PR #1628:
URL: https://github.com/apache/systemds/pull/1628#discussion_r891284083
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/QuantilePickCPInstruction.java:
##########
@@ -65,7 +68,7 @@ else if( parts.length == 5 ) {
CPOperand out = new CPOperand(parts[2]);
OperationTypes ptype = OperationTypes.valueOf(parts[3]);
boolean inmem = Boolean.parseBoolean(parts[4]);
- return new QuantilePickCPInstruction(null, in1, new CPOperand(), out, ptype, inmem, opcode, str);
+ return new QuantilePickCPInstruction(null, in1, out, ptype, inmem, opcode, str);
Review Comment:
Our new approach allows us to remove this fix/hack.
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/CovarianceCPInstruction.java:
##########
@@ -82,4 +85,28 @@ public void processInstruction(ExecutionContext ec)
double val = covobj.getRequiredResult(_optr);
ec.setScalarOutput(output_name, new DoubleObject(val));
}
+
+ @Override
+ protected FEDInstruction tryReplaceWithFederated(ExecutionContext ec) {
+ /* NOTE: the requirement before was:
+ if( (instruction.input1.isMatrix() && ec.getMatrixObject(instruction.input1).isFederatedExcept(FTypes.FType.BROADCAST))
+ || (instruction.input2.isMatrix() && ec.getMatrixObject(instruction.input2).isFederatedExcept(FTypes.FType.BROADCAST))) {
+ if(instruction.getOpcode().equals(...) )
+ ...
+ else if("cov".equals(instruction.getOpcode()) && (ec.getMatrixObject(instruction.input1).isFederated(
+ FTypes.FType.ROW) ||
+ ec.getMatrixObject(instruction.input2).isFederated(FTypes.FType.ROW)))
+ ...
+ else
+ ...
+
+ This allowed `input1(Broadcast & Row) & input2(!Broadcast & ...)`, I am not sure if that is intentional.
+ */
+ if((input1.isMatrix() && ec.getMatrixObject(input1).isFederated(FTypes.FType.ROW) &&
+ ec.getMatrixObject(input1).isFederatedExcept(FTypes.FType.BROADCAST)) ||
+ (input2.isMatrix() && ec.getMatrixObject(input2).isFederated(FTypes.FType.ROW) &&
+ ec.getMatrixObject(input2).isFederatedExcept(FTypes.FType.BROADCAST)))
Review Comment:
I am not sure about this, no testcase covers the case explained at the bottom of the comment I think.
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/UnaryScalarCPInstruction.java:
##########
@@ -26,10 +26,10 @@
import org.apache.sysds.runtime.matrix.operators.Operator;
import org.apache.sysds.runtime.matrix.operators.UnaryOperator;
-public class UnaryScalarCPInstruction extends UnaryMatrixCPInstruction {
+public class UnaryScalarCPInstruction extends UnaryCPInstruction {
Review Comment:
This is a problem due to inheritance. We replace `UnaryMatrixCPInstruction`s, but don't replace `UnaryScalarCPInstruction`s, with FED instructions. I think this inheritance made no sense and probably was a bug.
If it *wasn't* we can also overload the `tryReplaceWithFederated()` here.
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/QuantileSortCPInstruction.java:
##########
@@ -119,4 +123,23 @@ public void processInstruction(ExecutionContext ec) {
//set and release output
ec.setMatrixOutput(output.getName(), resultBlock);
}
+
+ public int getNumThreads() {
+ return _numThreads;
+ }
+
+ @Override
+ protected FEDInstruction tryReplaceWithFederated(ExecutionContext ec) {
+ if(!input1.isMatrix())
+ return null;
+
+ MatrixObject mo1 = ec.getMatrixObject(input1);
+ if(!mo1.isFederatedExcept(FType.BROADCAST))
+ return null;
+
+ if(mo1.isFederated(FType.ROW) ||
+ (mo1.getFedMapping().getFederatedRanges().length == 1 && mo1.isFederated(FType.COL)))
+ return new QuantileSortFEDInstruction(this);
+ return null;
Review Comment:
As you can notice in all implementations I opted to use early return checks to reduce nesting. The last if on the other hand checks if replacement is appropriate. This is inconsistent to the checks before, which checked for failure, but is much more readable and therefore maintainable than confusing inverted conditions in my experience.
##########
src/main/java/org/apache/sysds/runtime/instructions/fed/AggregateBinaryFEDInstruction.java:
##########
@@ -53,6 +56,12 @@ public AggregateBinaryFEDInstruction(Operator op, CPOperand in1, CPOperand in2,
super(FEDType.AggregateBinary, op, in1, in2, out, opcode, istr, fedOut);
}
+ public AggregateBinaryFEDInstruction(AggregateBinaryCPInstruction instruction) {
+ // TODO @KEVIN: should we update the instruction string?
+ this(instruction.getOperator(), instruction.input1, instruction.input2, instruction.output,
+ instruction.getOpcode(), instruction.getInstructionString(), FederatedOutput.NONE);
Review Comment:
I always keep the instruction string without changes, but we might want to replace the leading `CP` with `FED` and add the type of `FederatedOutput` (always `NONE`, as it is decided at runtime).
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/QuantilePickCPInstruction.java:
##########
@@ -127,4 +130,22 @@ public void processInstruction(ExecutionContext ec) {
throw new DMLRuntimeException("Unsupported qpick operation type: "+_type);
}
}
+
+ public OperationTypes getOperationType() {
+ return _type;
+ }
+
+ public boolean isInMem() {
+ return _inmem;
+ }
+
+ @Override
+ protected FEDInstruction tryReplaceWithFederated(ExecutionContext ec) {
+ // Should we federate if the quantiles are given as a federated matrix? (input2)
+ // This might be useful for privacy and for performance just in extreme corner cases.
+ // The current implementation does not support it at the moment.
Review Comment:
This comment could be removed as we anyway only replace instructions where equivalent FED implementations exist, but it is worth discussing/remembering what to do about this case.
##########
src/main/java/org/apache/sysds/runtime/instructions/fed/TsmmFEDInstruction.java:
##########
@@ -50,6 +52,11 @@ public TsmmFEDInstruction(CPOperand in, CPOperand out, MMTSJType type, int k, St
this(in, out, type, k, opcode, istr, FederatedOutput.NONE);
}
+ public TsmmFEDInstruction(MMTSJCPInstruction instruction) {
+ this(instruction.getInputs()[0], instruction.getOutput(), instruction.getMMTSJType(), -1 /*ignore numThreads*/,
Review Comment:
Again, `numThreads` without any impact I think.
##########
src/main/java/org/apache/sysds/runtime/instructions/fed/AggregateBinaryFEDInstruction.java:
##########
@@ -69,6 +78,22 @@ public static AggregateBinaryFEDInstruction parseInstruction(String str) {
InstructionUtils.getMatMultOperator(k), in1, in2, out, opcode, str, fedOut);
}
+ public static AggregateBinaryFEDInstruction tryReplace(Instruction inst, ExecutionContext ec) {
+ if (!(inst instanceof AggregateBinaryCPInstruction)) {
+ return null;
+ }
+ AggregateBinaryCPInstruction instruction = (AggregateBinaryCPInstruction) inst;
+ if(!instruction.input1.isMatrix() || !instruction.input2.isMatrix()) {
+ return null;
+ }
+ MatrixObject mo1 = ec.getMatrixObject(instruction.input1);
+ MatrixObject mo2 = ec.getMatrixObject(instruction.input2);
+ if (!(mo1.isFederated(FType.ROW) || mo2.isFederated(FType.ROW) || mo1.isFederated(FType.COL))) {
+ return null;
+ }
+ return new AggregateBinaryFEDInstruction(instruction);
+ }
+
Review Comment:
This is outdated from my first attempt, will be removed.
##########
src/main/java/org/apache/sysds/runtime/instructions/fed/MMChainFEDInstruction.java:
##########
@@ -43,6 +44,12 @@ public MMChainFEDInstruction(CPOperand in1, CPOperand in2, CPOperand in3,
_type = type;
}
+ public MMChainFEDInstruction(MMChainCPInstruction instruction) {
+ this(instruction.input1, instruction.input2, instruction.input3, instruction.output,
+ instruction.getMMChainType(), -1 /* ignore numThreads */, instruction.getOpcode(),
+ instruction.getInstructionString());
Review Comment:
I will update this to use the same `numThreads`, but I don't think this has any effect for the federated 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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1149648533
> As a side note, when I ran all the tests the following testcases fail, but work when executed in isolation:
>
> * `FederatedWorkerIntegrationCRUDTest.testWorkerAddedForMonitoring`
>
> * `FederatedColAggregateTest.testTernaryColSumDenseMatrix`
>
> * `FederatedCtableTest.federatedCtableSinglenode` (configuration [3])
>
> * `FederatedFullAggregateTest.testTernarySumDenseMatrixCP` (configuration [0] and [1])
>
> * `FederatedWeightedCrossEntropy` (configuration [0] and [1])
>
> * `federatedWeightedCrossEntropySpark`
> * `federatedWeightedCrossEntropySparkEpsilon`
> * `federatedWeightedCrossEntropySparkNode`
> * `federatedWeightedCrossEntropySparkNodeEpsilon`
>
> * `FederatedWeightedDivMatrixMultTest` all or almost all
>
> * `FederatedWeightedSigmoidTest` all
>
> * `FederatedWeightedSquaredLossTest` all
>
> * `FederatedWeightedUnaryMatrixMultTest` all (both configurations)
>
>
> I remember that this is a known problem. Edit: I was behind a few commits, so this information might be outdated.
Yes this is known.
You can run your tests such that they repeat if failing, there you will detect these "flaky" tests.
If you use IntelliJ, you can also add a new execution mode where you repeat failing tests
mvn clean compile test -Dmaven.test.skip=false -Drerun.failingtests.count=1 -D automatedtestbase.outputbuffering=true -Dtest=org.apache.sysds.test.$@
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1153891131
I am sorry that this PR got so large. I believe it is better to do it in one go, but I can also break it down so the review is faster.
I recommend to open the old `FEDInstructionUtils.java` file on one screen and on the other screen go through all added `toFedInstruction()` methods. This will allows to easily match the old conditions with the new, more compact and (hopefully) easier to read conditions.
There aren't many changes to the logic, except that I changed (or added, see `AppendSPInstruction`) some super-classes.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] kev-inn commented on pull request #1628: [SYSTEMDS-3386] Refactor replacement of CP instructions
Posted by GitBox <gi...@apache.org>.
kev-inn commented on PR #1628:
URL: https://github.com/apache/systemds/pull/1628#issuecomment-1153937269
Not ready for review, found some problems with the spark instructions.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org