[SPARK-29823][MLLIB] Improper persist strategy in mllib.clustering.KMeans.run()

[amanomer]

What changes were proposed in this pull request?

Adjust RDD to persist.

Why are the changes needed?

To handle the improper persist strategy.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manually

[amanomer]

norms have only one child zippedData so all actions that rely on norms also rely on zippedData and in runAlgorithm(), multiple actions have been applied on zippedData.

[amanomer]

cc @srowen

[srowen]

OK looks good as a point fix, pending tests. I checked and VectorWithNorm is registered by default wit Kryo, so serializing it to memory should be OK

[SparkQA]

Test build #4930 has finished for PR 26483 at commit 99f165d.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Merged to master

[zhengruifeng]

@srowen @amanomer Do we need this change?
data: RDD[Vector] and norms: RDD[Double] are both cached, zippedData just involved extra cost of convert vectors to VectorWithNorms.

Do this help improving performace? I donot see any performance result.
on the contrary, this will cause double caching problem:
Since vectors are already cached in the .ml or maybe outside of the run method, but never used.

[zhengruifeng]

I mean, input: RDD[Vector] is likely to be cached outside of this method:
1, it is cached in ml.KMeans
2, end uers are likely to cache it outside of train/run, since it is suggested in related docs

So if we cache zippedData, we really cache input twice.

[srowen]

It's better than the previous behavior, but yes this could also instead only bother persisting if the input wasn't (and remove the warning about persisting the input)

[zhengruifeng]

@srowen I agree that there should be somewhat perfermance improvement if RAM is big enough, at the cost of double caching.
However if the RAM can not fit the two copies, I think this will hurt the perfermance.

End uers are telled to cache the input RDD/DF for ML algorithms, and I think double caching matters more than this perfermance improvement.
So I am against to this change.

[srowen]

Yes I'm not suggesting two caches is helpful here; it was doing that before too though. That's not what this change does. I do think it's fine to follow this up and make it conditionally cache. @amanomer are you interested?

[amanomer]

Yes. I will raise a follow up PR.