-
Notifications
You must be signed in to change notification settings - Fork 37
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
[FEA] Skip Databricks Photon jobs at app level in Qualification tool #886
Conversation
Signed-off-by: cindyyuanjiang <[email protected]>
Signed-off-by: cindyyuanjiang <[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.
Thanks @cindyyuanjiang ! Just a couple of nits.
@@ -795,6 +795,10 @@ class QualificationAppInfo( | |||
val sqlPlanInfoGraphEntry = SqlPlanInfoGraphBuffer.createEntry(sqlID, planInfo) | |||
checkMetadataForReadSchema(sqlPlanInfoGraphEntry) | |||
for (node <- sqlPlanInfoGraphEntry.sparkPlanGraph.allNodes) { | |||
if (node.name.contains("Photon")) { |
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.
Nit: Does Photon operators always begin with Photon keyword. If so can we use startsWith instead of contains here.
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.
I am not certain, but from previous observations in: #449 (comment), it looks like they begin with "Photon". I will update this.
case gpuLog: GpuEventLogException => | ||
gpuLog.message | ||
("unknow", gpuLog.message) |
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.
Nit: unknow -> unknown
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, updated!
Signed-off-by: cindyyuanjiang <[email protected]>
Signed-off-by: cindyyuanjiang <[email protected]>
core/src/main/scala/com/nvidia/spark/rapids/tool/qualification/Qualification.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/qualification/QualificationAppInfo.scala
Show resolved
Hide resolved
Signed-off-by: cindyyuanjiang <[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.
Thanks @cindyyuanjiang !
I am fine with merging this as is for now, given that we investigate moving the logic of detection during the eventlog-processing phase as described in my comment.
No need to file a followup as\the investigation can be part of the existing #845, which is pretty similar to this feature.
@@ -795,6 +795,10 @@ class QualificationAppInfo( | |||
val sqlPlanInfoGraphEntry = SqlPlanInfoGraphBuffer.createEntry(sqlID, planInfo) | |||
checkMetadataForReadSchema(sqlPlanInfoGraphEntry) | |||
for (node <- sqlPlanInfoGraphEntry.sparkPlanGraph.allNodes) { | |||
if (node.name.startsWith("Photon")) { |
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.
I thought that we could detect the Photon in an earlier phase as we process the eventlogs.
For example, doSparkListenerSQLExecutionStart()
could check for certain conditions to disqualify the entire app.
The pros of having the code in doSparkListenerSQLExecutionStart()
:
- For Qualification tool, the bottleneck of performance is actually in reading and processing the evntlogs. Therefore, if we fail during that stage, then we are saving significant amount of useless processing and computation.
- If we need to propagate that logic to Profiling, it becomes much easier as we need only to move the implementation to the
EvenProcessorBase.doSparkListenerSQLExecutionStart
instead ofQualificationEventProcessor.doSparkListenerSQLExecutionStart
. Qualification if we need
Fixes #804
This PR implements the logic to skip Databricks Photon jobs at app level in Qualification tool. Each Photon event log will be marked at
SKIPPED
inrapids_4_spark_qualification_output_status.csv
and removed from other generated files.Changes
SkippedQualAppResult
to represent skipped Qualification App creation andskippedCounter
for console progress bar visualizationPhoton
when processing SQL plans and short-circuit the process if there exists oneFailureApp
for handling different kinds of exceptions when creating Qualification AppTests
Before this PR:
rapids_4_spark_qualification_output.csv
rapids_4_spark_qualification_output_status.csv
Console Log
After this PR:
rapids_4_spark_qualification_output.csv
rapids_4_spark_qualification_output_status.csv
Console Log