[Minor][ML] Fix some PySpark & SparkR flaky tests #17757
Conversation
|
Test build #76133 has finished for PR 17757 at commit
|
|
Hi @yanboliang, do you mind if I ask why they were flaky? I am just curious and want to know. |
|
@HyukjinKwon We hit this issue at #17746 when we upgrade breeze to 0.13.1. Since these tests don't converged, we check intermediate result which is vulnerable, so I switched to check the last converged result. Thanks. |
There was a problem hiding this comment.
If set maxIter = 2, the result is not converged, so the result is vulnerable. We should check the last converged result.
|
LGTM. Please merge the current master to resolve the conflicts. |
There was a problem hiding this comment.
so is there any result we could use when it is converged?
we have remove a call to predict - we should keep the call to make sure the api works and ideally check for the prediction results too if we could
There was a problem hiding this comment.
Yeah, here we just removed the unconverged test(with maxIter = 2), since we can't guarantee any equality during the iteration. I think the best way to test the api works well is to check number of iterations. If we set proper initial weights, the number of iterations to converge would be different from other initial weights or no initial weights. Let's open a separate JIRA to expose training summary for MLP at MLlib side, and then we can expose them at SparkR and add check here. Thanks.
There was a problem hiding this comment.
I got the uncoverged test with the maxIter.
My main concern at this end is to at least exercise calling from R to JVM for each public API we export (ie. by calling predict on the MLP model) - we have had issues in the past the API never works and/or it is broken and we don't know.
There was a problem hiding this comment.
checking more closely it looks like earlier tests do call predict. I'm good with simplifying this part of the test with weights.
|
Test build #76168 has finished for PR 17757 at commit
|
## What changes were proposed in this pull request? Some PySpark & SparkR tests run with tiny dataset and tiny ```maxIter```, which means they are not converged. I don’t think checking intermediate result during iteration make sense, and these intermediate result may vulnerable and not stable, so we should switch to check the converged result. We hit this issue at #17746 when we upgrade breeze to 0.13.1. ## How was this patch tested? Existing tests. Author: Yanbo Liang <[email protected]> Closes #17757 from yanboliang/flaky-test. (cherry picked from commit dbb06c6) Signed-off-by: Yanbo Liang <[email protected]>
|
Merged into master and branch-2.2. Thanks for all reviewing. |
5 participants
What changes were proposed in this pull request?
Some PySpark & SparkR tests run with tiny dataset and tiny
maxIter, which means they are not converged. I don’t think checking intermediate result during iteration make sense, and these intermediate result may vulnerable and not stable, so we should switch to check the converged result. We hit this issue at #17746 when we upgrade breeze to 0.13.1.How was this patch tested?
Existing tests.