-
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
Remove total core second heuristic and filter apps only in top candidate view #1376
Remove total core second heuristic and filter apps only in top candidate view #1376
Conversation
Signed-off-by: cindyyuanjiang <[email protected]>
Signed-off-by: cindyyuanjiang <[email protected]>
Signed-off-by: cindyyuanjiang <[email protected]>
can you remove bullet 1 in the description. or reword it to say from the application_summary file, it was a bit confusing when I read it until I read the second bullet because I thought this some how changed to remove it fully. most people won't know when you say heuristic it doesn't apply to everything |
Thanks @tgravescs! I updated the PR description. |
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. Added a minor comment.
Also after this change in, I was thinking that we should improve usage of Heuristics + Filtering and what should be used when. Created a tracking issue #1379
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. LGTME.
Fixes #1367
Changes
qualification_summary.csv
file.--filter_apps=TOP_CANDIDATES
. Nothing should be affected when--filter_apps=ALL
.Testing
Cmd:
spark_rapids qualification -v -e <my-event-logs> --filter_apps 'ALL'
Stdout:
Cmd:
spark_rapids qualification -v -e <my-event-logs>
Stdout: