Skip to content

[SYSTEMDS-3382] Federated Left Indexing with Scalar#1622

Closed
ywcb00 wants to merge 2 commits into
apache:mainfrom
ywcb00:fix/runtime/instructions/fed/indexing/left
Closed

[SYSTEMDS-3382] Federated Left Indexing with Scalar#1622
ywcb00 wants to merge 2 commits into
apache:mainfrom
ywcb00:fix/runtime/instructions/fed/indexing/left

Conversation

@ywcb00
Copy link
Copy Markdown
Contributor

@ywcb00 ywcb00 commented May 22, 2022

Hi,
this PR adds support for the federated left indexing instruction with a scalar rhs, including the respective tests.
When running the FederatedLeftIndexTest, it throws an exception because of invalid matrix dimensions (-1, -1) at the first test run. The second test run passes. Somehow this exception is only thrown if the two frame tests (testLeftIndexFullDenseFrameCP, testLeftIndexFullDenseFrameSP) are executed before the failing scalar test (testLeftIndexScalarCP). I tried to put the empty matrix for the output to the federated workers with the correct sliced dimensions instead of [-1, -1], but then we get a "file does not exist on hdfs" exception. Any idea or suspicion why this could be the case?

Thanks for review :)

ywcb00 added 2 commits May 22, 2022 11:48
…xing

chore(FederationMap.java): override the execute method to directly call it with single items instead of arrays

feat(FederatedLeftIndexScalarTest): add test for scalar left indexing
@mboehm7
Copy link
Copy Markdown
Contributor

mboehm7 commented Jun 5, 2022

LGTM - thanks for the patch @ywcb00 as well as catching and reporting this issue.

After spending way too much time on debugging this issue, I found the reason and fixed it. A simple FederationUtils.resetFedDataID() at the beginning of the test mitigated the described problem, which however pointed to a more severe issue. In detail, after running the two other tests, we got an output ID 13 for the federated left indexing. The scalar literal was also 13 which lead to incorrect replacements when preparing the individual instructions sent to the workers. By rearranging the order to input/output replacements, we now generally prevent such problems. Thanks.

@asfgit asfgit closed this in fa81f6a Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants