-
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] Make top candidates view as the default view in user-tools #879
Conversation
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]> Fixes NVIDIA#868 - Use Top-candidate view as default for user-tools qualification - Revisit the spark_rapids CLI to verify that resetting the filter based on user-input does not override the default flag argument - Fix unit-tests
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.
Currently the qualification.yaml also contains an entry for default value of filter apps as SAVINGS
. It seems to be used at a single place only. Probably we should remove it or it can cause consistency issues.
Additionally, this seems more of a general bug, in method __process_filter_args()
we validate the selected filter as
try:
selected_filter = QualFilterApp.fromstring(arg_val)
except:
However, fromstring()
does not throw an Exception. It will return None
if the key does not exist. This can cause the weird selection of filter app and we do not display the warning message as well.
user_tools/tests/spark_rapids_tools_ut/test_tool_argprocessor.py
Outdated
Show resolved
Hide resolved
user_tools/tests/spark_rapids_tools_ut/test_tool_argprocessor.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Good catch. |
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 @amahussein. Created a tracking issue #887 to handle fromstring()
for invalid enums
@@ -102,7 +105,7 @@ def qualification(cpu_cluster: str = None, | |||
if cpu_cluster is None: | |||
raise RuntimeError('OnPrem\'s cluster property file required to calculate' | |||
'savings for ' + target_platform + ' platform') | |||
filter_apps: str = QualFilterApp.tostring(QualFilterApp.SAVINGS) | |||
# filter_apps: str = QualFilterApp.tostring(QualFilterApp.SAVINGS) |
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: remove commented code.
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 @amahussein!
Signed-off-by: Ahmed Hussein (amahussein) [email protected]
Fixes #868