-
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] Add Estimation Model to Qualification CLI #870
Conversation
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]> Fixes NVIDIA#869 - Add `estimation_model` to qualification arguments - Refactor the job sumbission to run as concurrent processes as we need to run both qualification and profiling tool in the xgboost model - Remove `--per-sql` from the allowed list of qualification tool because it is used in the XGBOOST model
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
@@ -92,6 +93,10 @@ def qualification(cpu_cluster: str = None, | |||
"MATCH": keep GPU cluster same number of nodes as CPU cluster; | |||
"CLUSTER": recommend optimal GPU cluster by cost for entire cluster; | |||
"JOB": recommend optimal GPU cluster by cost per job. | |||
:param estimation_model: Model used to calculate the estimated GPU duration and cost savings. | |||
It accepts one of the following: | |||
"XGBOOST": an XGBoost model for GPU duration estimation |
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.
minor preference to use lowercase for argument values: XGBOOST --> xgboost
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 @mattahrens.
Done!
The CLI handles both lower/upper-cases. I changed the comments to lower-case which reflects on the output of the --help
command.
Signed-off-by: Ahmed Hussein (amahussein) <[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 @amahussein.
Signed-off-by: Ahmed Hussein (amahussein) [email protected]
Fixes #869
estimation_model
to qualification arguments--per-sql
from the allowed list of qualification tool because it is used in the XGBOOST model--estimation_model XGBOOST
runs the prediction model and generate the results as intermediate output, but it won't affect the final resultsqual_*_app.csv
andqual_*_sql
Notes: