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 2021/09/09 13:36:36 UTC
[GitHub] [systemds] ywcb00 opened a new pull request #1390: [SYSTEMDS-3124] Federated Binary Aggregation - Federated Output
ywcb00 opened a new pull request #1390:
URL: https://github.com/apache/systemds/pull/1390
Hi,
This PR contains a small fix for the MV case to keep the output federated. The problem was that the branch to create a federated output was only taken if it was forced federated. I have now changed it so that the partial output is only set if it is forced federated, and the federated output is set if it is not forced local.
The other changes inside AggregateBinaryFEDInstruction should not change the functional behaviour of the instruction. I only tried to cleanup some code redundancies that could be merged.
This PR also includes an implicit change to the FederatedTokenizeTest, where I removed the creation of the fourth thread because it is never used.
Thanks 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] ywcb00 commented on pull request #1390: [WIP][SYSTEMDS-3124] Federated Binary Aggregation and (Map)MM - Federated Output
Posted by GitBox <gi...@apache.org>.
ywcb00 commented on pull request #1390:
URL: https://github.com/apache/systemds/pull/1390#issuecomment-963060951
Since I now know that we should not create a federated output in the MV case (unless it is forced federated) for reasons of performance, I'm taking over the refactoring of the two instructions (AggregateBinaryFEDInstruction and MMFEDInstruction) to SYSTEMDS-3206 and continue there with the fix of the endless loop mentioned above.
--
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] ywcb00 commented on pull request #1390: [SYSTEMDS-3124] Federated Binary Aggregation and (Map)MM - Federated Output
Posted by GitBox <gi...@apache.org>.
ywcb00 commented on pull request #1390:
URL: https://github.com/apache/systemds/pull/1390#issuecomment-933578229
When I resolved the conflicts with the master branch, I saw that Spark tests were added to the FederatedRowAggregate tests.
Since Spark uses (Map)MM instead of the AggregateBinary instruction, I had to make the very same changes to the (Map)MM instruction as I did to the AggregateBinary instruction.
--
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] sebwrede commented on pull request #1390: [SYSTEMDS-3124] Federated Binary Aggregation and (Map)MM - Federated Output
Posted by GitBox <gi...@apache.org>.
sebwrede commented on pull request #1390:
URL: https://github.com/apache/systemds/pull/1390#issuecomment-937626683
> When I resolved the conflicts with the master branch, I saw that Spark tests were added to the FederatedRowAggregate tests. Since Spark uses (Map)MM instead of the AggregateBinary instruction, I had to make the very same changes to the (Map)MM instruction as I did to the AggregateBinary instruction.
There is now a StackOverflowError in FederatedMSVMTest.federatedMSVM1CP. I think it is because of MatrixObject.java:438, where an `acquireReadAndRelease()` call is made when the matrix object is federated. This does not work when it has to read from HDFS. I am not entirely sure how your changes causes this, but we need to change it so that it does not call `acquireReadAndRelease()` whenever the data is federated. It should instead make some check to see if it needs to call `DataConverter.readMatrixFromHDFS` even if the object itself 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] ywcb00 commented on pull request #1390: [SYSTEMDS-3124] Federated Binary Aggregation and (Map)MM - Federated Output
Posted by GitBox <gi...@apache.org>.
ywcb00 commented on pull request #1390:
URL: https://github.com/apache/systemds/pull/1390#issuecomment-938587552
Thanks for pointing this out @sebwrede.
I found an endless loop formed by _MatrixObject:438_ and _CacheableData:551_, which occurs because we exclude the _FType.BROADCAST_ type from the federation check in _CacheableData_ and include it for the federation check in the _MatrixObject_.
That's why the _acquireReadIntern()_ and _readBlobFromHDFS()_ methods always call each other until it results in a StackOverflow.
I'm trying to find a solution for it :eyes:
--
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] ywcb00 edited a comment on pull request #1390: [WIP][SYSTEMDS-3124] Federated Binary Aggregation and (Map)MM - Federated Output
Posted by GitBox <gi...@apache.org>.
ywcb00 edited a comment on pull request #1390:
URL: https://github.com/apache/systemds/pull/1390#issuecomment-938587552
Thanks for pointing this out @sebwrede.
I found an endless loop formed by _MatrixObject:438_ and _CacheableData:565_, which occurs because we exclude the _FType.BROADCAST_ type from the federation check in _CacheableData_ and include it for the federation check in the _MatrixObject_.
That's why the _acquireReadIntern()_ and _readBlobFromHDFS()_ methods always call each other until it results in a StackOverflow.
I'm trying to find a solution for it :eyes:
--
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] sebwrede commented on pull request #1390: [SYSTEMDS-3124] Federated Binary Aggregation and (Map)MM - Federated Output
Posted by GitBox <gi...@apache.org>.
sebwrede commented on pull request #1390:
URL: https://github.com/apache/systemds/pull/1390#issuecomment-937626683
> When I resolved the conflicts with the master branch, I saw that Spark tests were added to the FederatedRowAggregate tests. Since Spark uses (Map)MM instead of the AggregateBinary instruction, I had to make the very same changes to the (Map)MM instruction as I did to the AggregateBinary instruction.
There is now a StackOverflowError in FederatedMSVMTest.federatedMSVM1CP. I think it is because of MatrixObject.java:438, where an `acquireReadAndRelease()` call is made when the matrix object is federated. This does not work when it has to read from HDFS. I am not entirely sure how your changes causes this, but we need to change it so that it does not call `acquireReadAndRelease()` whenever the data is federated. It should instead make some check to see if it needs to call `DataConverter.readMatrixFromHDFS` even if the object itself 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] ywcb00 closed pull request #1390: [WIP][SYSTEMDS-3124] Federated Binary Aggregation and (Map)MM - Federated Output
Posted by GitBox <gi...@apache.org>.
ywcb00 closed pull request #1390:
URL: https://github.com/apache/systemds/pull/1390
--
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