-
Notifications
You must be signed in to change notification settings - Fork 45
Add "preserve" and "exclude" sections to target cluster #1879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Partho Sarthi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds "preserve" and "exclude" sections to the target cluster configuration for AutoTuner, providing greater flexibility in property management beyond the existing "enforced" section. The preserve list maintains original CPU job property values, while the exclude list removes properties from tuning recommendations.
Key changes:
- Extended
SparkProperties
class to support preserve and exclude lists with validation - Enhanced AutoTuner logic to handle preserved properties and exclude unwanted ones
- Added comprehensive test coverage for overlapping properties and edge cases
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
QualificationAutoTunerSuite.scala | Added comprehensive test cases for preserve/exclude functionality and validation |
ToolTestUtils.scala | Extended utility methods to support preserve/exclude parameters in test setup |
TargetClusterProps.scala | Extended SparkProperties with preserve/exclude fields and overlap validation |
AutoTuner.scala | Core implementation for handling preserved/excluded properties in recommendations |
Platform.scala | Added helper methods to check if properties are preserved or excluded |
tuningTable.yaml | Added new tuning definition for incompatible date formats property |
targetCluster07.yaml | Added example configuration demonstrating preserve/exclude usage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
core/src/main/scala/com/nvidia/spark/rapids/tool/tuning/AutoTuner.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/nvidia/spark/rapids/tool/tuning/AutoTuner.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Partho Sarthi <[email protected]>
core/src/test/scala/com/nvidia/spark/rapids/tool/tuning/QualificationAutoTunerSuite.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/nvidia/spark/rapids/tool/tuning/AutoTuner.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/nvidia/spark/rapids/tool/tuning/AutoTuner.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/nvidia/spark/rapids/tool/tuning/TargetClusterProps.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @parthosa ,overall looks good ! Just some questions regarding the overall flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this issue for sometime where the union of the recommendations show properties that are provided by the CSP. #1257
Two main points I see here:
- Just curious if this implementation allows us later to define an automatic excluded list for each CSP?
- excluded/preserve flags: we may want to consider to allow properties that are not necessarily handled by the AutoTuner. For example, does the
property
name be defined in the tuningTable.yaml in order to be considered a valid excluded/preserve entry?
Yes. There is a CSP/Platform specific list
This PR supports this. I have added E2E test that covers the following: val excludeProperties = List(
// Exclude a property that is set in event log
"spark.master",
// Exclude a property that is recommended by AutoTuner
"spark.rapids.sql.concurrentGpuTasks"
)
val preserveProperties = List(
// Preserve a property that is not present in event log
"spark.task.resource.gpu.amount",
// Preserve a property that is present and recommended by AutoTuner
"spark.executor.memory",
// Preserve a property that is present but not recommended by AutoTuner
"spark.dataproc.engine"
)
val enforcedSparkProperties = Map(
"spark.sql.shuffle.partitions" -> "800"
) The current implementation leverages:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks @parthosa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @parthosa . LGTM
Fixes #1873
Problem Statement
Currently, AutoTuner's target cluster only supports "enforced" section for overriding property values. In some cases, users need additional flexibility to:
Changes
This PR introduces two new sections to the target cluster:
1. "preserve" Section
'<property>' was preserved from source application properties as specified in target cluster.
2. "exclude" Section
'<property>' was excluded from tuning recommendations as specified in target cluster.
Example Target Cluster (also included in the PR).
Key Changes
Core Implementation
isPropertyPreserved()
andisPropertyExcluded()
methodsfinalTuningTable
creation to handle preserve/exclude listsConfiguration Management
SparkProperties
class withpreserve
andexclude
fieldsTesting
IllegalArgumentException
)