Fix string comparison for memory overhead in pinned pool size recommendation in AutoTuner #1508
Conversation
Signed-off-by: Partho Sarthi <[email protected]>
Signed-off-by: Partho Sarthi <[email protected]>
Signed-off-by: Partho Sarthi <[email protected]>
parthosa
added
bug
Signed-off-by: Partho Sarthi <[email protected]>
| appendRecommendationForMemoryMB(memOverheadLookup, recomValue) | ||
| getPropertyValue("spark.rapids.memory.pinnedPool.size").foreach { lookup => | ||
| if (lookup != "spark.executor.memoryOverhead") { |
There was a problem hiding this comment.
This comparison was incorrect because lookup is a memory string value, whereas spark.executor.memoryOverhead is a label
| case object Standalone extends SparkMaster | ||
|
|
||
| object SparkMaster { | ||
| def apply(master: Option[String]): Option[SparkMaster] = { |
There was a problem hiding this comment.
nit: indent here is inconsistent 4 vs 2
| val platform = PlatformFactory.createInstance(PlatformNames.ONPREM, clusterPropsOpt) | ||
| val autoTuner: AutoTuner = ProfilingAutoTunerConfigsProvider | ||
| .buildAutoTunerFromProps(dataprocWorkerInfo, infoProvider, | ||
| platform) |
There was a problem hiding this comment.
nit: we probably do not need a newline here -
.buildAutoTunerFromProps(dataprocWorkerInfo, infoProvider, platform)
| val platform = PlatformFactory.createInstance(PlatformNames.ONPREM, clusterPropsOpt) | ||
| val autoTuner: AutoTuner = ProfilingAutoTunerConfigsProvider | ||
| .buildAutoTunerFromProps(dataprocWorkerInfo, infoProvider, | ||
| platform) |
There was a problem hiding this comment.
nit: same as above
| val platform = PlatformFactory.createInstance(PlatformNames.ONPREM, clusterPropsOpt) | ||
| val autoTuner: AutoTuner = ProfilingAutoTunerConfigsProvider | ||
| .buildAutoTunerFromProps(dataprocWorkerInfo, infoProvider, | ||
| platform) |
There was a problem hiding this comment.
nit: same as above
| // This UT sets a custom spark-property "spark.master" pointing to a spark | ||
| // k8s value. The Autotuner should detect that the spark-master is k8s and | ||
| // should not comment on the missing memoryOverhead value since pinned pool is not set. | ||
| test(s"missing memoryOverhead comment is not included for k8s without pinned pool") { |
There was a problem hiding this comment.
can we include spark-version in the test comment to differentiate between the 2 test runs?
There was a problem hiding this comment.
Included spark version in the test name since spark version is dynamic and set during runtime.
| // value. The Autotuner should detect that the spark-master is yarn and | ||
| // should not comment on the missing memoryOverhead value even though pinned | ||
| // pool is set. | ||
| test("missing memoryOverhead comment is not included for yarn") { |
There was a problem hiding this comment.
can we include spark-version in the test comment to differentiate between the 2 test runs?
There was a problem hiding this comment.
Included spark version in the test name since spark version is dynamic and set during runtime.
| // if using k8s and pinned pool size is set, add a comment if memory overhead is missing | ||
| if (sparkMaster.contains(Kubernetes) && | ||
| getPropertyValue(pinnedPoolSizeLookup).isDefined && | ||
| getPropertyValue(memOverheadLookup).isEmpty) { |
There was a problem hiding this comment.
nit: inconsistent indent here
Signed-off-by: Partho Sarthi <[email protected]>
Fixes #1398.
There were two bugs related to setting pinned pool size:
Incorrect cluster memory calculation with dynamic allocation enabled - This was resolved in #1121 by fixing cluster shape and memory calculation.
Memory value string (e.g.,
4G) was incorrectly compared to the labelspark.executor.memoryOverhead.This PR specifically fixes the second issue by ensuring the memory comparison is performed correctly. The corrected condition is as follows:
Key changes:
Handling Spark Master Types:
SparkMasterwith case objectsLocal,Yarn,Kubernetes, andStandaloneto represent different Spark master types.sparkMasterin theAutoTunerclass to determine the Spark master type based on thespark.masterproperty.Memory Overhead Recommendation Logic:
memoryOverheadLabelmethod to use the newsparkMasterfield instead of directly accessing thespark.masterproperty. [1] [2]addRecommendationForMemoryOverheadmethod to check if the Spark master is notStandalonebefore adding memory overhead recommendations.enableMemoryOverheadRecommendationmethod from theAutoTunerConfigsProvidertrait as it is no longer needed.Test Suite Updates:
compareOutputmethod in theProfilingAutoTunerSuiteto improve test output comparison. [1] [2]spark.executor.memoryOverheadfrom multiple test cases in theProfilingAutoTunerSuite. [1] [2] [3] [4] [5] [6] [7] [8] [9]