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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
