You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@systemds.apache.org by GitBox <gi...@apache.org> on 2022/07/15 12:47:32 UTC

[GitHub] [systemds] kev-inn opened a new pull request, #1665: [WIP] Write matrices and frames at site for federated write

kev-inn opened a new pull request, #1665:
URL: https://github.com/apache/systemds/pull/1665

   Perform federated write at site of workers and locally write a MTD file containing the addresses. Frames work too, but testcases are still missing.


-- 
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: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] Baunsgaard commented on pull request #1665: [SYSTEMDS-3405] Write matrices and frames at site for federated write

Posted by GitBox <gi...@apache.org>.

Baunsgaard commented on PR #1665:
URL: https://github.com/apache/systemds/pull/1665#issuecomment-1225824243

   > There is a minor problem I don't know how to best fix. I need to select a path at the sites for the workers to write their partition, I am currently choosing to create a (most likely) unique filename and write into the LOCAL_TEMP_DIR (defined by the configuration). This usually works, but the federated python testcases have it set to `/tmp/systemds` which is apparently not available for our git runners. @Baunsgaard maybe we could change the configuration for the tests?
   > 
   > The problems mostly impact our testcases, but I also don't really have a favorite from a user side. Other choices for where to write and downsides:
   > 
   >     * current working directory (cwd): the cwd of our testcases is the root, therefore we would clutter it full of partitions
   > 
   >     * the scratch space: will be deleted when the worker is killed and in our case, because we just start the new workers from the same JVM, also when the testcase finishes. Therefore, we can't check the results.
   > 
   >     * A new configuration directory: pretty clean solution IMO, but would only be used by workers and I don't want to add it, except if we agree it is necessary
   > 
   > 
   > The other aspects are finished and this PR is ready for review.
   
   hmm, good question what the best option is.
   
   I find the last option with a new configuration tempting.
   It could be made such that the federated workers use a configured path to make a directory, furthermore we can use their process ID to make a unique sub directory, and maintain a static counter inside to guarantee incrementing folder/file names, while appending the user specified file name in the end. 


-- 
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: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] kev-inn commented on pull request #1665: [SYSTEMDS-3405] Write matrices and frames at site for federated write

Posted by GitBox <gi...@apache.org>.

kev-inn commented on PR #1665:
URL: https://github.com/apache/systemds/pull/1665#issuecomment-1200477730

There is a minor problem I don't know how to best fix. I need to select a path at the sites for the workers to write their partition, I am currently choosing to create a (most likely) unique filename and write into the LOCAL_TEMP_DIR (defined by the configuration). This usually works, but the federated python testcases have it set to `/tmp/systemds` which is apparently not available for our git runners. @Baunsgaard maybe we could change the configuration for the tests?

The problems mostly impact our testcases, but I also don't really have a favorite from a user side.
Other choices for where to write and downsides:
- current working directory (cwd): the cwd of our testcases is the root, therefore we would clutter it full of partitions
- the scratch space: will be deleted when the worker is killed and in our case, because we just start the new workers from the same JVM, also when the testcase finishes. Therefore, we can't check the results.
- A new configuration directory: pretty clean solution IMO, but would only be used by workers and I don't want to add it, except if we agree it is necessary

The other aspects are finished and this PR is ready for review.

--
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: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] Baunsgaard commented on pull request #1665: [WIP] Write matrices and frames at site for federated write

Posted by GitBox <gi...@apache.org>.

Baunsgaard commented on PR #1665:
URL: https://github.com/apache/systemds/pull/1665#issuecomment-1186563432

   yes please !


-- 
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: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] kev-inn commented on pull request #1665: [SYSTEMDS-3405] Write matrices and frames at site for federated write

Posted by GitBox <gi...@apache.org>.

kev-inn commented on PR #1665:
URL: https://github.com/apache/systemds/pull/1665#issuecomment-1200477752

The other aspects are finished and this PR is ready for review.

--
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: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] j143 commented on pull request #1665: [SYSTEMDS-3405] Write matrices and frames at site for federated write

Posted by "j143 (via GitHub)" <gi...@apache.org>.

j143 commented on PR #1665:
URL: https://github.com/apache/systemds/pull/1665#issuecomment-1407441681

   > I will need some time to refresh my memory and get it in a merge ready state next month.
   
   sure thing, @kev-inn - thanks


-- 
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: devnull-unsubscribe@infra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] j143 commented on pull request #1665: [SYSTEMDS-3405] Write matrices and frames at site for federated write

Posted by "j143 (via GitHub)" <gi...@apache.org>.

j143 commented on PR #1665:
URL: https://github.com/apache/systemds/pull/1665#issuecomment-1404524490

   Hi @kev-inn , this PR seems to be ready.
   
   I guess @Baunsgaard is in favor of your suggestion:
   
   > A new configuration directory: pretty clean solution IMO, but would only be used by workers and I don't want to add it, except if we agree it is necessary
   
   shall we wrap this up?
   
   Regards
   


-- 
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: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] kev-inn commented on a diff in pull request #1665: [SYSTEMDS-3405] Write matrices and frames at site for federated write

Posted by GitBox <gi...@apache.org>.

kev-inn commented on code in PR #1665:
URL: https://github.com/apache/systemds/pull/1665#discussion_r922877822


##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedData.java:
##########
@@ -105,6 +105,10 @@ public long getVarID() {
 		return _varID;
 	}
 
+	public void setFilepath(String filepath) {
+		_filepath = filepath;
+	}
+

Review Comment:
   We might want to keep `FederatedData` constant and create a copy with a different filepath instead. I am open for discussion.



##########
src/main/java/org/apache/sysds/runtime/instructions/fed/FEDInstructionUtils.java:
##########
@@ -232,12 +232,7 @@ else if((tinst.input1.isMatrix() && ec.getCacheableData(tinst.input1).isFederate
 			}
 			else if(inst instanceof VariableCPInstruction ){
 				VariableCPInstruction ins = (VariableCPInstruction) inst;
-				if(ins.getVariableOpcode() == VariableOperationCode.Write
-					&& ins.getInput1().isMatrix()
-					&& ins.getInput3().getName().contains("federated")){
-					fedinst = VariableFEDInstruction.parseInstruction(ins);
-				}
-				else if(ins.getVariableOpcode() == VariableOperationCode.CastAsFrameVariable
+				if(ins.getVariableOpcode() == VariableOperationCode.CastAsFrameVariable

Review Comment:
   The federated writing happens inside the `CacheableData<T>.exportData()` method, therefore we don't need to replace the `VariableCPInstruction`.



-- 
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: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] Baunsgaard commented on pull request #1665: [SYSTEMDS-3405] Write matrices and frames at site for federated write

Posted by GitBox <gi...@apache.org>.

Baunsgaard commented on PR #1665:
URL: https://github.com/apache/systemds/pull/1665#issuecomment-1225851390

   This PR also change the write(M) (if M is federated) to not collect the matrix and write it locally.
   Is it because it is always expected to be written to federated site if the output is federated?
   
   


-- 
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: dev-unsubscribe@systemds.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [systemds] kev-inn commented on pull request #1665: [SYSTEMDS-3405] Write matrices and frames at site for federated write

Posted by "kev-inn (via GitHub)" <gi...@apache.org>.

kev-inn commented on PR #1665:
URL: https://github.com/apache/systemds/pull/1665#issuecomment-1406668351

   I will need some time to refresh my memory and get it in a merge ready state next month.


-- 
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: devnull-unsubscribe@infra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org