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