[SPARK-18471][MLLIB] In LBFGS, avoid sending huge vectors of 0 #15963
Conversation
CostFun used to send a dense vector of zeroes as a closure in a treeAggregate call. To avoid that, we replace treeAggregate by mapPartition + treeReduce, creating a zero vector inside the mapPartition block in-place.
[SPARK-18471][MLLIB] In LBFGS, avoid sending huge vectors of 0
|
Can one of the admins verify this patch? |
|
@srowen Here is at last the real PR for SPARK-18471. Sorry for the noise due to GitHub fiddling... |
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
I think it's still worth looking into making a similar change elsewhere that seqOp and combOp are used this way, and which might also accidentally take something into the closure. But (modulo minor changes) I think this is an OK change.
| seqOp = (c, v) => (c, v) match { case ((grad, loss), (label, features)) => | ||
| val l = localGradient.compute( | ||
| features, label, bcW.value, grad) | ||
| /** Given (current accumulated gradient, current loss) and (label, features) |
There was a problem hiding this comment.
Nit: just use // for two lines of comment. Really /** (vs /*) is for javadoc.
| }) | ||
| } | ||
|
|
||
| val (gradientSum, lossSum) = data.mapPartitions { it => { |
There was a problem hiding this comment.
Nit: the second brace and its match are redundant
|
Ping @AnthonyTruchet |
|
Hello @srowen , sorry not to have updated this lately, I've been taken by other emergencies at work. I'll update this on Monday. Actually I'll submit a variant of treeAggregate in core that we will be able to use for other similar use case in ML(lib). I appreciate your care about those patches and will attempt to reach a reasonable reactivity too :-) |
|
Once more (last time hopefully) I mistakenly fiddled with PR. Closing this one and replace it with #16037. |
3 participants
What changes were proposed in this pull request?
CostFun used to send a dense vector of zeroes as a closure in a
treeAggregate call. To avoid that, we replace treeAggregate by
mapPartition + treeReduce, creating a zero vector inside the mapPartition
block in-place.
How was this patch tested?
Unit test for module mllib run locally for correctness.
As for performance we run an heavy optimization on our production data (50 iterations on 128 MB weight vectors) and have seen significant decrease in terms both of runtime and container being killed by lack of off-heap memory.