[SYSTEMDS-3221] Federated DataGENInstruction #1459
Conversation
OlgaOvcharenko
changed the title
There was a problem hiding this comment.
The design with a global list of federated addresses is not good,
i would extend the rand to include federated addresses,
Then a next task would be to make a rewrite instruction to rewrite data gen to a federated data gen based on subsequent operations.
With this design it will nicely fit into the framework.
| @@ -277,6 +280,7 @@ public static boolean executeScript( Configuration conf, String[] args ) | |||
| //reset runtime platform and visualize flag | |||
| setGlobalExecMode(oldrtplatform); | |||
| EXPLAIN = oldexplain; | |||
| DMLScript.FED_WORKER_PORTS.clear(); | |||
There was a problem hiding this comment.
I would like if this was not a variable in DMLScript, since it would not be available when executing in python.
maybe put it in:
src/main/java/org/apache/sysds/runtime/controlprogram/context/ExecutionContext.java
or
not have it.
Maybe the best design is to add another argument to rand, that allows you to specify a federated range of worker addresses.
| catch(UnknownHostException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| String host = addr.getHostName(); |
There was a problem hiding this comment.
With only here is another argument for why we need to specify a list of federated addresses, in the arguments for the instruction since we need to specify other hosts than localhost.
| int addedRows = size * (nworkers -1); | ||
| long[] rowsPerWorker = new long[nworkers]; | ||
| Arrays.fill(rowsPerWorker, 0, nworkers-1, size); | ||
| rowsPerWorker[nworkers-1] = getRows() - addedRows; |
There was a problem hiding this comment.
we should specify the rows as well in the instruction, using the federated addresses, since these give the dimensions of each federated site.
src/main/java/org/apache/sysds/runtime/instructions/fed/FEDInstructionUtils.java
Show resolved
Hide resolved
src/test/java/org/apache/sysds/test/functions/federated/datagen/FederatedRandTest.java
Show resolved
Hide resolved
This PR adds a simple matrix rand(..) case. Currently, federated rand(..) is called only for the corresponding test, later this should be modified.