[FEA] Profiler autotuner should only specify standard Spark versions for shuffle manager setting #662
Conversation
kuhushukla
force-pushed
the
sm_ver_profiler
branch
from
a13d43d to
9df400b
Compare
core/src/main/scala/com/nvidia/spark/rapids/tool/profiling/AutoTuner.scala
Outdated
Show resolved
Hide resolved
…for shuffle manager setting Signed-off-by: Kuhu Shukla <[email protected]>
kuhushukla
force-pushed
the
sm_ver_profiler
branch
from
ec0a5ad to
deea588
Compare
|
Added test, addressed comment and taking it out of draft. Will tag reviewers when build is green. |
Signed-off-by: Kuhu Shukla <[email protected]>
kuhushukla
force-pushed
the
sm_ver_profiler
branch
from
cfc83e1 to
39fbf94
Compare
kuhushukla
requested review from
parthosa and
amahussein
and removed request for
parthosa and
amahussein
Signed-off-by: Kuhu Shukla <[email protected]>
There was a problem hiding this comment.
Thanks @kuhushukla for the changes. I have some few questions
| val finalShuffleVersion : String = if (platform.getName == PlatformNames.DATABRICKS_AWS | ||
| || platform.getName == PlatformNames.DATABRICKS_AZURE) { | ||
| val dbVersion = appInfoProvider.getProperty( | ||
| "spark.databricks.clusterUsageTags.sparkVersion").getOrElse("") | ||
| dbVersion match { |
There was a problem hiding this comment.
For the if-condition, I would think that we should not rely on the platform input.
Instead we look for a property that distinguished eventlogs generated on Databricks.
For example,
- if this configuration
spark.databricks.clusterUsageTags.sparkVersionis always set for all Databricks executions, then all we need to do is to check that this config entry is not emptyt - If the above is not always true, then we can use pattern match of
SparkVersionpulled from the app configuration indicates it is a databricks-spark
Deciding the platform from the configuration is going to be a good step toward improving the analysis. We are going to need that anyway.
There was a problem hiding this comment.
Yeah, I sort of alluded to that when I first put the PR up. I like trying out option#1 above. Will update.
| dbVersion match { | ||
| case ver if ver.contains("10.4") => "321db" | ||
| case ver if ver.contains("11.3") => "330db" | ||
| case _ => "332db" |
There was a problem hiding this comment.
This is a good start. We can have this list hardcoded for this PR.
However moving forward, it is going to maintain the code everytime a new DB class is added.
Perhaps later we can create a static properties file to map between db and plugin versions? At least we won't need to change scala code for that. Also, we can automate a script whenever a new class is added in the plugin repo. WDYT?
There was a problem hiding this comment.
Completely agree. This needs to be maintained some place. Do we have examples of static properties files elsewhere at the moment? If not, we can start with this property. Automating is more interesting as adding a new class may not be a complete indicator that we need a new config key, DB in particular might fit that use case.
There was a problem hiding this comment.
I started the seed to this infrastructure in the existing PR #705 (loadPropertiesFile)
My plan is to build on that by adding trait configurable that will load the the properties file from a static file.
There was a problem hiding this comment.
Should I address the change here as part of this PR or are we good with this for now?
There was a problem hiding this comment.
This is good for this PR.
| case ver if ver.contains("11.3") => "330db" | ||
| case _ => "332db" | ||
| } | ||
| } else shuffleManagerVersion |
There was a problem hiding this comment.
it would be more readable
} else {
shuffleManagerVersion
}
core/src/test/scala/com/nvidia/spark/rapids/tool/profiling/AutoTunerSuite.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Kuhu Shukla <[email protected]>
Signed-off-by: Kuhu Shukla <[email protected]>
|
@amahussein I have revised the PR to address all but one (the properties file) change. Please take another look. Thank u! |
| dbVersion match { | ||
| case ver if ver.contains("10.4") => "321db" | ||
| case ver if ver.contains("11.3") => "330db" | ||
| case _ => "332db" |
There was a problem hiding this comment.
This is good for this PR.
Fixes #599
Draft : needs test which is WIP. I also wonder if it is ok to have the platform check as I have here at the moment. I know it is yet another thing user must pass correctly so I am testing a version which does not need it and we infer that we are not on Databricks.