[GitHub] [systemds] sebwrede commented on pull request #1113: [WIP][SYSTEMDS-2550] Shuffle data partitioner

[GitBox <gi...@apache.org>]

sebwrede commented on pull request #1113:
URL: https://github.com/apache/systemds/pull/1113#issuecomment-732130997


   **Generally, I think this PR looks good.** 
   In AutomatedTestBase:640, you create a new PrivacyConstraint. This means that it writes the privacy level to the metadata file. Is this necessary for your tests? You could just pass null to the method call and then it would not write any privacy constraints to the metadata file. 
   
   **Comments on you questions/issues:**
   
   > The hybrid federated parameter server unit tests fail stating "Shuffle" scheme is not supported. This is because the read somehow pulls in the matrix and presents it as a dense block. This means the parameter server is running in regular mode thus failing on the shuffle scheme.
   
   Which "read" are you talking about here? Is it the one in ShuffleFederatedScheme or is it the one that originally reads the input? 
   
   > The federated parameter server unit test input directory is currently not cleaned up. Should this be done after every run?
   
   I think that tests in other packages do not clean this up. That is at least what I have observed, so I don't think you have to worry about it. 
   
   > How to generate the ID of a newly created FederationMap correctly?
   
   I think that you can make a call to: 
   `FederationUtils.getNextFedDataID()`


----------------------------------------------------------------
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