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