Uh oh!
There was an error while loading. Please reload this page.
[SPARK-28062][ML] Avoid unnecessary copy of coefficients vector in HuberAggregator - #24880
[SPARK-28062][ML] Avoid unnecessary copy of coefficients vector in HuberAggregator#24880Andrew-Crosby wants to merge 2 commits into
Conversation
…s added Follows approach used in LeastSquaresAggregator
Andrew-Crosby
commented
Jun 17, 2019
@yanboliang@sethah does this look reasonable to you? |
| 0.0 | ||
| } | ||
| // make transient so we do not serialize between aggregation stages | ||
| @transient private lazy val featuresStd = bcFeaturesStd.value |
There was a problem hiding this comment.
I don't think this change makes sense. It is just getting the broadcast, not a big overhead...
There was a problem hiding this comment.
Yes, this one isn't necessary. coefficients looks good. But it doesn't need to be lazy.
There was a problem hiding this comment.
Thanks for the feedback. I've removed the unnecessary change to featuresStd.
@srowen I tried removing the lazy modifier, but that causes both the unit tests and my test case to fail with the following NPE. I don't understand why.
org.apache.spark.SparkException: Job aborted due to stage failure: Task 2 in stage 3.0 failed 1 times, most recent failure: Lost task 2.0 in stage 3.0 (TID 11, localhost, executor driver): java.lang.NullPointerException
at org.apache.spark.ml.optim.aggregator.HuberAggregator.$anonfun$add$3(HuberAggregator.scala:109)
at org.apache.spark.ml.linalg.SparseVector.foreachActive(Vectors.scala:613)
at org.apache.spark.ml.optim.aggregator.HuberAggregator.add(HuberAggregator.scala:107)
There was a problem hiding this comment.
Oh I get it. You wouldn't want to eagerly evaluate the broadcast as it might eval on the driver. OK I think this is a reasonable solution.
mgaido91
commented
Jun 18, 2019
LGTM, @srowen may you please start the CI? Thanks. |
SparkQA
commented
Jun 18, 2019
Test build #4800 has finished for PR 24880 at commit
|
srowen
commented
Jun 19, 2019
Merged to master |
What changes were proposed in this pull request?
Modifies the HuberAggregator class so that a copy of the coefficients vector isn't created every time that an instance is added. Follows the approach of LeastSquaresAggregator and uses transient lazy class variable to store the reused quantities. (See #14109 for explanation of the use of transient lazy variables)
On the test case in the linked JIRA, this change gives an order of magnitude performance improvement reducing the time taken to fit the model from 540 to 47 seconds.
How was this patch tested?
Existing unit tests.
See https://issues.apache.org/jira/browse/SPARK-28062 for results from running a benchmark script.