-
Notifications
You must be signed in to change notification settings - Fork 82
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
Migrating Python model REST protocol test on triton for Kserve ( UI -> API ) #2133
Migrating Python model REST protocol test on triton for Kserve ( UI -> API ) #2133
Conversation
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.
Robocop found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Robot Results
|
Verified with Jenkins Build 2104 |
@@ -339,7 +339,7 @@ Get Model Inference | |||
${rc} ${url}= Run And Return Rc And Output | |||
... oc get ksvc ${model_name}-predictor -n ${project_title} -o jsonpath='{.status.url}' | |||
Should Be Equal As Integers ${rc} 0 | |||
${curl_cmd}= Set Variable curl -s ${url}${end_point} -d ${inference_input} | |||
${curl_cmd}= Set Variable curl -ks ${url}${end_point} -d ${inference_input} |
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 should already have the logic to retrieve the certificate instead of using "-k" flag. Could you try using that?
Set Default Storage Class In GCP default=standard-csi | ||
RHOSi Teardown | ||
|
||
Setup Test Variables # robocop: off=too-many-calls-in-keyword |
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.
why duplicating this keyword and the following ones? we already have them in common files don't we?
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.
No we don't have these keywords in Common file
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.
Compile Inference Service YAML
exits in ods_ci/tests/Resources/CLI/ModelServing/llm.resource
About Setup Test Variables
, true it is not in a common file but it is in
ods_ci/tests/Tests/1000__model_serving/1007__model_serving_llm/1007__model_serving_llm_models.robot
. However, I think you should move it to a common file rather than duplicating it here
Verified with Jenkins Build 2196 |
should this one be backported to other branches for supported releases? |
@@ -414,6 +416,46 @@ Query Model Multiple Times | |||
END | |||
END | |||
|
|||
Setup Test Variables # robocop: off=too-many-calls-in-keyword |
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.
the keyword already exists in
Line 1305 in 1157066
Setup Test Variables # robocop: off=too-many-calls-in-keyword |
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.
@rnetser It does not come under the scope of this PR; it will be checked and updated in upcoming PRs.
Setup Test Variables model_name=${PYTHON_MODEL_NAME} use_pvc=${FALSE} use_gpu=${FALSE} | ||
... kserve_mode=${KSERVE_MODE} model_path=triton/model_repository/ | ||
Set Project And Runtime runtime=${KSERVE_RUNTIME_REST_NAME} protocol=${PROTOCOL} namespace=${test_namespace} | ||
... download_in_pvc=${DOWNLOAD_IN_PVC} model_name=${PYTHON_MODEL_NAME} | ||
... storage_size=100Mi memory_request=100Mi | ||
${requests}= Create Dictionary memory=1Gi | ||
Compile Inference Service YAML isvc_name=${PYTHON_MODEL_NAME} | ||
... sa_name=models-bucket-sa | ||
... model_storage_uri=${storage_uri} | ||
... model_format=python serving_runtime=${KSERVE_RUNTIME_REST_NAME} | ||
... version="1" | ||
... limits_dict=${limits} requests_dict=${requests} kserve_mode=${KSERVE_MODE} | ||
Deploy Model Via CLI isvc_filepath=${INFERENCESERVICE_FILLED_FILEPATH} | ||
... namespace=${test_namespace} | ||
# File is not needed anymore after applying | ||
Remove File ${INFERENCESERVICE_FILLED_FILEPATH} | ||
Wait For Pods To Be Ready label_selector=serving.kserve.io/inferenceservice=${PYTHON_MODEL_NAME} | ||
... namespace=${test_namespace} |
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.
this block is repeated in multiple tests; create a test template and re-use
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 will discuss with @tarukumar and create a template.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bdattoma, Raghul-M, tarukumar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Quality Gate passedIssues Measures |
No description provided.