From 174f4a5fd5b3021dcd7e67f902ff2a08142ac611 Mon Sep 17 00:00:00 2001 From: lugi0 Date: Tue, 26 Nov 2024 16:26:12 +0100 Subject: [PATCH] Address PR comments Signed-off-by: lugi0 --- tests/conftest.py | 8 ++--- tests/constants.py | 22 ++++++++++++++ tests/model_serving/constants.py | 2 -- .../model_server/private_endpoint/conftest.py | 20 ++++++------- .../test_kserve_private_endpoint.py | 29 +++++++++++-------- .../model_server/private_endpoint/utils.py | 10 +++---- .../model_server/storage/pvc/conftest.py | 4 +-- .../storage/pvc/test_kserve_pvc_rwx.py | 4 +-- 8 files changed, 62 insertions(+), 37 deletions(-) create mode 100644 tests/constants.py delete mode 100644 tests/model_serving/constants.py diff --git a/tests/conftest.py b/tests/conftest.py index b6f4fb3..7f0cc1a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -63,7 +63,7 @@ def ci_s3_bucket_region(pytestconfig: pytest.Config) -> str: ci_bucket_region = pytestconfig.option.ci_s3_bucket_region if not ci_bucket_region: raise ValueError( - "Bucket name for the models bucket is not defined." + "Region for the ci s3 bucket is not defined." "Either pass with `--ci-s3-bucket-region` or set `CI_S3_BUCKET_REGION` environment variable" ) return ci_bucket_region @@ -74,7 +74,7 @@ def ci_s3_bucket_endpoint(pytestconfig: pytest.Config) -> str: ci_bucket_endpoint = pytestconfig.option.ci_s3_bucket_endpoint if not ci_bucket_endpoint: raise ValueError( - "Bucket name for the models bucket is not defined." + "Endpoint for the ci s3 bucket is not defined." "Either pass with `--ci-s3-bucket-endpoint` or set `CI_S3_BUCKET_ENDPOINT` environment variable" ) return ci_bucket_endpoint @@ -96,7 +96,7 @@ def models_s3_bucket_region(pytestconfig: pytest.Config) -> str: models_bucket_region = pytestconfig.option.models_s3_bucket_region if not models_bucket_region: raise ValueError( - "Bucket name for the models bucket is not defined." + "region for the models bucket is not defined." "Either pass with `--models-s3-bucket-region` or set `MODELS_S3_BUCKET_REGION` environment variable" ) return models_bucket_region @@ -107,7 +107,7 @@ def models_s3_bucket_endpoint(pytestconfig: pytest.Config) -> str: models_bucket_endpoint = pytestconfig.option.models_s3_bucket_endpoint if not models_bucket_endpoint: raise ValueError( - "Bucket name for the models bucket is not defined." + "endpoint for the models bucket is not defined." "Either pass with `--models-s3-bucket-endpoint` or set `MODELS_S3_BUCKET_ENDPOINT` environment variable" ) return models_bucket_endpoint diff --git a/tests/constants.py b/tests/constants.py new file mode 100644 index 0000000..90e6eb5 --- /dev/null +++ b/tests/constants.py @@ -0,0 +1,22 @@ +from enum import Enum + + +class KServeDeploymentType(Enum): + SERVERLESS: str = "Serverless" + RAW_DEPLOYMENT: str = "RawDeployment" + + +class ModelStoragePath(Enum): + FLAN_T5_SMALL: str = "flan-t5-small/flan-t5-small-caikit" + + +class ModelFormat(Enum): + CAIKIT: str = "caikit" + + +class CurlOutput(Enum): + HEALTH_OK: str = "OK" + + +class ModelEndpoint(Enum): + HEALTH: str = "health" diff --git a/tests/model_serving/constants.py b/tests/model_serving/constants.py deleted file mode 100644 index 2b07f37..0000000 --- a/tests/model_serving/constants.py +++ /dev/null @@ -1,2 +0,0 @@ -KSERVE_SERVERLESS: str = "Serverless" -KSERVE_RAWDEPLOYMENT: str = "RawDeployment" diff --git a/tests/model_serving/model_server/private_endpoint/conftest.py b/tests/model_serving/model_server/private_endpoint/conftest.py index 79b504a..c6cb1a2 100644 --- a/tests/model_serving/model_server/private_endpoint/conftest.py +++ b/tests/model_serving/model_server/private_endpoint/conftest.py @@ -13,11 +13,11 @@ from tests.model_serving.model_server.utils import create_isvc from tests.model_serving.model_server.private_endpoint.utils import ( create_sidecar_pod, - get_flan_deployment, + get_kserve_predictor_deployment, b64_encoded_string, ) from utilities.infra import create_ns -from tests.model_serving.constants import KSERVE_SERVERLESS +from tests.constants import KServeDeploymentType, ModelStoragePath, ModelFormat LOGGER = get_logger(name=__name__) @@ -88,10 +88,10 @@ def endpoint_isvc( client=admin_client, name="test", namespace=endpoint_namespace.name, - deployment_mode=KSERVE_SERVERLESS, + deployment_mode=KServeDeploymentType.SERVERLESS.value, storage_key="endpoint-s3-secret", - storage_path="flan-t5-small/flan-t5-small-caikit", - model_format="caikit", + storage_path=ModelStoragePath.FLAN_T5_SMALL.value, + model_format=ModelFormat.CAIKIT.value, runtime=endpoint_sr.name, ) as isvc: yield isvc @@ -135,7 +135,7 @@ def endpoint_pod_with_istio_sidecar( with create_sidecar_pod( admin_client=admin_client, namespace=endpoint_namespace.name, - istio=True, + use_istio=True, pod_name="test-with-istio", ) as pod: yield pod @@ -148,7 +148,7 @@ def endpoint_pod_without_istio_sidecar( with create_sidecar_pod( admin_client=admin_client, namespace=endpoint_namespace.name, - istio=False, + use_istio=False, pod_name="test", ) as pod: yield pod @@ -162,7 +162,7 @@ def diff_pod_with_istio_sidecar( with create_sidecar_pod( admin_client=admin_client, namespace=diff_namespace.name, - istio=True, + use_istio=True, pod_name="test-with-istio", ) as pod: yield pod @@ -176,7 +176,7 @@ def diff_pod_without_istio_sidecar( with create_sidecar_pod( admin_client=admin_client, namespace=diff_namespace.name, - istio=False, + use_istio=False, pod_name="test", ) as pod: yield pod @@ -184,7 +184,7 @@ def diff_pod_without_istio_sidecar( @pytest.fixture() def ready_predictor(admin_client: DynamicClient, endpoint_isvc: InferenceService) -> None: - get_flan_deployment( + get_kserve_predictor_deployment( namespace=endpoint_isvc.namespace, client=admin_client, name_prefix=endpoint_isvc.name, diff --git a/tests/model_serving/model_server/private_endpoint/test_kserve_private_endpoint.py b/tests/model_serving/model_server/private_endpoint/test_kserve_private_endpoint.py index 78fcfb6..c17f32f 100644 --- a/tests/model_serving/model_server/private_endpoint/test_kserve_private_endpoint.py +++ b/tests/model_serving/model_server/private_endpoint/test_kserve_private_endpoint.py @@ -6,20 +6,25 @@ from ocp_resources.pod import Pod from ocp_resources.deployment import Deployment from tests.model_serving.model_server.private_endpoint.utils import curl_from_pod +from tests.constants import CurlOutput, ModelEndpoint LOGGER = get_logger(name=__name__) class TestKserveInternalEndpoint: + "Tests the internal endpoint of a kserve predictor" + def test_deploy_model_state_loaded( self: Self, endpoint_namespace: Namespace, endpoint_isvc: InferenceService, ready_predictor: Deployment ) -> None: + "Verifies that the predictor gets to state Loaded" assert endpoint_isvc.instance.status.modelStatus.states.activeModelState == "Loaded" def test_deploy_model_url( self: Self, endpoint_namespace: Namespace, endpoint_isvc: InferenceService, ready_predictor: Deployment ) -> None: + "Verifies that the internal endpoint has the expected formatting" assert ( endpoint_isvc.instance.status.address.url == f"https://{endpoint_isvc.name}.{endpoint_namespace.name}.svc.cluster.local" @@ -30,56 +35,56 @@ def test_curl_with_istio_same_ns( endpoint_isvc: InferenceService, endpoint_pod_with_istio_sidecar: Pod, ) -> None: - LOGGER.info("Testing curl from the same namespace with a pod part of the service mesh") + "Verifies the response from the health endpoint, sending a request from a pod in the same ns and part of the Istio Service Mesh" curl_stdout = curl_from_pod( isvc=endpoint_isvc, pod=endpoint_pod_with_istio_sidecar, - endpoint="health", + endpoint=ModelEndpoint.HEALTH.value, ) - assert curl_stdout == "OK" + assert curl_stdout == CurlOutput.HEALTH_OK.value def test_curl_with_istio_diff_ns( self: Self, endpoint_isvc: InferenceService, diff_pod_with_istio_sidecar: Pod, ) -> None: - LOGGER.info("Testing curl from a different namespace with a pod part of the service mesh") + "Verifies the response from the health endpoint, sending a request from a pod in a different ns and part of the Istio Service Mesh" curl_stdout = curl_from_pod( isvc=endpoint_isvc, pod=diff_pod_with_istio_sidecar, - endpoint="health", + endpoint=ModelEndpoint.HEALTH.value, protocol="https", ) - assert curl_stdout == "OK" + assert curl_stdout == CurlOutput.HEALTH_OK.value def test_curl_outside_istio_same_ns( self: Self, endpoint_isvc: InferenceService, endpoint_pod_without_istio_sidecar: Pod, ) -> None: - LOGGER.info("Testing curl from the same namespace with a pod not part of the service mesh") + "Verifies the response from the health endpoint, sending a request from a pod in the same ns and not part of the Istio Service Mesh" curl_stdout = curl_from_pod( isvc=endpoint_isvc, pod=endpoint_pod_without_istio_sidecar, - endpoint="health", + endpoint=ModelEndpoint.HEALTH.value, protocol="https", ) - assert curl_stdout == "OK" + assert curl_stdout == CurlOutput.HEALTH_OK.value def test_curl_outside_istio_diff_ns( self: Self, endpoint_isvc: InferenceService, diff_pod_without_istio_sidecar: Pod, ) -> None: - LOGGER.info("Testing curl from a different namespace with a pod not part of the service mesh") + "Verifies the response from the health endpoint, sending a request from a pod in a different ns and not part of the Istio Service Mesh" curl_stdout = curl_from_pod( isvc=endpoint_isvc, pod=diff_pod_without_istio_sidecar, - endpoint="health", + endpoint=ModelEndpoint.HEALTH.value, protocol="https", ) - assert curl_stdout == "OK" + assert curl_stdout == CurlOutput.HEALTH_OK.value diff --git a/tests/model_serving/model_server/private_endpoint/utils.py b/tests/model_serving/model_server/private_endpoint/utils.py index ca6a84f..c6d573c 100644 --- a/tests/model_serving/model_server/private_endpoint/utils.py +++ b/tests/model_serving/model_server/private_endpoint/utils.py @@ -47,7 +47,7 @@ def __str__(self) -> str: return msg -def get_flan_deployment(client: DynamicClient, namespace: str, name_prefix: str) -> Deployment: +def get_kserve_predictor_deployment(client: DynamicClient, namespace: str, name_prefix: str) -> Deployment: deployments = list( Deployment.get( label_selector=f"serving.kserve.io/inferenceservice={name_prefix}", @@ -62,9 +62,9 @@ def get_flan_deployment(client: DynamicClient, namespace: str, name_prefix: str) deployment.wait_for_replicas() return deployment elif len(deployments) > 1: - raise ResourceNotUniqueError(f"Multiple flan predictor deployments found in namespace {namespace}") + raise ResourceNotUniqueError(f"Multiple predictor deployments found in namespace {namespace}") else: - raise ResourceNotFoundError(f"Flan predictor deployment not found in namespace {namespace}") + raise ResourceNotFoundError(f"Predictor deployment not found in namespace {namespace}") def curl_from_pod( @@ -86,11 +86,11 @@ def curl_from_pod( def create_sidecar_pod( admin_client: DynamicClient, namespace: str, - istio: bool, + use_istio: bool, pod_name: str, ) -> Generator[Pod, Any, Any]: cmd = f"oc run {pod_name} -n {namespace} --image=registry.access.redhat.com/rhel7/rhel-tools" - if istio: + if use_istio: cmd = f'{cmd} --annotations=sidecar.istio.io/inject="true"' cmd += " -- sleep infinity" diff --git a/tests/model_serving/model_server/storage/pvc/conftest.py b/tests/model_serving/model_server/storage/pvc/conftest.py index becbe5b..8578158 100644 --- a/tests/model_serving/model_server/storage/pvc/conftest.py +++ b/tests/model_serving/model_server/storage/pvc/conftest.py @@ -17,7 +17,7 @@ from tests.model_serving.model_server.storage.constants import NFS_STR from tests.model_serving.model_server.utils import create_isvc -from tests.model_serving.constants import KSERVE_SERVERLESS +from tests.constants import KServeDeploymentType from utilities.serving_runtime import ServingRuntimeFromTemplate @@ -125,7 +125,7 @@ def inference_service( "runtime": serving_runtime.name, "storage_uri": f"pvc://{model_pvc.name}/{downloaded_model_data}", "model_format": serving_runtime.instance.spec.supportedModelFormats[0].name, - "deployment_mode": request.param.get("deployment-mode", KSERVE_SERVERLESS), + "deployment_mode": request.param.get("deployment-mode", KServeDeploymentType.SERVERLESS.value), } if min_replicas := request.param.get("min-replicas"): diff --git a/tests/model_serving/model_server/storage/pvc/test_kserve_pvc_rwx.py b/tests/model_serving/model_server/storage/pvc/test_kserve_pvc_rwx.py index d3f6f78..38f522c 100644 --- a/tests/model_serving/model_server/storage/pvc/test_kserve_pvc_rwx.py +++ b/tests/model_serving/model_server/storage/pvc/test_kserve_pvc_rwx.py @@ -1,6 +1,6 @@ import shlex from typing import List -from tests.model_serving.constants import KSERVE_SERVERLESS +from tests.constants import KServeDeploymentType import pytest @@ -25,7 +25,7 @@ {"model-dir": "test-dir"}, {"access-modes": "ReadWriteMany", "storage-class-name": NFS_STR}, KSERVE_OVMS_SERVING_RUNTIME_PARAMS, - INFERENCE_SERVICE_PARAMS | {"deployment-mode": KSERVE_SERVERLESS, "min-replicas": 2}, + INFERENCE_SERVICE_PARAMS | {"deployment-mode": KServeDeploymentType.SERVERLESS.value, "min-replicas": 2}, ) ], indirect=True,