-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-16933] [ML] Fix AFTAggregator in AFTSurvivalRegression serializes unnecessary data. #14519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Test build #63314 has finished for PR 14519 at commit
|
|
Let's put this into #14109 |
| // sigma is the scale parameter of the AFT model | ||
| private val sigma = math.exp(parameters(0)) | ||
| @transient private lazy val sigma = math.exp(parameters(0)) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 506,
private val gradientSumArray = Array.ofDim[Double](parameters.length)the code will evaluate the lazy parameters in the driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, after thinking a bit, some of the lazy is not needed. lazy is for avoiding doing computation in the driver; however
@transient private val parameters = bcParameters.value should work without lazy. Also, sigma or intercept may not need lazy. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbtsai I addressed the parameters.length issue. But I can not remove lazy from @transient private lazy val parameters = bcParameters.value and intercept/sigma. Otherwise, it complains NullPointerException. If I removed both @transient and lazy, it works well, but this does not coincide with our requirements. It's a little weird and I'm still work on to figure out the root cause, can you give me some suggestion? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. In scala, when we use @transient private val, that lazy evaluation will be only evaluated once even after serialization/deserialization cycle. As a result, after the AFTAggregator is broadcasted into executors, the variable will be be evaluated again, and will be default to null.
|
Test build #63362 has finished for PR 14519 at commit
|
|
@yanboliang Can you do a quick test to make sure the shuffle write size is the expected size? For example, in logistic regression only the gradient should be serialized which is an array of |
|
LGTM. Will be nice to see the compassion of shuffle write size, and then will be ready to merge. Thanks. |
What changes were proposed in this pull request?
Similar to
LeastSquaresAggregatorin #14109,AFTAggregatorused forAFTSurvivalRegressionends up serializing theparametersandfeaturesStd, which is not necessary and can cause performance issues for high dimensional data. This patch removes this serialization. This PR is highly inspired by #14109.How was this patch tested?
I tested this locally and verified the serialization reduction.
Before patch

After patch
