test: add REST and gRPC test suite for Seldon MLServer runtime#328
test: add REST and gRPC test suite for Seldon MLServer runtime#328dbasunag merged 14 commits intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces comprehensive support for MLServer-based model serving tests. It adds new runtime templates, constants, fixtures, utility functions, and protocol buffer definitions to enable and validate MLServer deployments using both REST and gRPC protocols. Parameterized tests and snapshot files are provided for sklearn, XGBoost, LightGBM, CatBoost, MLflow, and HuggingFace model inference scenarios. Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/verified', '/hold', '/lgtm'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
tests/conftest.py (1)
239-245: LGTM! Consider adding a docstring for better documentation.The
mlserver_runtime_imagefixture follows the exact same pattern as the existingvllm_runtime_imagefixture and integrates well with the pytest infrastructure.Consider adding a docstring to improve documentation:
@pytest.fixture(scope="session") def mlserver_runtime_image(pytestconfig: pytest.Config) -> str | None: + """ + Retrieves the MLServer runtime image from pytest configuration. + + Returns: + str | None: The MLServer runtime image if configured, None otherwise. + """ runtime_image = pytestconfig.option.mlserver_runtime_image if not runtime_image: return None return runtime_image🧰 Tools
🪛 Pylint (3.3.7)
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/constant.py (2)
1-1: Add module docstring for better documentation.The module would benefit from a docstring explaining its purpose and the constants it defines.
+""" +Constants module for MLServer model serving tests. + +This module defines configuration values, resource specifications, deployment +configurations, and input queries used across MLServer runtime tests. +""" + import os🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
37-49: Comprehensive resource configuration with minor formatting issues.The PREDICT_RESOURCES configuration is well-structured and covers all necessary Kubernetes resource specifications. Consider splitting the long type annotation for better readability.
-PREDICT_RESOURCES: dict[str, Union[list[dict[str, Union[str, dict[str, str]]]], dict[str, dict[str, str]]]] = { +PREDICT_RESOURCES: dict[ + str, Union[list[dict[str, Union[str, dict[str, str]]]], dict[str, dict[str, str]]] +] = {🧰 Tools
🪛 Pylint (3.3.7)
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
tests/model_serving/model_runtime/mlserver/utils.py (3)
1-1: Add module docstring and fix import order.The module needs documentation explaining its purpose and utility functions for MLServer testing.
+""" +Utility functions for MLServer model runtime testing. + +This module provides functions for managing Kubernetes secrets, sending inference +requests over REST and gRPC protocols, and validating model responses in test +environments supporting both raw and serverless deployment modes. +""" + +import json +import os +import subprocess +from contextlib import contextmanager +from typing import Any, Generator + -import os -import json import requests import portforward -import subprocess - -from contextlib import contextmanager -from typing import Generator, Any🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
51-54: Add function docstring for clarity.The REST request function would benefit from documentation.
def send_rest_request(url: str, input_data: dict) -> Any: + """Send a REST inference request and return the JSON response.""" response = requests.post(url=url, json=input_data, verify=False, timeout=60)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 51-51: Missing function or method docstring
(C0116)
83-119: Comprehensive inference function handling multiple deployment modes.The function correctly handles both RAW_DEPLOYMENT and SERVERLESS modes with appropriate port forwarding and URL construction for each protocol.
Consider simplifying the conditional structure for better readability:
elif deployment_mode == KServeDeploymentType.SERVERLESS: base_url = isvc.instance.status.url.rstrip("/") if is_rest: return send_rest_request(f"{base_url}{rest_endpoint}", input_data) - else: - grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) - return send_grpc_request(grpc_url, input_data, root_dir, insecure=True) + + grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) + return send_grpc_request(grpc_url, input_data, root_dir, insecure=True)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 84-84: Line too long (109/100)
(C0301)
[convention] 103-103: Line too long (114/100)
(C0301)
[convention] 104-104: Line too long (110/100)
(C0301)
[refactor] 83-83: Too many arguments (6/5)
(R0913)
[refactor] 83-83: Too many positional arguments (6/5)
(R0917)
[refactor] 113-117: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
tests/model_serving/model_runtime/mlserver/conftest.py (1)
1-1: Add module docstring for test fixture documentation.The module needs documentation explaining the pytest fixtures for MLServer testing.
+""" +Pytest fixtures for MLServer model runtime testing. + +This module provides fixtures for setting up MLServer serving runtimes, inference +services, Kubernetes resources, and snapshot testing infrastructure to support +comprehensive testing of model serving across different protocols and deployment modes. +""" + from typing import Any, Generator🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
utilities/manifests/common/grpc_predict_v2.proto (2)
15-16: Add Buf lint ignore directive to suppress PACKAGE_DIRECTORY_MATCH.This file is intentionally using the standardized
package inference;from the KServe V2 spec and lives outside aninference/folder. To prevent Buf lint failures, add a lint-ignore directive above thesyntaxdeclaration.@@ -14,0 +15 @@ +// buf:lint:ignore PACKAGE_DIRECTORY_MATCH🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
31-39: Clarify error-handling comments for gRPC status semantics.The comments on
ServerMetadataandModelMetadatareferencegoogle.rpc.Statusreturned for the request, which may be misleading since errors in gRPC are surfaced via error codes rather than as a protobuf message in the response. Consider updating the comments to reflect that failures are returned as gRPC error statuses, or explicitly import and usegoogle.rpc.Statusin the service definitions if that’s the intended behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.206Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (2)
tests/model_serving/model_runtime/mlserver/constant.py (1)
utilities/constants.py (3)
KServeDeploymentType(6-9)Protocols(89-96)RuntimeTemplates(65-75)
tests/model_serving/model_runtime/mlserver/utils.py (3)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (1)
admin_client(53-54)tests/model_serving/model_runtime/mlserver/conftest.py (2)
root_dir(43-44)protocol(70-71)
🪛 Pylint (3.3.7)
tests/conftest.py
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (109/100)
(C0301)
[convention] 115-115: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 97-97: Too many arguments (6/5)
(R0913)
[refactor] 97-97: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-8: Unable to import 'utilities.constants'
(E0401)
tests/model_serving/model_runtime/mlserver/utils.py
[convention] 13-13: Line too long (102/100)
(C0301)
[convention] 84-84: Line too long (109/100)
(C0301)
[convention] 103-103: Line too long (114/100)
(C0301)
[convention] 104-104: Line too long (110/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-4: Unable to import 'portforward'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.secret'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 12-12: Unable to import 'utilities.constants'
(E0401)
[error] 13-13: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 14-18: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[convention] 22-22: Missing function or method docstring
(C0116)
[refactor] 22-22: Too many arguments (7/5)
(R0913)
[refactor] 22-22: Too many positional arguments (7/5)
(R0917)
[convention] 51-51: Missing function or method docstring
(C0116)
[convention] 57-57: Missing function or method docstring
(C0116)
[refactor] 83-83: Too many arguments (6/5)
(R0913)
[refactor] 83-83: Too many positional arguments (6/5)
(R0917)
[refactor] 113-117: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[convention] 122-122: Missing function or method docstring
(C0116)
[convention] 126-126: Missing function or method docstring
(C0116)
[refactor] 126-126: Too many arguments (7/5)
(R0913)
[refactor] 126-126: Too many positional arguments (7/5)
(R0917)
[convention] 5-5: standard import "subprocess" should be placed before third party imports "requests", "portforward"
(C0411)
[convention] 7-7: standard import "contextlib.contextmanager" should be placed before third party imports "requests", "portforward"
(C0411)
[convention] 8-8: standard import "typing.Generator" should be placed before third party imports "requests", "portforward"
(C0411)
tests/model_serving/model_runtime/mlserver/conftest.py
[convention] 142-142: Line too long (108/100)
(C0301)
[convention] 179-179: Line too long (108/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'syrupy.extensions.json'
(E0401)
[error] 7-7: Unable to import 'pytest_testconfig'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.serving_runtime'
(E0401)
[error] 12-12: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.pod'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.secret'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.template'
(E0401)
[error] 16-16: Unable to import 'ocp_resources.service_account'
(E0401)
[error] 18-23: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 24-24: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[error] 26-31: Unable to import 'utilities.constants'
(E0401)
[error] 32-32: Unable to import 'utilities.inference_utils'
(E0401)
[error] 33-33: Unable to import 'utilities.infra'
(E0401)
[error] 34-34: Unable to import 'utilities.serving_runtime'
(E0401)
[error] 36-36: Unable to import 'simple_logger.logger'
(E0401)
[convention] 43-43: Missing function or method docstring
(C0116)
[convention] 48-48: Missing function or method docstring
(C0116)
[convention] 59-59: Missing function or method docstring
(C0116)
[convention] 70-70: Missing function or method docstring
(C0116)
[convention] 75-75: Missing function or method docstring
(C0116)
[warning] 80-80: Redefining name 'protocol' from outer scope (line 70)
(W0621)
[refactor] 95-95: Too many arguments (6/5)
(R0913)
[refactor] 95-95: Too many positional arguments (6/5)
(R0917)
[warning] 99-99: Redefining name 'mlserver_serving_runtime' from outer scope (line 75)
(W0621)
[warning] 101-101: Redefining name 'mlserver_model_service_account' from outer scope (line 142)
(W0621)
[convention] 142-142: Missing function or method docstring
(C0116)
[warning] 142-142: Redefining name 'kserve_s3_secret' from outer scope (line 153)
(W0621)
[convention] 153-153: Missing function or method docstring
(C0116)
[refactor] 153-153: Too many arguments (6/5)
(R0913)
[refactor] 153-153: Too many positional arguments (6/5)
(R0917)
[convention] 174-174: Missing function or method docstring
(C0116)
[convention] 179-179: Missing function or method docstring
(C0116)
[warning] 179-179: Redefining name 'mlserver_inference_service' from outer scope (line 95)
(W0621)
[convention] 36-36: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (19)
conftest.py (1)
105-109: LGTM! Consistent implementation following established patterns.The new
--mlserver-runtime-imageoption is well-implemented, following the exact same pattern as the existing--vllm-runtime-imageoption. The integration with the runtime group, environment variable fallback, and help text are all consistent with the codebase conventions.utilities/constants.py (1)
74-75: LGTM! Well-designed constants following established conventions.The new MLServer runtime template constants are properly named and follow the established patterns in the
RuntimeTemplatesclass. The naming clearly indicates the protocol (GRPC/REST) and uses consistent kebab-case for the template identifiers.tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json (1)
1-23: LGTM! Well-structured test snapshot for sklearn model inference.The JSON snapshot correctly captures the expected MLServer inference response format for the sklearn iris model. The structure includes:
- Proper model identification (sklearn-iris v1.0.0)
- Correct datatype (INT64) and shape [2,1] for classification output
- Realistic prediction values [1,1] for iris classification
- Appropriate content type ("np" for NumPy arrays)
This snapshot will provide reliable validation for the MLServer REST inference tests.
tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json (1)
1-23: Snapshot content looks correct for REST inference.The JSON structure aligns with MLServer V2 REST response schema:
- Uses
id,model_name,model_version.- Includes
outputsarray withdata,datatype,name,parameters.content_type, and numericshape.
Everything matches the expected inference output for the sklearn-iris model.tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json (1)
1-25: Snapshot structure is valid for gRPC raw inference.The JSON keys follow the gRPC-to-JSON mapping:
modelName/modelVersionin camelCase,outputs.contents.int64Contentsfor the data,parameters.stringParamfor content_type,datatypeand stringifiedshapevalues.
This matches the expected protobuf serialization.tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json (1)
1-25: Snapshot structure is valid for serverless gRPC inference.Identical to the raw-gRPC snapshot in structure and content, correctly capturing the serverless deployment output.
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
1-6: Excellent test module documentation and structure.The module docstring clearly explains the purpose and scope of the tests. The comprehensive coverage of different protocols and deployment types is well-planned.
44-87: Well-structured parameterized test cases.The parameterization effectively covers all combinations of protocols (REST/gRPC) and deployment types (raw/serverless), providing comprehensive test coverage for the MLServer runtime.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 45-45: Line too long (109/100)
(C0301)
97-125: Clean test implementation with proper validation.The test method correctly uses the utility function for inference validation and follows pytest conventions. The input query selection based on protocol is appropriate.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 115-115: Line too long (106/100)
(C0301)
[refactor] 97-97: Too many arguments (6/5)
(R0913)
[refactor] 97-97: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/constant.py (1)
63-86: Well-defined input queries for both protocols.The sklearn input queries are properly structured for both REST and gRPC protocols, with appropriate data types and shapes for the iris model.
tests/model_serving/model_runtime/mlserver/utils.py (3)
21-48: Well-implemented S3 secret management.The context manager correctly creates and manages Kubernetes secrets with proper S3 annotations for KServe integration.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 22-22: Missing function or method docstring
(C0116)
[refactor] 22-22: Too many arguments (7/5)
(R0913)
[refactor] 22-22: Too many positional arguments (7/5)
(R0917)
57-80: Robust gRPC request implementation with good error handling.The function correctly uses grpcurl and handles subprocess errors appropriately. The error handling returns a descriptive string which is good for debugging.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 57-57: Missing function or method docstring
(C0116)
126-143: Effective inference validation with snapshot testing.The validation function correctly calls the inference function and compares results against snapshots, providing clear error messages for debugging test failures.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 126-126: Missing function or method docstring
(C0116)
[refactor] 126-126: Too many arguments (7/5)
(R0913)
[refactor] 126-126: Too many positional arguments (7/5)
(R0917)
tests/model_serving/model_runtime/mlserver/conftest.py (5)
47-66: Well-designed template fixtures for both protocols.The fixtures correctly load and manage OpenShift templates for both gRPC and REST serving runtimes, with proper scoping and resource management.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 48-48: Missing function or method docstring
(C0116)
[convention] 59-59: Missing function or method docstring
(C0116)
74-91: Flexible serving runtime fixture with proper parameterization.The fixture correctly creates serving runtimes based on protocol and deployment type parameters, with appropriate template and runtime name mapping.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 75-75: Missing function or method docstring
(C0116)
[warning] 80-80: Redefining name 'protocol' from outer scope (line 70)
(W0621)
94-138: Comprehensive inference service configuration.The fixture handles complex inference service configuration including GPU resources, volumes, timeouts, and replica settings. The conditional resource allocation based on GPU count is well-implemented.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 95-95: Too many arguments (6/5)
(R0913)
[refactor] 95-95: Too many positional arguments (6/5)
(R0917)
[warning] 99-99: Redefining name 'mlserver_serving_runtime' from outer scope (line 75)
(W0621)
[warning] 101-101: Redefining name 'mlserver_model_service_account' from outer scope (line 142)
(W0621)
141-170: Proper secret and service account management.The fixtures correctly create and link service accounts with S3 secrets, following Kubernetes best practices for secure model storage access.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 142-142: Line too long (108/100)
(C0301)
[convention] 142-142: Missing function or method docstring
(C0116)
[warning] 142-142: Redefining name 'kserve_s3_secret' from outer scope (line 153)
(W0621)
[convention] 153-153: Missing function or method docstring
(C0116)
[refactor] 153-153: Too many arguments (6/5)
(R0913)
[refactor] 153-153: Too many positional arguments (6/5)
(R0917)
173-183: Effective snapshot testing and pod retrieval utilities.The fixtures provide essential utilities for snapshot-based response validation and reliable pod retrieval with appropriate error handling.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 179-179: Line too long (108/100)
(C0301)
[convention] 174-174: Missing function or method docstring
(C0116)
[convention] 179-179: Missing function or method docstring
(C0116)
[warning] 179-179: Redefining name 'mlserver_inference_service' from outer scope (line 95)
(W0621)
utilities/manifests/common/grpc_predict_v2.proto (1)
16-16: Retain the standardized package declaration.As per the official KServe V2 inference protocol, the
package inference;must remain unchanged. This aligns with the retrieved learning that this file is a reference implementation and should not be modified.🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
Show resolved
Hide resolved
tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
Show resolved
Hide resolved
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
3862db3 to
d83b69d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
39-41: Fix line length violations.The
pytestmarkdeclaration and parametrize decorator exceed the 100-character line limit.Apply this diff to fix line length issues:
pytestmark = pytest.mark.usefixtures( - "root_dir", "valid_aws_config", "mlserver_rest_serving_runtime_template", "mlserver_grpc_serving_runtime_template" + "root_dir", + "valid_aws_config", + "mlserver_rest_serving_runtime_template", + "mlserver_grpc_serving_runtime_template" )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 40-40: Line too long (118/100)
(C0301)
44-46: Fix line length violation in parametrize decorator.The parametrize decorator line exceeds the 100-character limit.
Apply this diff to improve readability:
@pytest.mark.parametrize( - "protocol, model_namespace, mlserver_inference_service, s3_models_storage_uri, mlserver_serving_runtime", + ( + "protocol, model_namespace, mlserver_inference_service, " + "s3_models_storage_uri, mlserver_serving_runtime" + ), [🧰 Tools
🪛 Pylint (3.3.7)
[convention] 45-45: Line too long (109/100)
(C0301)
115-115: Fix line length violation.The conditional assignment line is too long.
Apply this diff to improve readability:
- input_query = SKLEARN_REST_INPUT_QUERY if protocol == Protocols.REST else SKLEARN_GRPC_INPUT_QUERY + input_query = ( + SKLEARN_REST_INPUT_QUERY + if protocol == Protocols.REST + else SKLEARN_GRPC_INPUT_QUERY + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 115-115: Line too long (106/100)
(C0301)
tests/model_serving/model_runtime/mlserver/constant.py (2)
1-1: Add module docstring for better documentation.The module lacks a docstring explaining its purpose and contents.
Apply this diff to add proper module documentation:
+""" +Constants for MLServer model serving tests. + +This module defines configuration values, resource specifications, deployment +configurations, and example input queries used across MLServer runtime tests. +""" + import os🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
37-49: Fix line length violations in resource specifications.The type annotation and resources dictionary lines exceed the 100-character limit.
Apply this diff to improve readability:
-PREDICT_RESOURCES: dict[str, Union[list[dict[str, Union[str, dict[str, str]]]], dict[str, dict[str, str]]]] = { +PREDICT_RESOURCES: dict[ + str, Union[list[dict[str, Union[str, dict[str, str]]]], dict[str, dict[str, str]]] +] = { "volumes": [ {"name": "shared-memory", "emptyDir": {"medium": "Memory", "sizeLimit": "2Gi"}}, {"name": "tmp", "emptyDir": {}}, {"name": "home", "emptyDir": {}}, ], "volume_mounts": [ {"name": "shared-memory", "mountPath": "/dev/shm"}, {"name": "tmp", "mountPath": "/tmp"}, {"name": "home", "mountPath": "/home/mlserver"}, ], - "resources": {"requests": {"cpu": "1", "memory": "2Gi"}, "limits": {"cpu": "2", "memory": "4Gi"}}, + "resources": { + "requests": {"cpu": "1", "memory": "2Gi"}, + "limits": {"cpu": "2", "memory": "4Gi"} + }, }🧰 Tools
🪛 Pylint (3.3.7)
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
tests/model_serving/model_runtime/mlserver/utils.py (4)
21-48: Add function docstring for the S3 secret context manager.The context manager lacks documentation explaining its purpose and parameters.
Apply this diff to add proper documentation:
@contextmanager def kserve_s3_endpoint_secret( admin_client: DynamicClient, name: str, namespace: str, aws_access_key: str, aws_secret_access_key: str, aws_s3_endpoint: str, aws_s3_region: str, ) -> Generator[Secret, Any, Any]: + """ + Create and manage a Kubernetes Secret for S3 access with KServe annotations. + + Args: + admin_client: Kubernetes dynamic client + name: Secret name + namespace: Target namespace + aws_access_key: AWS access key ID + aws_secret_access_key: AWS secret access key + aws_s3_endpoint: S3 endpoint URL + aws_s3_region: AWS region + + Yields: + Secret: The created Kubernetes Secret resource + """ with Secret(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 22-22: Missing function or method docstring
(C0116)
[refactor] 22-22: Too many arguments (7/5)
(R0913)
[refactor] 22-22: Too many positional arguments (7/5)
(R0917)
51-54: Add docstring for REST request function.The function lacks documentation.
def send_rest_request(url: str, input_data: dict) -> Any: + """Send a REST inference request and return the JSON response.""" response = requests.post(url=url, json=input_data, verify=False, timeout=60)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 51-51: Missing function or method docstring
(C0116)
57-81: Add docstring and fix line length for gRPC request function.The function needs documentation and has line length issues.
Apply this diff:
def send_grpc_request(url: str, input_data: dict, root_dir: str, insecure: bool = False) -> Any: + """ + Send a gRPC inference request using grpcurl and return the JSON response. + + Args: + url: gRPC endpoint URL + input_data: Request payload as dictionary + root_dir: Root directory for proto file resolution + insecure: Whether to use insecure connection + + Returns: + Parsed JSON response or error message + """ grpc_proto_path = os.path.join(root_dir, PROTO_FILE_PATH)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 57-57: Missing function or method docstring
(C0116)
122-143: Add docstrings for helper functions.Both utility functions lack documentation.
Apply this diff to add proper documentation:
def get_grpc_url(base_url: str, port: int) -> str: + """Format a gRPC URL by removing protocol prefix and adding port.""" return f"{base_url.replace('https://', '').replace('http://', '')}:{port}" def validate_inference_request( pod_name: str, isvc: InferenceService, response_snapshot: Any, input_query: Any, model_version: str, protocol: str, root_dir: str, ) -> None: + """ + Validate an inference request by comparing the response with a snapshot. + + Args: + pod_name: Name of the pod running the model server + isvc: InferenceService resource + response_snapshot: Expected response for validation + input_query: Input data for the inference request + model_version: Model version to use for inference + protocol: Communication protocol (REST or gRPC) + root_dir: Root directory for file resolution + + Raises: + AssertionError: If the response doesn't match the snapshot + """ response = run_mlserver_inference(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 122-122: Missing function or method docstring
(C0116)
[convention] 126-126: Missing function or method docstring
(C0116)
[refactor] 126-126: Too many arguments (7/5)
(R0913)
[refactor] 126-126: Too many positional arguments (7/5)
(R0917)
tests/model_serving/model_runtime/mlserver/conftest.py (4)
1-37: Fix import organization to follow PEP 8 conventions.The
simple_loggerimport should be placed with other third-party imports before the first-party imports.from ocp_resources.service_account import ServiceAccount +from simple_logger.logger import get_logger + from tests.model_serving.model_runtime.mlserver.constant import ( PREDICT_RESOURCES, RUNTIME_MAP, TEMPLATE_MAP, TEMPLATE_FILE_PATH, ) from tests.model_serving.model_runtime.mlserver.utils import kserve_s3_endpoint_secret from utilities.constants import ( KServeDeploymentType, Labels, RuntimeTemplates, Protocols, ) from utilities.inference_utils import create_isvc from utilities.infra import get_pods_by_isvc_label from utilities.serving_runtime import ServingRuntimeFromTemplate -from simple_logger.logger import get_logger🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'syrupy.extensions.json'
(E0401)
[error] 7-7: Unable to import 'pytest_testconfig'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.serving_runtime'
(E0401)
[error] 12-12: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.pod'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.secret'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.template'
(E0401)
[error] 16-16: Unable to import 'ocp_resources.service_account'
(E0401)
[error] 18-23: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 24-24: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[error] 26-31: Unable to import 'utilities.constants'
(E0401)
[error] 32-32: Unable to import 'utilities.inference_utils'
(E0401)
[error] 33-33: Unable to import 'utilities.infra'
(E0401)
[error] 34-34: Unable to import 'utilities.serving_runtime'
(E0401)
[error] 36-36: Unable to import 'simple_logger.logger'
(E0401)
[convention] 36-36: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
42-71: Add docstrings to improve fixture documentation.These utility fixtures would benefit from docstrings explaining their purpose and usage pattern.
@pytest.fixture(scope="session") def root_dir(pytestconfig: pytest.Config) -> Any: + """Return the root directory path from pytest configuration.""" return pytestconfig.rootpath @pytest.fixture(scope="class") def mlserver_grpc_serving_runtime_template(admin_client: DynamicClient) -> Template: + """Create and yield MLServer gRPC serving runtime template.""" grpc_template_yaml = TEMPLATE_FILE_PATH.get(Protocols.GRPC) with Template( client=admin_client, yaml_file=grpc_template_yaml, namespace=py_config["applications_namespace"], ) as tp: yield tp @pytest.fixture(scope="class") def mlserver_rest_serving_runtime_template(admin_client: DynamicClient) -> Template: + """Create and yield MLServer REST serving runtime template.""" rest_template_yaml = TEMPLATE_FILE_PATH.get(Protocols.REST) with Template( client=admin_client, yaml_file=rest_template_yaml, namespace=py_config["applications_namespace"], ) as tp: yield tp @pytest.fixture(scope="class") def protocol(request: FixtureRequest) -> str: + """Extract protocol type from test parameters.""" return request.param["protocol_type"]🧰 Tools
🪛 Pylint (3.3.7)
[convention] 43-43: Missing function or method docstring
(C0116)
[convention] 48-48: Missing function or method docstring
(C0116)
[convention] 59-59: Missing function or method docstring
(C0116)
[convention] 70-70: Missing function or method docstring
(C0116)
141-150: Fix line length issue.The fixture correctly creates a service account with S3 secret integration, but the line exceeds the 100-character limit.
@pytest.fixture(scope="class") -def mlserver_model_service_account(admin_client: DynamicClient, kserve_s3_secret: Secret) -> ServiceAccount: +def mlserver_model_service_account( + admin_client: DynamicClient, kserve_s3_secret: Secret +) -> ServiceAccount: + """Create service account with S3 secret for MLServer model access.""" with ServiceAccount( client=admin_client, namespace=kserve_s3_secret.namespace, name="mlserver-models-bucket-sa", secrets=[{"name": kserve_s3_secret.name}], ) as sa: yield sa🧰 Tools
🪛 Pylint (3.3.7)
[convention] 142-142: Line too long (108/100)
(C0301)
[convention] 142-142: Missing function or method docstring
(C0116)
[warning] 142-142: Redefining name 'kserve_s3_secret' from outer scope (line 153)
(W0621)
178-183: Fix line length and add docstring.The fixture has good error handling but needs line length correction and documentation.
@pytest.fixture -def mlserver_pod_resource(admin_client: DynamicClient, mlserver_inference_service: InferenceService) -> Pod: +def mlserver_pod_resource( + admin_client: DynamicClient, mlserver_inference_service: InferenceService +) -> Pod: + """Retrieve the pod associated with the MLServer inference service.""" pods = get_pods_by_isvc_label(client=admin_client, isvc=mlserver_inference_service) if not pods: raise RuntimeError(f"No pods found for InferenceService {mlserver_inference_service.name}") return pods[0]🧰 Tools
🪛 Pylint (3.3.7)
[convention] 179-179: Line too long (108/100)
(C0301)
[convention] 179-179: Missing function or method docstring
(C0116)
[warning] 179-179: Redefining name 'mlserver_inference_service' from outer scope (line 95)
(W0621)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
- utilities/constants.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
- conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.206Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (3)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(126-143)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(95-138)mlserver_pod_resource(179-183)mlserver_response_snapshot(174-175)protocol(70-71)root_dir(43-44)
tests/model_serving/model_runtime/mlserver/constant.py (1)
utilities/constants.py (3)
KServeDeploymentType(6-9)Protocols(89-96)RuntimeTemplates(65-75)
tests/model_serving/model_runtime/mlserver/utils.py (4)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (1)
admin_client(53-54)tests/model_serving/model_runtime/mlserver/conftest.py (2)
root_dir(43-44)protocol(70-71)tests/model_serving/model_runtime/vllm/conftest.py (1)
response_snapshot(141-142)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/conftest.py
[convention] 142-142: Line too long (108/100)
(C0301)
[convention] 179-179: Line too long (108/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'syrupy.extensions.json'
(E0401)
[error] 7-7: Unable to import 'pytest_testconfig'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.serving_runtime'
(E0401)
[error] 12-12: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.pod'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.secret'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.template'
(E0401)
[error] 16-16: Unable to import 'ocp_resources.service_account'
(E0401)
[error] 18-23: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 24-24: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[error] 26-31: Unable to import 'utilities.constants'
(E0401)
[error] 32-32: Unable to import 'utilities.inference_utils'
(E0401)
[error] 33-33: Unable to import 'utilities.infra'
(E0401)
[error] 34-34: Unable to import 'utilities.serving_runtime'
(E0401)
[error] 36-36: Unable to import 'simple_logger.logger'
(E0401)
[convention] 43-43: Missing function or method docstring
(C0116)
[convention] 48-48: Missing function or method docstring
(C0116)
[convention] 59-59: Missing function or method docstring
(C0116)
[convention] 70-70: Missing function or method docstring
(C0116)
[convention] 75-75: Missing function or method docstring
(C0116)
[warning] 80-80: Redefining name 'protocol' from outer scope (line 70)
(W0621)
[refactor] 95-95: Too many arguments (6/5)
(R0913)
[refactor] 95-95: Too many positional arguments (6/5)
(R0917)
[warning] 99-99: Redefining name 'mlserver_serving_runtime' from outer scope (line 75)
(W0621)
[warning] 101-101: Redefining name 'mlserver_model_service_account' from outer scope (line 142)
(W0621)
[convention] 142-142: Missing function or method docstring
(C0116)
[warning] 142-142: Redefining name 'kserve_s3_secret' from outer scope (line 153)
(W0621)
[convention] 153-153: Missing function or method docstring
(C0116)
[refactor] 153-153: Too many arguments (6/5)
(R0913)
[refactor] 153-153: Too many positional arguments (6/5)
(R0917)
[convention] 174-174: Missing function or method docstring
(C0116)
[convention] 179-179: Missing function or method docstring
(C0116)
[warning] 179-179: Redefining name 'mlserver_inference_service' from outer scope (line 95)
(W0621)
[convention] 36-36: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
tests/conftest.py
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (109/100)
(C0301)
[convention] 115-115: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 97-97: Too many arguments (6/5)
(R0913)
[refactor] 97-97: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-8: Unable to import 'utilities.constants'
(E0401)
tests/model_serving/model_runtime/mlserver/utils.py
[convention] 13-13: Line too long (102/100)
(C0301)
[convention] 84-84: Line too long (109/100)
(C0301)
[convention] 103-103: Line too long (114/100)
(C0301)
[convention] 104-104: Line too long (110/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-4: Unable to import 'portforward'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.secret'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 12-12: Unable to import 'utilities.constants'
(E0401)
[error] 13-13: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 14-18: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[convention] 22-22: Missing function or method docstring
(C0116)
[refactor] 22-22: Too many arguments (7/5)
(R0913)
[refactor] 22-22: Too many positional arguments (7/5)
(R0917)
[convention] 51-51: Missing function or method docstring
(C0116)
[convention] 57-57: Missing function or method docstring
(C0116)
[refactor] 83-83: Too many arguments (6/5)
(R0913)
[refactor] 83-83: Too many positional arguments (6/5)
(R0917)
[refactor] 113-117: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[convention] 122-122: Missing function or method docstring
(C0116)
[convention] 126-126: Missing function or method docstring
(C0116)
[refactor] 126-126: Too many arguments (7/5)
(R0913)
[refactor] 126-126: Too many positional arguments (7/5)
(R0917)
[convention] 5-5: standard import "subprocess" should be placed before third party imports "requests", "portforward"
(C0411)
[convention] 7-7: standard import "contextlib.contextmanager" should be placed before third party imports "requests", "portforward"
(C0411)
[convention] 8-8: standard import "typing.Generator" should be placed before third party imports "requests", "portforward"
(C0411)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (10)
tests/conftest.py (1)
239-244: LGTM! Fixture follows established patterns.The
mlserver_runtime_imagefixture implementation is consistent with the existingvllm_runtime_imagefixture above it, following the same pattern for retrieving runtime images from pytest configuration.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (2)
1-7: Excellent module documentation.The module docstring clearly explains the purpose and scope of the sklearn model deployment tests.
88-125: Well-structured test implementation.The test class and method are well-documented and follow good testing practices:
- Clear class docstring explaining test scenarios
- Comprehensive method documentation
- Proper use of fixtures and protocol-specific input selection
- Clean validation logic
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 115-115: Line too long (106/100)
(C0301)
[refactor] 97-97: Too many arguments (6/5)
(R0913)
[refactor] 97-97: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py (2)
10-35: Well-organized network and template constants.The constants for ports, URLs, and template mappings are clearly defined and properly organized. The mapping dictionaries provide good abstraction for protocol-specific configurations.
51-86: Excellent deployment configurations and input queries.The base deployment configurations clearly distinguish between raw and serverless modes, and the input queries provide comprehensive examples for both REST and gRPC protocols with proper data structure and formatting.
tests/model_serving/model_runtime/mlserver/conftest.py (4)
74-92: Well-designed serving runtime fixture.The fixture correctly handles template mapping, runtime configuration, and uses proper context management for cleanup.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 75-75: Missing function or method docstring
(C0116)
[warning] 80-80: Redefining name 'protocol' from outer scope (line 70)
(W0621)
94-138: Comprehensive inference service configuration with proper GPU handling.The fixture handles complex configuration scenarios well, including:
- GPU resource allocation and limits
- Conditional volume mounting for multi-GPU setups
- Comprehensive service configuration options
- Proper context management for cleanup
The high parameter count is justified given the comprehensive configuration this fixture manages.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 95-95: Too many arguments (6/5)
(R0913)
[refactor] 95-95: Too many positional arguments (6/5)
(R0917)
[warning] 99-99: Redefining name 'mlserver_serving_runtime' from outer scope (line 75)
(W0621)
[warning] 101-101: Redefining name 'mlserver_model_service_account' from outer scope (line 142)
(W0621)
152-171: Well-structured S3 secret configuration.The fixture properly configures S3 credentials using the utility function and context management.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 153-153: Missing function or method docstring
(C0116)
[refactor] 153-153: Too many arguments (6/5)
(R0913)
[refactor] 153-153: Too many positional arguments (6/5)
(R0917)
173-176: Useful snapshot testing configuration.The fixture correctly configures JSON snapshot extension for response validation.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 174-174: Missing function or method docstring
(C0116)
utilities/manifests/common/grpc_predict_v2.proto (1)
1-327: Approve addition of standard KServe V2 inference protocol definition.This file is the official KServe V2 inference protocol specification and should remain unchanged. The comprehensive gRPC service definition correctly includes:
- Server lifecycle methods (ServerLive, ServerReady, ModelReady)
- Metadata retrieval methods (ServerMetadata, ModelMetadata)
- Model inference method (ModelInfer)
- Complete message definitions for requests, responses, and tensor data
- Proper support for raw bytes and typed tensor contents
The Buf linting error about package directory mismatch should be ignored as this is a reference implementation that must maintain the standardized package declaration.
Based on retrieved learnings, this file should be excluded from Buf linting as it's from the KServe V2 inference protocol standard and the package declaration "inference" is standardized and should remain unchanged.
🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
d83b69d to
b783a11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/model_serving/model_runtime/mlserver/utils.py (2)
1-19: Module structure needs improvement as noted in previous reviews.The import organization and missing module docstring issues have been thoroughly addressed in previous review comments.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 13-13: Line too long (102/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-4: Unable to import 'portforward'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.secret'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 12-12: Unable to import 'utilities.constants'
(E0401)
[error] 13-13: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 14-18: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[convention] 5-5: standard import "subprocess" should be placed before third party imports "requests", "portforward"
(C0411)
[convention] 7-7: standard import "contextlib.contextmanager" should be placed before third party imports "requests", "portforward"
(C0411)
[convention] 8-8: standard import "typing.Generator" should be placed before third party imports "requests", "portforward"
(C0411)
83-119: Function structure improvements noted in previous reviews.The line length violations and structural issues in
run_mlserver_inferencehave been comprehensively addressed in previous review comments.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 84-84: Line too long (119/100)
(C0301)
[convention] 103-103: Line too long (114/100)
(C0301)
[convention] 104-104: Line too long (110/100)
(C0301)
[refactor] 83-83: Too many arguments (6/5)
(R0913)
[refactor] 83-83: Too many positional arguments (6/5)
(R0917)
[refactor] 113-117: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🧹 Nitpick comments (16)
tests/conftest.py (1)
239-244: LGTM! Consider adding a docstring for consistency.The fixture implementation correctly follows the same pattern as the existing
vllm_runtime_imagefixture. The logic and session scope are appropriate.Consider adding a docstring to match the level of documentation in other fixtures:
@pytest.fixture(scope="session") def mlserver_runtime_image(pytestconfig: pytest.Config) -> str | None: + """Return the MLServer runtime image from pytest configuration, or None if not set.""" runtime_image = pytestconfig.option.mlserver_runtime_image if not runtime_image: return None return runtime_image🧰 Tools
🪛 Pylint (3.3.7)
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (2)
44-87: Fix line length violations for better readability.The parameterization logic is excellent and covers all necessary test scenarios (REST/gRPC × raw/serverless). However, there are some line length violations that should be addressed.
Apply this diff to improve line formatting:
@pytest.mark.parametrize( - "protocol, model_namespace, mlserver_inference_service, s3_models_storage_uri, mlserver_serving_runtime", + ( + "protocol, model_namespace, mlserver_inference_service, " + "s3_models_storage_uri, mlserver_serving_runtime" + ), [ pytest.param( {"protocol_type": Protocols.REST}, {"name": "sklearn-iris-raw-rest"}, { **BASE_RAW_DEPLOYMENT_CONFIG, **MODEL_NAME_DICT, }, MODEL_STORAGE_URI_DICT, BASE_RAW_DEPLOYMENT_CONFIG, id="sklearn-iris-raw-rest-deployment", ), pytest.param( {"protocol_type": Protocols.GRPC}, {"name": "sklearn-iris-raw-grpc"}, { **BASE_RAW_DEPLOYMENT_CONFIG, **MODEL_NAME_DICT, }, MODEL_STORAGE_URI_DICT, BASE_RAW_DEPLOYMENT_CONFIG, id="sklearn-iris-raw-grpc-deployment", ), pytest.param( {"protocol_type": Protocols.REST}, {"name": "sklearn-iris-serverless-rest"}, - {**BASE_SERVERLESS_DEPLOYMENT_CONFIG, **MODEL_NAME_DICT}, + { + **BASE_SERVERLESS_DEPLOYMENT_CONFIG, + **MODEL_NAME_DICT + }, MODEL_STORAGE_URI_DICT, BASE_SERVERLESS_DEPLOYMENT_CONFIG, id="sklearn-iris-serverless-rest-deployment", ), pytest.param( {"protocol_type": Protocols.GRPC}, {"name": "sklearn-iris-serverless-grpc"}, - {**BASE_SERVERLESS_DEPLOYMENT_CONFIG, **MODEL_NAME_DICT}, + { + **BASE_SERVERLESS_DEPLOYMENT_CONFIG, + **MODEL_NAME_DICT + }, MODEL_STORAGE_URI_DICT, BASE_SERVERLESS_DEPLOYMENT_CONFIG, id="sklearn-iris-serverless-grpc-deployment", ), ], indirect=True, )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 45-45: Line too long (109/100)
(C0301)
97-125: Test method implementation is correct and well-documented.The test logic is sound:
- Proper protocol-based input selection
- Correct use of utility function for validation
- Good parameter documentation
The static analysis warnings about too many arguments are acceptable in this context since they're pytest fixtures.
Fix the line length violation on line 115:
- input_query = SKLEARN_REST_INPUT_QUERY if protocol == Protocols.REST else SKLEARN_GRPC_INPUT_QUERY + input_query = ( + SKLEARN_REST_INPUT_QUERY + if protocol == Protocols.REST + else SKLEARN_GRPC_INPUT_QUERY + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 115-115: Line too long (106/100)
(C0301)
[refactor] 97-97: Too many arguments (6/5)
(R0913)
[refactor] 97-97: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/constant.py (2)
1-9: Add module docstring to document the purpose of constants.The imports and structure look good, but the module lacks documentation.
Add a module docstring at the top:
+""" +Constants for MLServer model serving tests. + +This module defines configuration values, resource specifications, +deployment settings, and input queries used across MLServer runtime tests. +""" + import os from typing import Any, Union🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-8: Unable to import 'utilities.constants'
(E0401)
37-49: Fix line length violations in resource specifications.The resource structure is comprehensive and appropriate for MLServer deployments.
Apply this diff to fix line length issues:
-PREDICT_RESOURCES: dict[str, Union[list[dict[str, Union[str, dict[str, str]]]], dict[str, dict[str, str]]]] = { +PREDICT_RESOURCES: dict[ + str, + Union[list[dict[str, Union[str, dict[str, str]]]], dict[str, dict[str, str]]] +] = { "volumes": [ {"name": "shared-memory", "emptyDir": {"medium": "Memory", "sizeLimit": "2Gi"}}, {"name": "tmp", "emptyDir": {}}, {"name": "home", "emptyDir": {}}, ], "volume_mounts": [ {"name": "shared-memory", "mountPath": "/dev/shm"}, {"name": "tmp", "mountPath": "/tmp"}, {"name": "home", "mountPath": "/home/mlserver"}, ], - "resources": {"requests": {"cpu": "1", "memory": "2Gi"}, "limits": {"cpu": "2", "memory": "4Gi"}}, + "resources": { + "requests": {"cpu": "1", "memory": "2Gi"}, + "limits": {"cpu": "2", "memory": "4Gi"} + }, }🧰 Tools
🪛 Pylint (3.3.7)
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
tests/model_serving/model_runtime/mlserver/conftest.py (11)
1-40: Add module docstring and address import ordering.The file is missing a module docstring and has an import ordering issue.
Add a module docstring at the top:
+""" +Pytest fixtures for MLServer model serving runtime tests. + +This module provides fixtures for setting up MLServer testing infrastructure, +including serving runtimes, inference services, and related Kubernetes resources. +""" from typing import cast, Any, GeneratorAlso, move the third-party import to the correct location:
from utilities.serving_runtime import ServingRuntimeFromTemplate + +from simple_logger.logger import get_logger - -from simple_logger.logger import get_logger🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'syrupy.extensions.json'
(E0401)
[error] 7-7: Unable to import 'pytest_testconfig'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.serving_runtime'
(E0401)
[error] 12-12: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.pod'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.secret'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.template'
(E0401)
[error] 16-16: Unable to import 'ocp_resources.service_account'
(E0401)
[error] 18-23: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 24-24: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[error] 26-31: Unable to import 'utilities.constants'
(E0401)
[error] 32-32: Unable to import 'utilities.inference_utils'
(E0401)
[error] 33-33: Unable to import 'utilities.infra'
(E0401)
[error] 34-34: Unable to import 'utilities.serving_runtime'
(E0401)
[error] 36-36: Unable to import 'simple_logger.logger'
(E0401)
[convention] 36-36: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
42-44: Add missing docstring.The fixture lacks documentation explaining its purpose.
@pytest.fixture(scope="session") def root_dir(pytestconfig: pytest.Config) -> Any: + """Return the pytest root directory path.""" return pytestconfig.rootpath🧰 Tools
🪛 Pylint (3.3.7)
[convention] 43-43: Missing function or method docstring
(C0116)
47-55: Add missing docstring.The fixture lacks documentation explaining its purpose.
@pytest.fixture(scope="class") def mlserver_grpc_serving_runtime_template(admin_client: DynamicClient) -> Template: + """Create and yield MLServer gRPC serving runtime template.""" grpc_template_yaml = TEMPLATE_FILE_PATH.get(Protocols.GRPC)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 48-48: Missing function or method docstring
(C0116)
58-66: Add missing docstring.The fixture lacks documentation explaining its purpose.
@pytest.fixture(scope="class") def mlserver_rest_serving_runtime_template(admin_client: DynamicClient) -> Template: + """Create and yield MLServer REST serving runtime template.""" rest_template_yaml = TEMPLATE_FILE_PATH.get(Protocols.REST)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 59-59: Missing function or method docstring
(C0116)
69-71: Add missing docstring.The fixture lacks documentation explaining its purpose.
@pytest.fixture(scope="class") def protocol(request: FixtureRequest) -> str: + """Extract protocol type from test request parameters.""" return request.param["protocol_type"]🧰 Tools
🪛 Pylint (3.3.7)
[convention] 70-70: Missing function or method docstring
(C0116)
74-91: Add missing docstring.The fixture lacks documentation explaining its purpose and usage.
@pytest.fixture(scope="class") def mlserver_serving_runtime( request: FixtureRequest, admin_client: DynamicClient, model_namespace: Namespace, mlserver_runtime_image: str, protocol: str, ) -> Generator[ServingRuntime, None, None]: + """Create and yield MLServer serving runtime from template based on protocol.""" template_name = TEMPLATE_MAP.get(protocol, RuntimeTemplates.MLSERVER_REST)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 75-75: Missing function or method docstring
(C0116)
[warning] 80-80: Redefining name 'protocol' from outer scope (line 70)
(W0621)
95-138: Fix line length and add comprehensive documentation.The function has line length violations and needs better documentation for its complex configuration logic.
@pytest.fixture(scope="class") def mlserver_inference_service( request: FixtureRequest, admin_client: DynamicClient, model_namespace: Namespace, mlserver_serving_runtime: ServingRuntime, s3_models_storage_uri: str, - mlserver_model_service_account: ServiceAccount, + mlserver_model_service_account: ServiceAccount, ) -> Generator[InferenceService, Any, Any]: - """Create and yield a configured InferenceService for testing.""" + """ + Create and yield a configured InferenceService for testing. + + Configures GPU resources, volumes, timeout, and replica settings + based on test parameters. Supports both raw and serverless deployment modes. + """ params = request.param service_config = { "client": admin_client, "name": params.get("name"), "namespace": model_namespace.name, "runtime": mlserver_serving_runtime.name, "storage_uri": s3_models_storage_uri, - "model_format": mlserver_serving_runtime.instance.spec.supportedModelFormats[0].name, + "model_format": mlserver_serving_runtime.instance.spec.supportedModelFormats[0].name, # noqa: E501 "model_service_account": mlserver_model_service_account.name, - "deployment_mode": params.get("deployment_type", KServeDeploymentType.RAW_DEPLOYMENT), + "deployment_mode": params.get( + "deployment_type", KServeDeploymentType.RAW_DEPLOYMENT + ), "external_route": params.get("enable_external_route", False), }🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 95-95: Too many arguments (6/5)
(R0913)
[refactor] 95-95: Too many positional arguments (6/5)
(R0917)
[warning] 99-99: Redefining name 'mlserver_serving_runtime' from outer scope (line 75)
(W0621)
[warning] 101-101: Redefining name 'mlserver_model_service_account' from outer scope (line 142)
(W0621)
141-149: Add missing docstring and fix line length.@pytest.fixture(scope="class") -def mlserver_model_service_account(admin_client: DynamicClient, kserve_s3_secret: Secret) -> ServiceAccount: +def mlserver_model_service_account( + admin_client: DynamicClient, kserve_s3_secret: Secret +) -> ServiceAccount: + """Create and yield service account linked to S3 secret for model access.""" with ServiceAccount(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 142-142: Line too long (108/100)
(C0301)
[convention] 142-142: Missing function or method docstring
(C0116)
[warning] 142-142: Redefining name 'kserve_s3_secret' from outer scope (line 153)
(W0621)
152-170: Add missing docstring.@pytest.fixture(scope="class") def kserve_s3_secret( admin_client: DynamicClient, model_namespace: Namespace, aws_access_key_id: str, aws_secret_access_key: str, models_s3_bucket_region: str, models_s3_bucket_endpoint: str, ) -> Secret: + """Create and yield S3 secret with AWS credentials for model storage access.""" with kserve_s3_endpoint_secret(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 153-153: Missing function or method docstring
(C0116)
[refactor] 153-153: Too many arguments (6/5)
(R0913)
[refactor] 153-153: Too many positional arguments (6/5)
(R0917)
173-175: Add missing docstring.@pytest.fixture def mlserver_response_snapshot(snapshot: Any) -> Any: + """Configure snapshot testing with JSON extension for response validation.""" return snapshot.use_extension(extension_class=JSONSnapshotExtension)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 174-174: Missing function or method docstring
(C0116)
178-183: Add missing docstring and fix line length.@pytest.fixture -def mlserver_pod_resource(admin_client: DynamicClient, mlserver_inference_service: InferenceService) -> Pod: +def mlserver_pod_resource( + admin_client: DynamicClient, mlserver_inference_service: InferenceService +) -> Pod: + """Get the first pod associated with the MLServer inference service.""" pods = get_pods_by_isvc_label(client=admin_client, isvc=mlserver_inference_service) if not pods: - raise RuntimeError(f"No pods found for InferenceService {mlserver_inference_service.name}") + raise RuntimeError( + f"No pods found for InferenceService {mlserver_inference_service.name}" + ) return pods[0]🧰 Tools
🪛 Pylint (3.3.7)
[convention] 179-179: Line too long (108/100)
(C0301)
[convention] 179-179: Missing function or method docstring
(C0116)
[warning] 179-179: Redefining name 'mlserver_inference_service' from outer scope (line 95)
(W0621)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- utilities/constants.py
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.206Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (3)
tests/model_serving/model_runtime/mlserver/utils.py (4)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (1)
admin_client(53-54)tests/model_serving/model_runtime/mlserver/conftest.py (2)
root_dir(43-44)protocol(70-71)tests/model_serving/model_runtime/vllm/conftest.py (1)
response_snapshot(141-142)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(126-143)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(95-138)mlserver_pod_resource(179-183)mlserver_response_snapshot(174-175)protocol(70-71)root_dir(43-44)
tests/model_serving/model_runtime/mlserver/constant.py (1)
utilities/constants.py (3)
KServeDeploymentType(6-9)Protocols(89-96)RuntimeTemplates(65-75)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/utils.py
[convention] 13-13: Line too long (102/100)
(C0301)
[convention] 57-57: Line too long (106/100)
(C0301)
[convention] 84-84: Line too long (119/100)
(C0301)
[convention] 103-103: Line too long (114/100)
(C0301)
[convention] 104-104: Line too long (110/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-4: Unable to import 'portforward'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.secret'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 12-12: Unable to import 'utilities.constants'
(E0401)
[error] 13-13: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 14-18: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[convention] 22-22: Missing function or method docstring
(C0116)
[refactor] 22-22: Too many arguments (7/5)
(R0913)
[refactor] 22-22: Too many positional arguments (7/5)
(R0917)
[convention] 51-51: Missing function or method docstring
(C0116)
[convention] 57-57: Missing function or method docstring
(C0116)
[refactor] 83-83: Too many arguments (6/5)
(R0913)
[refactor] 83-83: Too many positional arguments (6/5)
(R0917)
[refactor] 113-117: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[convention] 122-122: Missing function or method docstring
(C0116)
[convention] 126-126: Missing function or method docstring
(C0116)
[refactor] 126-126: Too many arguments (7/5)
(R0913)
[refactor] 126-126: Too many positional arguments (7/5)
(R0917)
[convention] 5-5: standard import "subprocess" should be placed before third party imports "requests", "portforward"
(C0411)
[convention] 7-7: standard import "contextlib.contextmanager" should be placed before third party imports "requests", "portforward"
(C0411)
[convention] 8-8: standard import "typing.Generator" should be placed before third party imports "requests", "portforward"
(C0411)
tests/model_serving/model_runtime/mlserver/conftest.py
[convention] 142-142: Line too long (108/100)
(C0301)
[convention] 179-179: Line too long (108/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'syrupy.extensions.json'
(E0401)
[error] 7-7: Unable to import 'pytest_testconfig'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.serving_runtime'
(E0401)
[error] 12-12: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.pod'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.secret'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.template'
(E0401)
[error] 16-16: Unable to import 'ocp_resources.service_account'
(E0401)
[error] 18-23: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 24-24: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[error] 26-31: Unable to import 'utilities.constants'
(E0401)
[error] 32-32: Unable to import 'utilities.inference_utils'
(E0401)
[error] 33-33: Unable to import 'utilities.infra'
(E0401)
[error] 34-34: Unable to import 'utilities.serving_runtime'
(E0401)
[error] 36-36: Unable to import 'simple_logger.logger'
(E0401)
[convention] 43-43: Missing function or method docstring
(C0116)
[convention] 48-48: Missing function or method docstring
(C0116)
[convention] 59-59: Missing function or method docstring
(C0116)
[convention] 70-70: Missing function or method docstring
(C0116)
[convention] 75-75: Missing function or method docstring
(C0116)
[warning] 80-80: Redefining name 'protocol' from outer scope (line 70)
(W0621)
[refactor] 95-95: Too many arguments (6/5)
(R0913)
[refactor] 95-95: Too many positional arguments (6/5)
(R0917)
[warning] 99-99: Redefining name 'mlserver_serving_runtime' from outer scope (line 75)
(W0621)
[warning] 101-101: Redefining name 'mlserver_model_service_account' from outer scope (line 142)
(W0621)
[convention] 142-142: Missing function or method docstring
(C0116)
[warning] 142-142: Redefining name 'kserve_s3_secret' from outer scope (line 153)
(W0621)
[convention] 153-153: Missing function or method docstring
(C0116)
[refactor] 153-153: Too many arguments (6/5)
(R0913)
[refactor] 153-153: Too many positional arguments (6/5)
(R0917)
[convention] 174-174: Missing function or method docstring
(C0116)
[convention] 179-179: Missing function or method docstring
(C0116)
[warning] 179-179: Redefining name 'mlserver_inference_service' from outer scope (line 95)
(W0621)
[convention] 36-36: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
tests/conftest.py
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (109/100)
(C0301)
[convention] 115-115: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 97-97: Too many arguments (6/5)
(R0913)
[refactor] 97-97: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-8: Unable to import 'utilities.constants'
(E0401)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (7)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
1-41: Excellent test module structure and documentation.The module is well-organized with:
- Clear module docstring explaining purpose
- Proper imports and constants
- Good use of pytest markers for fixture requirements
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 40-40: Line too long (118/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
tests/model_serving/model_runtime/mlserver/constant.py (2)
10-36: Well-organized constants with clear naming conventions.The port definitions, path configurations, and template mappings are logically structured and follow good naming practices.
51-86: Deployment configurations and input queries are well-defined.The base deployment configurations appropriately distinguish between raw and serverless modes. The sklearn input queries for both REST and gRPC protocols are correctly structured and provide realistic test data.
tests/model_serving/model_runtime/mlserver/utils.py (3)
21-48: S3 secret management function is well-implemented.The
kserve_s3_endpoint_secretcontext manager correctly handles:
- Proper KServe annotations for S3 configuration
- Secure credential handling via string_data
- Resource lifecycle management with context manager pattern
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 22-22: Missing function or method docstring
(C0116)
[refactor] 22-22: Too many arguments (7/5)
(R0913)
[refactor] 22-22: Too many positional arguments (7/5)
(R0917)
51-81: Request handling functions are robust.Both
send_rest_requestandsend_grpc_requestfunctions implement:
- Appropriate error handling with timeouts and status checks
- Correct use of external tools (grpcurl for gRPC)
- Proper JSON serialization/deserialization
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 57-57: Line too long (106/100)
(C0301)
[convention] 51-51: Missing function or method docstring
(C0116)
[convention] 57-57: Missing function or method docstring
(C0116)
122-143: Helper and validation functions are correctly implemented.The
get_grpc_urlfunction properly handles URL formatting for gRPC endpoints, andvalidate_inference_requestprovides appropriate test validation with clear assertion messages.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 122-122: Missing function or method docstring
(C0116)
[convention] 126-126: Missing function or method docstring
(C0116)
[refactor] 126-126: Too many arguments (7/5)
(R0913)
[refactor] 126-126: Too many positional arguments (7/5)
(R0917)
utilities/manifests/common/grpc_predict_v2.proto (1)
1-327: Standard KServe V2 protocol definition - no changes needed.This file implements the official KServe V2 inference protocol specification and should remain unchanged. The Buf linting error about package directory mismatch should be ignored as this is a reference implementation that follows the standard KServe protocol specification.
Based on retrieved learnings, this proto file should be excluded from Buf linting rules since it's from the upstream KServe project and maintains compatibility with the standard inference protocol.
🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
b783a11 to
81dc456
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/model_serving/model_runtime/mlserver/utils.py (4)
99-99: Fix line length violation in function signature.The line length issue remains from previous reviews.
-def send_grpc_request(url: str, input_data: dict[str, Any], root_dir: str, insecure: bool = False) -> Any: +def send_grpc_request( + url: str, input_data: dict[str, Any], root_dir: str, insecure: bool = False +) -> Any:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 99-99: Line too long (106/100)
(C0301)
139-139: Fix line length violation in function signature.The line length issue remains from previous reviews.
-def run_mlserver_inference( - pod_name: str, isvc: InferenceService, input_data: dict[str, Any], model_version: str, protocol: str, root_dir: str -) -> Any: +def run_mlserver_inference( + pod_name: str, + isvc: InferenceService, + input_data: dict[str, Any], + model_version: str, + protocol: str, + root_dir: str +) -> Any:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 139-139: Line too long (119/100)
(C0301)
173-174: Fix line length violations in port forwarding section.These lines exceed the recommended length limit.
- with portforward.forward(pod_or_service=pod_name, namespace=isvc.namespace, from_port=port, to_port=port): - host = f"{LOCAL_HOST_URL}:{port}" if is_rest else get_grpc_url(base_url=LOCAL_HOST_URL, port=port) + with portforward.forward( + pod_or_service=pod_name, + namespace=isvc.namespace, + from_port=port, + to_port=port + ): + host = ( + f"{LOCAL_HOST_URL}:{port}" + if is_rest + else get_grpc_url(base_url=LOCAL_HOST_URL, port=port) + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 173-173: Line too long (114/100)
(C0301)
[convention] 174-174: Line too long (110/100)
(C0301)
181-187: Remove unnecessary else clause after return.The else clause is unnecessary since the previous branch returns.
elif deployment_mode == KServeDeploymentType.SERVERLESS: base_url = isvc.instance.status.url.rstrip("/") if is_rest: return send_rest_request(f"{base_url}{rest_endpoint}", input_data) - else: - grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) - return send_grpc_request(grpc_url, input_data, root_dir, insecure=True) + + grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) + return send_grpc_request(grpc_url, input_data, root_dir, insecure=True)🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🧹 Nitpick comments (11)
tests/conftest.py (1)
239-245: Add docstring for consistency with existing fixtures.The fixture implementation is correct, but it's missing a docstring. For consistency with other fixtures in the file, please add documentation.
@pytest.fixture(scope="session") def mlserver_runtime_image(pytestconfig: pytest.Config) -> str | None: + """ + Provides the MLServer runtime image for testing. + + Args: + pytestconfig (pytest.Config): The pytest configuration object. + + Returns: + str | None: The MLServer runtime image string, or None if not configured. + """ runtime_image = pytestconfig.option.mlserver_runtime_image if not runtime_image: return None return runtime_image🧰 Tools
🪛 Pylint (3.3.7)
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
39-41: Fix line length violation in pytestmark.The line exceeds the recommended length limit.
pytestmark = pytest.mark.usefixtures( - "root_dir", "valid_aws_config", "mlserver_rest_serving_runtime_template", "mlserver_grpc_serving_runtime_template" + "root_dir", + "valid_aws_config", + "mlserver_rest_serving_runtime_template", + "mlserver_grpc_serving_runtime_template" )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 40-40: Line too long (118/100)
(C0301)
44-46: Consider breaking long parametrize decorator for readability.The parametrize decorator line is quite long. Consider formatting it for better readability.
@pytest.mark.parametrize( - "protocol, model_namespace, mlserver_inference_service, s3_models_storage_uri, mlserver_serving_runtime", + ( + "protocol, model_namespace, mlserver_inference_service, " + "s3_models_storage_uri, mlserver_serving_runtime" + ), [🧰 Tools
🪛 Pylint (3.3.7)
[convention] 45-45: Line too long (109/100)
(C0301)
116-116: Fix line length violation in conditional statement.The line exceeds the recommended length limit.
- input_query = SKLEARN_REST_INPUT_QUERY if protocol == Protocols.REST else SKLEARN_GRPC_INPUT_QUERY + input_query = ( + SKLEARN_REST_INPUT_QUERY + if protocol == Protocols.REST + else SKLEARN_GRPC_INPUT_QUERY + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 116-116: Line too long (106/100)
(C0301)
tests/model_serving/model_runtime/mlserver/constant.py (3)
1-9: Add module docstring for documentation.The module lacks documentation explaining its purpose and contents.
+""" +Constants for MLServer model serving tests. + +This module defines configuration constants, deployment settings, +resource specifications, and input queries used across MLServer +test infrastructure for REST and gRPC protocols. +""" + import os from typing import Any, Union🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-8: Unable to import 'utilities.constants'
(E0401)
37-37: Fix line length violation in type annotation.The line exceeds the recommended length limit.
-PREDICT_RESOURCES: dict[str, Union[list[dict[str, Union[str, dict[str, str]]]], dict[str, dict[str, str]]]] = { +PREDICT_RESOURCES: dict[ + str, Union[list[dict[str, Union[str, dict[str, str]]]], dict[str, dict[str, str]]] +] = {🧰 Tools
🪛 Pylint (3.3.7)
[convention] 37-37: Line too long (111/100)
(C0301)
48-48: Fix line length violation in resources configuration.The line exceeds the recommended length limit.
- "resources": {"requests": {"cpu": "1", "memory": "2Gi"}, "limits": {"cpu": "2", "memory": "4Gi"}}, + "resources": { + "requests": {"cpu": "1", "memory": "2Gi"}, + "limits": {"cpu": "2", "memory": "4Gi"} + },🧰 Tools
🪛 Pylint (3.3.7)
[convention] 48-48: Line too long (102/100)
(C0301)
tests/model_serving/model_runtime/mlserver/conftest.py (4)
1-1: Add module docstring for better documentation.The module lacks a docstring explaining its purpose and the fixtures it provides.
+""" +Pytest fixtures for MLServer runtime testing. + +This module provides session and class-scoped fixtures for setting up MLServer +serving runtimes, inference services, and related Kubernetes resources needed +for comprehensive end-to-end testing of MLServer deployments with both REST +and gRPC protocols. +""" from typing import cast, Any, Generator🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
36-36: Fix import ordering to follow PEP 8 conventions.Third-party imports should come before local imports according to the static analysis hint.
+from simple_logger.logger import get_logger + from tests.model_serving.model_runtime.mlserver.constant import ( PREDICT_RESOURCES, RUNTIME_MAP, TEMPLATE_MAP, TEMPLATE_FILE_PATH, ) from tests.model_serving.model_runtime.mlserver.utils import kserve_s3_endpoint_secret from utilities.constants import ( KServeDeploymentType, Labels, RuntimeTemplates, Protocols, ) from utilities.inference_utils import create_isvc from utilities.infra import get_pods_by_isvc_label from utilities.serving_runtime import ServingRuntimeFromTemplate -from simple_logger.logger import get_logger🧰 Tools
🪛 Pylint (3.3.7)
[error] 36-36: Unable to import 'simple_logger.logger'
(E0401)
[convention] 36-36: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
57-57: Consider breaking long lines for better readability.Several function signatures exceed the 100-character line limit.
-def mlserver_grpc_serving_runtime_template(admin_client: DynamicClient) -> Generator[Template, None, None]: +def mlserver_grpc_serving_runtime_template( + admin_client: DynamicClient, +) -> Generator[Template, None, None]: -def mlserver_rest_serving_runtime_template(admin_client: DynamicClient) -> Generator[Template, None, None]: +def mlserver_rest_serving_runtime_template( + admin_client: DynamicClient, +) -> Generator[Template, None, None]: -def mlserver_model_service_account(admin_client: DynamicClient, kserve_s3_secret: Secret) -> ServiceAccount: +def mlserver_model_service_account( + admin_client: DynamicClient, kserve_s3_secret: Secret +) -> ServiceAccount:Also applies to: 77-77, 204-204
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 57-57: Line too long (107/100)
(C0301)
291-294: Enhance error handling with more descriptive information.The error message could include more context to help with debugging.
pods = get_pods_by_isvc_label(client=admin_client, isvc=mlserver_inference_service) if not pods: - raise RuntimeError(f"No pods found for InferenceService {mlserver_inference_service.name}") + raise RuntimeError( + f"No pods found for InferenceService '{mlserver_inference_service.name}' " + f"in namespace '{mlserver_inference_service.namespace}'" + ) return pods[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- utilities/constants.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
- conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.206Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (2)
tests/model_serving/model_runtime/mlserver/constant.py (1)
utilities/constants.py (3)
KServeDeploymentType(6-9)Protocols(89-96)RuntimeTemplates(65-75)
tests/model_serving/model_runtime/mlserver/utils.py (3)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (1)
admin_client(53-54)tests/model_serving/model_runtime/mlserver/conftest.py (2)
root_dir(43-53)protocol(97-107)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/conftest.py
[convention] 57-57: Line too long (107/100)
(C0301)
[convention] 77-77: Line too long (107/100)
(C0301)
[convention] 204-204: Line too long (108/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'syrupy.extensions.json'
(E0401)
[error] 7-7: Unable to import 'pytest_testconfig'
(E0401)
[error] 9-9: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.serving_runtime'
(E0401)
[error] 12-12: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.pod'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.secret'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.template'
(E0401)
[error] 16-16: Unable to import 'ocp_resources.service_account'
(E0401)
[error] 18-23: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 24-24: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[error] 26-31: Unable to import 'utilities.constants'
(E0401)
[error] 32-32: Unable to import 'utilities.inference_utils'
(E0401)
[error] 33-33: Unable to import 'utilities.infra'
(E0401)
[error] 34-34: Unable to import 'utilities.serving_runtime'
(E0401)
[error] 36-36: Unable to import 'simple_logger.logger'
(E0401)
[warning] 116-116: Redefining name 'protocol' from outer scope (line 97)
(W0621)
[refactor] 144-144: Too many arguments (6/5)
(R0913)
[refactor] 144-144: Too many positional arguments (6/5)
(R0917)
[warning] 148-148: Redefining name 'mlserver_serving_runtime' from outer scope (line 111)
(W0621)
[warning] 150-150: Redefining name 'mlserver_model_service_account' from outer scope (line 204)
(W0621)
[warning] 204-204: Redefining name 'kserve_s3_secret' from outer scope (line 225)
(W0621)
[refactor] 225-225: Too many arguments (6/5)
(R0913)
[refactor] 225-225: Too many positional arguments (6/5)
(R0917)
[warning] 276-276: Redefining name 'mlserver_inference_service' from outer scope (line 144)
(W0621)
[convention] 36-36: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
tests/conftest.py
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (109/100)
(C0301)
[convention] 116-116: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-8: Unable to import 'utilities.constants'
(E0401)
tests/model_serving/model_runtime/mlserver/utils.py
[convention] 99-99: Line too long (106/100)
(C0301)
[convention] 111-111: Line too long (101/100)
(C0301)
[convention] 139-139: Line too long (119/100)
(C0301)
[convention] 173-173: Line too long (114/100)
(C0301)
[convention] 174-174: Line too long (110/100)
(C0301)
[error] 18-18: Unable to import 'portforward'
(E0401)
[error] 20-20: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 21-21: Unable to import 'ocp_resources.secret'
(E0401)
[error] 22-22: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 24-24: Unable to import 'utilities.constants'
(E0401)
[error] 25-31: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
[refactor] 138-138: Too many arguments (6/5)
(R0913)
[refactor] 138-138: Too many positional arguments (6/5)
(R0917)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (5)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
88-127: Well-structured test class with comprehensive coverage.The test class effectively covers the key scenarios for MLServer sklearn model deployment:
- Both REST and gRPC protocols
- Both raw and serverless deployment modes
- Proper use of fixtures and parametrization
- Clear documentation and validation logic
The test design follows good practices for integration testing of model serving infrastructure.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 116-116: Line too long (106/100)
(C0301)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py (1)
10-86: Well-organized constants with comprehensive test coverage.The constants are well-structured and provide comprehensive configuration for:
- Protocol and runtime mappings consistent with
utilities/constants.py- Appropriate resource specifications for MLServer deployments
- Realistic input queries for sklearn iris model testing
- Clear separation between deployment types (raw vs serverless)
The REST and gRPC input queries contain the same logical data formatted appropriately for each protocol, which is exactly what's needed for comparative testing.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
tests/model_serving/model_runtime/mlserver/utils.py (1)
34-77: Comprehensive MLServer inference utilities with solid architecture.The utility functions provide a well-designed abstraction layer for MLServer testing:
kserve_s3_endpoint_secret: Proper context manager for K8s secret lifecyclesend_rest_requestandsend_grpc_request: Clean protocol-specific implementationsrun_mlserver_inference: Intelligent routing between deployment modes (RAW vs SERVERLESS)validate_inference_request: Simple but effective response validationThe architecture correctly handles the complexity of supporting multiple protocols and deployment types while maintaining clean interfaces for the test code.
Also applies to: 80-96, 138-189, 192-203, 206-239
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
tests/model_serving/model_runtime/mlserver/conftest.py (1)
42-53: LGTM! Well-structured pytest fixtures with proper resource management.The fixtures are well-implemented with:
- Proper context managers for resource cleanup
- Clear parameter handling and configuration
- Appropriate scoping (session/class level)
- Good separation of concerns
The fixture dependencies are logically structured and the use of generators with context managers ensures proper resource cleanup.
Also applies to: 56-73, 76-93, 96-107, 110-140, 143-200, 203-221, 224-256, 259-270, 273-294
utilities/manifests/common/grpc_predict_v2.proto (1)
1-327: LGTM! Standard KServe V2 inference protocol definition.This is the official KServe V2 inference protocol specification for gRPC. Based on the retrieved learnings, this file is a reference implementation that should not be modified to comply with local directory structure conventions.
The protocol definition provides comprehensive support for:
- Server and model readiness checks
- Metadata retrieval operations
- Inference requests with flexible tensor representations
- Both typed and raw tensor content formats
This enables the MLServer runtime tests to validate gRPC inference functionality properly.
🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
Snomaan6846
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_serving/model_runtime/mlserver/conftest.py (1)
182-189: GPU resource configuration is correctly implemented.The conditional logic properly handles GPU resources only when needed, avoiding the resource allocation issues mentioned in past reviews.
🧹 Nitpick comments (4)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (1)
39-41: Consider adding module docstring to address static analysis hint.While not critical, adding a module docstring would resolve the pylint convention warning.
+""" +Pytest configuration and fixtures for XGBoost MLServer model deployment tests. +""" pytestmark = pytest.mark.usefixtures( "root_dir", "valid_aws_config", "mlserver_rest_serving_runtime_template", "mlserver_grpc_serving_runtime_template" )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 40-40: Line too long (118/100)
(C0301)
tests/model_serving/model_runtime/mlserver/constant.py (1)
1-1: Add module docstring to improve code documentation.The module would benefit from a docstring explaining its purpose in the MLServer test suite.
+""" +Constants and configuration values for MLServer model serving tests. + +This module centralizes all configuration including ports, paths, resource +specifications, and input query templates for REST and gRPC protocols. +""" import os🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
tests/model_serving/model_runtime/mlserver/conftest.py (2)
1-1: Add module docstring to improve code organization.Adding a module docstring would help document the purpose of these fixtures in the MLServer test suite.
+""" +Pytest fixtures for MLServer model serving runtime tests. + +This module provides fixtures for creating MLServer serving runtimes, inference services, +Kubernetes resources, and utilities needed for comprehensive MLServer testing across +different protocols and deployment types. +""" from typing import cast, Any, Generator🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
35-35: Fix import ordering to follow PEP 8 conventions.The simple_logger import should be placed with other third-party imports.
import pytest from syrupy.extensions.json import JSONSnapshotExtension from pytest_testconfig import config as py_config +from simple_logger.logger import get_logger from kubernetes.dynamic import DynamicClient from ocp_resources.namespace import Namespace from ocp_resources.serving_runtime import ServingRuntime from ocp_resources.inference_service import InferenceService from ocp_resources.pod import Pod from ocp_resources.secret import Secret from ocp_resources.template import Template from ocp_resources.service_account import ServiceAccount from tests.model_serving.model_runtime.mlserver.constant import ( PREDICT_RESOURCES, RUNTIME_MAP, TEMPLATE_MAP, TEMPLATE_FILE_PATH, ) from tests.model_serving.model_runtime.mlserver.utils import kserve_s3_endpoint_secret from utilities.constants import ( KServeDeploymentType, Labels, RuntimeTemplates, Protocols, ) from utilities.inference_utils import create_isvc from utilities.infra import get_pods_by_isvc_label from utilities.serving_runtime import ServingRuntimeFromTemplate -from simple_logger.logger import get_logger🧰 Tools
🪛 Pylint (3.3.7)
[error] 35-35: Unable to import 'simple_logger.logger'
(E0401)
[convention] 35-35: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
tests/model_serving/model_runtime/mlserver/conftest.py (7)
tests/model_serving/model_runtime/mlserver/utils.py (1)
kserve_s3_endpoint_secret(35-77)utilities/constants.py (5)
KServeDeploymentType(6-9)Labels(169-191)RuntimeTemplates(65-75)Protocols(89-96)Nvidia(190-191)utilities/inference_utils.py (1)
create_isvc(517-717)utilities/infra.py (1)
get_pods_by_isvc_label(471-498)utilities/serving_runtime.py (1)
ServingRuntimeFromTemplate(11-230)tests/conftest.py (3)
admin_client(53-54)model_namespace(102-122)aws_secret_access_key(137-144)tests/model_serving/conftest.py (1)
s3_models_storage_uri(6-7)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
tests/model_serving/model_runtime/mlserver/constant.py (1)
utilities/constants.py (3)
KServeDeploymentType(6-9)Protocols(89-96)RuntimeTemplates(65-75)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (119/100)
(C0301)
[convention] 116-116: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/conftest.py
[convention] 56-56: Line too long (107/100)
(C0301)
[convention] 76-76: Line too long (107/100)
(C0301)
[convention] 202-202: Line too long (108/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'syrupy.extensions.json'
(E0401)
[error] 6-6: Unable to import 'pytest_testconfig'
(E0401)
[error] 8-8: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 9-9: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.serving_runtime'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 12-12: Unable to import 'ocp_resources.pod'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.secret'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.template'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.service_account'
(E0401)
[error] 17-22: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 23-23: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[error] 25-30: Unable to import 'utilities.constants'
(E0401)
[error] 31-31: Unable to import 'utilities.inference_utils'
(E0401)
[error] 32-32: Unable to import 'utilities.infra'
(E0401)
[error] 33-33: Unable to import 'utilities.serving_runtime'
(E0401)
[error] 35-35: Unable to import 'simple_logger.logger'
(E0401)
[warning] 115-115: Redefining name 'protocol' from outer scope (line 96)
(W0621)
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
[warning] 147-147: Redefining name 'mlserver_serving_runtime' from outer scope (line 110)
(W0621)
[warning] 149-149: Redefining name 'mlserver_model_service_account' from outer scope (line 202)
(W0621)
[warning] 202-202: Redefining name 'kserve_s3_secret' from outer scope (line 223)
(W0621)
[refactor] 223-223: Too many arguments (6/5)
(R0913)
[refactor] 223-223: Too many positional arguments (6/5)
(R0917)
[warning] 274-274: Redefining name 'mlserver_inference_service' from outer scope (line 143)
(W0621)
[convention] 35-35: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (119/100)
(C0301)
[convention] 116-116: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-8: Unable to import 'utilities.constants'
(E0401)
🔇 Additional comments (14)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (3)
1-7: Excellent test module structure for comprehensive XGBoost model validation.The module docstring clearly describes the test scope and the parameterized approach provides thorough coverage of different protocol and deployment combinations.
44-87: Well-designed parameterized test configuration.The parametrization comprehensively covers all combinations of protocols (REST/gRPC) and deployment types (raw/serverless), ensuring thorough validation of the XGBoost model deployment functionality.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 45-45: Line too long (119/100)
(C0301)
98-126: Clean and focused test implementation.The test method effectively leverages the validate_inference_request utility function and properly selects protocol-specific input queries. The fixture-based approach promotes reusability and maintainability.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 116-116: Line too long (106/100)
(C0301)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
1-7: Consistent and well-structured test module for SKLearn model validation.The module follows the same excellent pattern as the XGBoost tests, ensuring consistency across the MLServer test suite.
88-96: Comprehensive test class documentation.The docstring clearly outlines the test scenarios covered, making it easy for maintainers to understand the scope and purpose.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
98-126: Proper test implementation with protocol-aware input selection.The test correctly selects between REST and gRPC input queries based on the protocol parameter, demonstrating good separation of concerns and protocol-specific handling.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 116-116: Line too long (106/100)
(C0301)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/constant.py (4)
22-35: Well-organized template and runtime mappings.The protocol-based mapping approach provides clean separation between REST and gRPC configurations, making it easy to extend for additional protocols in the future.
37-49: Appropriate resource specifications for test scenarios.The volume mounts and resource limits are well-suited for MLServer testing, providing shared memory access and reasonable CPU/memory allocations.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
63-86: Properly structured input queries for sklearn model testing.The REST and gRPC input queries correctly follow their respective protocol specifications with appropriate data types and shapes for the iris dataset.
88-111: Consistent XGBoost input query structure.The XGBoost input queries maintain the same structure as sklearn queries while using model-specific identifiers, ensuring consistency across different model types.
tests/model_serving/model_runtime/mlserver/conftest.py (4)
55-72: Well-designed template fixture with proper scope.The class-scoped template fixture appropriately loads gRPC runtime templates and integrates cleanly with the test configuration system.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 56-56: Line too long (107/100)
(C0301)
109-139: Excellent serving runtime fixture with flexible configuration.The fixture properly handles protocol selection, template mapping, and deployment type configuration, providing a robust foundation for MLServer testing.
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 115-115: Redefining name 'protocol' from outer scope (line 96)
(W0621)
245-254: Robust S3 secret management using utility function.The fixture leverages the kserve_s3_endpoint_secret utility function for consistent secret creation with proper AWS configuration and KServe annotations.
289-292: Proper error handling for pod retrieval.The fixture includes appropriate error handling when no pods are found, providing clear diagnostics for debugging test failures.
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
7c00b7e to
fd1bcf7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/model_serving/model_runtime/mlserver/utils.py (4)
99-99: Fix line length violation.This line exceeds the 100-character limit.
Break the function signature into multiple lines for better readability:
-def send_grpc_request(url: str, input_data: dict[str, Any], root_dir: str, insecure: bool = False) -> Any: +def send_grpc_request( + url: str, + input_data: dict[str, Any], + root_dir: str, + insecure: bool = False +) -> Any:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 99-99: Line too long (106/100)
(C0301)
139-139: Fix line length violation.This function signature exceeds the recommended line length.
Break the function signature into multiple lines:
-def run_mlserver_inference( - pod_name: str, isvc: InferenceService, input_data: dict[str, Any], model_version: str, protocol: str, root_dir: str -) -> Any: +def run_mlserver_inference( + pod_name: str, + isvc: InferenceService, + input_data: dict[str, Any], + model_version: str, + protocol: str, + root_dir: str +) -> Any:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 139-139: Line too long (119/100)
(C0301)
173-174: Fix line length violations.These lines exceed the 100-character limit.
Break long lines for better readability:
- with portforward.forward(pod_or_service=pod_name, namespace=isvc.namespace, from_port=port, to_port=port): - host = f"{LOCAL_HOST_URL}:{port}" if is_rest else get_grpc_url(base_url=LOCAL_HOST_URL, port=port) + with portforward.forward( + pod_or_service=pod_name, + namespace=isvc.namespace, + from_port=port, + to_port=port + ): + host = ( + f"{LOCAL_HOST_URL}:{port}" + if is_rest + else get_grpc_url(base_url=LOCAL_HOST_URL, port=port) + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 173-173: Line too long (114/100)
(C0301)
[convention] 174-174: Line too long (110/100)
(C0301)
183-187: Remove unnecessary else clause.The else clause after a return statement is unnecessary and reduces code clarity.
Apply this diff to simplify the control flow:
if is_rest: return send_rest_request(f"{base_url}{rest_endpoint}", input_data) - else: - grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) - return send_grpc_request(grpc_url, input_data, root_dir, insecure=True) + + grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) + return send_grpc_request(grpc_url, input_data, root_dir, insecure=True)🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🧹 Nitpick comments (6)
tests/model_serving/model_runtime/mlserver/constant.py (1)
1-1: Add module docstring for better documentation.Consider adding a module docstring to document the purpose and contents of this constants file.
+""" +Constants for MLServer model serving tests. + +This module defines configuration values, resource specifications, deployment configs, +and input queries used across MLServer runtime tests for sklearn and XGBoost models. +""" + import os🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
tests/model_serving/model_runtime/mlserver/conftest.py (5)
1-1: Add module docstring.The module lacks documentation explaining its purpose and functionality.
Add a module docstring at the top of the file:
+""" +Pytest fixtures for MLServer model serving runtime tests. + +This module provides fixtures for: +- Setting up MLServer serving runtimes and templates +- Creating inference services and related Kubernetes resources +- Managing S3 secrets and service accounts +- Providing test utilities like snapshots and pod resources +""" + from typing import cast, Any, Generator🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
56-56: Fix line length violation.The function signature exceeds the recommended line length.
Break the function signature:
-def mlserver_grpc_serving_runtime_template(admin_client: DynamicClient) -> Generator[Template, None, None]: +def mlserver_grpc_serving_runtime_template( + admin_client: DynamicClient, +) -> Generator[Template, None, None]:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 56-56: Line too long (107/100)
(C0301)
76-76: Fix line length violation.The function signature exceeds the recommended line length.
Break the function signature:
-def mlserver_rest_serving_runtime_template(admin_client: DynamicClient) -> Generator[Template, None, None]: +def mlserver_rest_serving_runtime_template( + admin_client: DynamicClient, +) -> Generator[Template, None, None]:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 76-76: Line too long (107/100)
(C0301)
202-202: Fix line length violation.The function signature exceeds the recommended line length.
Break the function signature:
-def mlserver_model_service_account(admin_client: DynamicClient, kserve_s3_secret: Secret) -> ServiceAccount: +def mlserver_model_service_account( + admin_client: DynamicClient, + kserve_s3_secret: Secret +) -> ServiceAccount:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 202-202: Line too long (108/100)
(C0301)
[warning] 202-202: Redefining name 'kserve_s3_secret' from outer scope (line 223)
(W0621)
35-35: Fix import organization.Third-party imports should be placed before local application imports.
Move the simple_logger import to the third-party imports section:
from ocp_resources.service_account import ServiceAccount + +from simple_logger.logger import get_logger from tests.model_serving.model_runtime.mlserver.constant import ( PREDICT_RESOURCES, RUNTIME_MAP, TEMPLATE_MAP, TEMPLATE_FILE_PATH, ) from tests.model_serving.model_runtime.mlserver.utils import kserve_s3_endpoint_secret from utilities.constants import ( KServeDeploymentType, Labels, RuntimeTemplates, Protocols, ) from utilities.inference_utils import create_isvc from utilities.infra import get_pods_by_isvc_label from utilities.serving_runtime import ServingRuntimeFromTemplate - -from simple_logger.logger import get_logger🧰 Tools
🪛 Pylint (3.3.7)
[error] 35-35: Unable to import 'simple_logger.logger'
(E0401)
[convention] 35-35: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- utilities/constants.py
- conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.603Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.206Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (4)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
tests/model_serving/model_runtime/mlserver/constant.py (1)
utilities/constants.py (3)
KServeDeploymentType(6-9)Protocols(89-96)RuntimeTemplates(65-75)
tests/model_serving/model_runtime/mlserver/utils.py (4)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (2)
admin_client(53-54)aws_secret_access_key(137-144)tests/model_serving/model_runtime/mlserver/conftest.py (1)
root_dir(42-52)tests/model_serving/model_runtime/vllm/conftest.py (1)
response_snapshot(141-142)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/conftest.py
[convention] 56-56: Line too long (107/100)
(C0301)
[convention] 76-76: Line too long (107/100)
(C0301)
[convention] 202-202: Line too long (108/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'syrupy.extensions.json'
(E0401)
[error] 6-6: Unable to import 'pytest_testconfig'
(E0401)
[error] 8-8: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 9-9: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.serving_runtime'
(E0401)
[error] 11-11: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 12-12: Unable to import 'ocp_resources.pod'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.secret'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.template'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.service_account'
(E0401)
[error] 17-22: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 23-23: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[error] 25-30: Unable to import 'utilities.constants'
(E0401)
[error] 31-31: Unable to import 'utilities.inference_utils'
(E0401)
[error] 32-32: Unable to import 'utilities.infra'
(E0401)
[error] 33-33: Unable to import 'utilities.serving_runtime'
(E0401)
[error] 35-35: Unable to import 'simple_logger.logger'
(E0401)
[warning] 115-115: Redefining name 'protocol' from outer scope (line 96)
(W0621)
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
[warning] 147-147: Redefining name 'mlserver_serving_runtime' from outer scope (line 110)
(W0621)
[warning] 149-149: Redefining name 'mlserver_model_service_account' from outer scope (line 202)
(W0621)
[warning] 202-202: Redefining name 'kserve_s3_secret' from outer scope (line 223)
(W0621)
[refactor] 223-223: Too many arguments (6/5)
(R0913)
[refactor] 223-223: Too many positional arguments (6/5)
(R0917)
[warning] 274-274: Redefining name 'mlserver_inference_service' from outer scope (line 143)
(W0621)
[convention] 35-35: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_serving.model_runtime.mlserver.constant.PREDICT_RESOURCES", "tests.model_serving.model_runtime.mlserver.utils.kserve_s3_endpoint_secret", "utilities.constants.KServeDeploymentType", "utilities.inference_utils.create_isvc", "utilities.infra.get_pods_by_isvc_label", "utilities.serving_runtime.ServingRuntimeFromTemplate"
(C0411)
tests/conftest.py
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (119/100)
(C0301)
[convention] 116-116: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (119/100)
(C0301)
[convention] 116-116: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-8: Unable to import 'utilities.constants'
(E0401)
tests/model_serving/model_runtime/mlserver/utils.py
[convention] 99-99: Line too long (106/100)
(C0301)
[convention] 111-111: Line too long (101/100)
(C0301)
[convention] 139-139: Line too long (119/100)
(C0301)
[convention] 173-173: Line too long (114/100)
(C0301)
[convention] 174-174: Line too long (110/100)
(C0301)
[error] 18-18: Unable to import 'portforward'
(E0401)
[error] 20-20: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 21-21: Unable to import 'ocp_resources.secret'
(E0401)
[error] 22-22: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 24-24: Unable to import 'utilities.constants'
(E0401)
[error] 25-31: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
[refactor] 138-138: Too many arguments (6/5)
(R0913)
[refactor] 138-138: Too many positional arguments (6/5)
(R0917)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (10)
tests/conftest.py (1)
239-245: LGTM! Consistent fixture implementation.The new
mlserver_runtime_imagefixture follows the same pattern as the existingvllm_runtime_imagefixture, maintaining consistency in the codebase. The session scope and configuration retrieval logic are appropriate for this type of runtime image configuration.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 240-240: Missing function or method docstring
(C0116)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (1)
1-127: Excellent test structure with comprehensive coverage.The test module is well-implemented with:
- Clear documentation and purpose
- Comprehensive parameterization covering all protocol/deployment combinations
- Proper use of fixtures and utility functions
- Consistent patterns that align with the sklearn test module
The static analysis warnings about line length and argument count are acceptable for test code where readability and comprehensive coverage are prioritized.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (119/100)
(C0301)
[convention] 116-116: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
1-127: Consistent and well-structured test implementation.This test module mirrors the XGBoost test structure perfectly, demonstrating excellent consistency in the test suite design. The parameterization, fixture usage, and validation approach are identical between the two test modules, differing only in the model-specific constants and input queries as expected.
This consistency makes the test suite maintainable and reduces cognitive overhead for developers working across different model types.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 40-40: Line too long (118/100)
(C0301)
[convention] 45-45: Line too long (119/100)
(C0301)
[convention] 116-116: Line too long (106/100)
(C0301)
[error] 11-11: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.inference_service'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.pod'
(E0401)
[error] 16-16: Unable to import 'utilities.constants'
(E0401)
[error] 18-24: Unable to import 'tests.model_serving.model_runtime.mlserver.constant'
(E0401)
[error] 25-25: Unable to import 'tests.model_serving.model_runtime.mlserver.utils'
(E0401)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/constant.py (2)
37-49: Well-structured resource specifications.The predict resources definition is comprehensive and appropriate for MLServer deployments, including shared memory, temporary storage, and reasonable CPU/memory limits for test environments.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 37-37: Line too long (111/100)
(C0301)
[convention] 48-48: Line too long (102/100)
(C0301)
63-111: Comprehensive input query definitions.The input queries are well-structured and correctly formatted for both REST and gRPC protocols. The data structures follow the expected schemas:
- REST queries use the standard inference request format with
idandinputs- gRPC queries include the required
model_name,model_version, and properly formattedcontentswithfp32_contentsThe sample data is consistent between sklearn and xgboost queries, which will help with comparing model behavior across different frameworks.
tests/model_serving/model_runtime/mlserver/utils.py (2)
94-96: Good implementation of REST request handling.The function correctly handles HTTP errors with
raise_for_status()and includes proper timeout and SSL verification settings for testing environments.
131-135: Excellent error handling for gRPC subprocess calls.The try-catch block properly handles
CalledProcessErrorand returns descriptive error messages instead of letting exceptions propagate.tests/model_serving/model_runtime/mlserver/conftest.py (2)
183-189: Excellent GPU resource configuration improvement.The conditional GPU resource allocation correctly addresses the previous feedback about setting GPU resources only when needed. This prevents resource allocation issues when
gpu_countis 0.
289-292: Good error handling for pod retrieval.The function properly checks for empty pod lists and raises a descriptive
RuntimeErrorwhen no pods are found, which will help with debugging test failures.utilities/manifests/common/grpc_predict_v2.proto (1)
1-327: Standard KServe V2 inference protocol implementation.This is the official KServe V2 inference protocol specification and should remain unchanged. The comprehensive gRPC service definition correctly implements the standard inference API with proper message types for server metadata, model metadata, and inference operations.
Note: The Buf linting error about package directory mismatch should be ignored as this is a standardized protocol definition that doesn't need to conform to local directory structure conventions.
🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
fd1bcf7 to
9f20eec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/model_serving/model_runtime/mlserver/utils.py (1)
181-187: Address the unnecessary else clause.There's still an unnecessary else clause after a return statement that should be simplified.
elif deployment_mode == KServeDeploymentType.SERVERLESS: base_url = isvc.instance.status.url.rstrip("/") if is_rest: return send_rest_request(f"{base_url}{rest_endpoint}", input_data) - else: - grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) - return send_grpc_request(grpc_url, input_data, root_dir, insecure=True) + + grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) + return send_grpc_request(grpc_url, input_data, root_dir, insecure=True)🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
tests/model_serving/model_runtime/mlserver/conftest.py (1)
183-189: GPU resource configuration improvement noted.The previous review comment about GPU resource configuration when
gpu_countis 0 was acknowledged and will be addressed in future work. The current implementation correctly handles the case wheregpu_count > 0.
🧹 Nitpick comments (5)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (1)
39-41: Consider consolidating fixture dependencies.The
pytestmarkdeclaration lists fixtures that may not all be directly used by this test class. Consider only including fixtures that are actually required by this specific test to improve clarity and reduce unnecessary setup.tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
44-87: Consider reducing code duplication between test modules.This parameterization is nearly identical to the XGBoost test file, differing only in model-specific constants. Consider creating a base test class or fixture to reduce duplication.
Example approach:
# In a shared base module def create_model_test_params(model_name: str, model_constants: dict): return [ pytest.param( {"protocol_type": Protocols.REST}, {"name": f"{model_name}-raw-rest"}, {**BASE_RAW_DEPLOYMENT_CONFIG, **model_constants["name_dict"]}, model_constants["storage_uri_dict"], BASE_RAW_DEPLOYMENT_CONFIG, id=f"{model_name}-raw-rest-deployment", ), # ... other parameters ]tests/model_serving/model_runtime/mlserver/utils.py (1)
94-96: Security consideration: SSL verification disabled.The REST request disables SSL verification with
verify=False. Ensure this is appropriate for the test environment and consider making this configurable.-def send_rest_request(url: str, input_data: dict[str, Any]) -> Any: +def send_rest_request(url: str, input_data: dict[str, Any], verify_ssl: bool = False) -> Any: """ Sends a REST POST request to the specified URL with the given JSON payload. Args: url (str): The endpoint URL to send the request to. input_data (dict[str, Any]): The input payload to send as JSON. + verify_ssl (bool): Whether to verify SSL certificates. Defaults to False for testing. Returns: Any: The parsed JSON response from the server. Raises: requests.HTTPError: If the response contains an HTTP error status. """ - response = requests.post(url=url, json=input_data, verify=False, timeout=60) + response = requests.post(url=url, json=input_data, verify=verify_ssl, timeout=60)tests/model_serving/model_runtime/mlserver/conftest.py (1)
288-292: Add error handling for edge cases.Consider enhancing the error handling to provide more context about why no pods were found.
def mlserver_pod_resource( admin_client: DynamicClient, mlserver_inference_service: InferenceService, ) -> Pod: """ Retrieves the first Kubernetes Pod associated with the given MLServer InferenceService. Args: admin_client (DynamicClient): Kubernetes dynamic client. mlserver_inference_service (InferenceService): The MLServer InferenceService resource. Returns: Pod: The first Pod found for the InferenceService. Raises: RuntimeError: If no pods are found for the specified InferenceService. """ pods = get_pods_by_isvc_label(client=admin_client, isvc=mlserver_inference_service) if not pods: - raise RuntimeError(f"No pods found for InferenceService {mlserver_inference_service.name}") + raise RuntimeError( + f"No pods found for InferenceService {mlserver_inference_service.name} " + f"in namespace {mlserver_inference_service.namespace}. " + f"Check if the deployment is ready and pods are running." + ) return pods[0]utilities/manifests/common/grpc_predict_v2.proto (1)
16-16: Preserve official KServe V2 specification and ignore Buf package directory mismatchThis proto must retain
package inferenceeven though it's inutilities/manifests/common, as it tracks the upstream KServe V2 spec. Rather than moving or renaming, exclude it from Buf linting:Possible approaches:
- Add an in-file ignore directive before the syntax declaration:
+// buf:lint:ignore PACKAGE_DIRECTORY_MATCH syntax = "proto3";
- Add
utilities/manifests/common/grpc_predict_v2.prototo your.bufignoreto skip this rule.🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json
- utilities/constants.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
- conftest.py
- tests/conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/constant.py
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.214Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (2)
tests/model_serving/model_runtime/mlserver/utils.py (4)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (1)
admin_client(53-54)tests/model_serving/model_runtime/mlserver/conftest.py (2)
root_dir(42-52)protocol(96-106)tests/model_serving/model_runtime/vllm/conftest.py (1)
response_snapshot(141-142)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/conftest.py
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
[refactor] 223-223: Too many arguments (6/5)
(R0913)
[refactor] 223-223: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/utils.py
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
[refactor] 138-138: Too many arguments (6/5)
(R0913)
[refactor] 138-138: Too many positional arguments (6/5)
(R0917)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
🔇 Additional comments (7)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (2)
44-87: LGTM: Well-structured parameterized test configuration.The parameterization correctly covers all four deployment scenarios (REST/gRPC × raw/serverless) with appropriate fixture injection. The test IDs provide clear identification for each scenario.
98-126: LGTM: Clean test implementation with proper validation.The test method effectively uses the validation utility function and selects the appropriate input query based on protocol. The implementation is concise and follows testing best practices.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
98-126: LGTM: Consistent test implementation.The test method follows the same pattern as the XGBoost test, which is good for consistency. The sklearn-specific input queries are correctly selected based on protocol.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/utils.py (1)
206-239: LGTM: Well-implemented validation function.The validation function properly orchestrates inference execution and response comparison. The error message provides useful debugging information.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
tests/model_serving/model_runtime/mlserver/conftest.py (2)
55-72: LGTM: Proper template fixture with class scope.The gRPC template fixture is well-implemented with appropriate scope and resource management using context manager.
75-92: LGTM: Consistent REST template fixture.The REST template fixture follows the same pattern as the gRPC fixture, ensuring consistency in template management.
utilities/manifests/common/grpc_predict_v2.proto (1)
1-327: Approve reference gRPC proto definitionThis file matches the official KServe V2 inference protocol spec and correctly defines all service methods and message types needed by MLServer’s gRPC interface. No changes are required to the RPC or message definitions.
🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (1)
98-126: Address static analysis warnings if team standards require it.The test method implementation is correct and follows the established pattern. However, static analysis flags some style issues:
- Too many arguments (6/5): This is common with pytest integration tests that require multiple fixtures
- Too few public methods: Expected for focused test classes
Consider these approaches if pylint compliance is required:
- Extract some fixtures into a test context object
- Add pylint disable comments for acceptable test patterns
The logic correctly selects protocol-specific input queries and properly validates inference requests.
If pylint compliance is required, add this comment above the method:
+ # pylint: disable=too-many-arguments,too-many-positional-arguments def test_lightgbm_model_inference(🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_serving/model_runtime/mlserver/constant.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
🔇 Additional comments (4)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (4)
1-7: LGTM! Clear and comprehensive module documentation.The docstring effectively explains the module's purpose and testing scope across different protocols and deployment types.
8-26: LGTM! Well-organized imports and clean structure.The imports are appropriate for the testing functionality and follow good organization practices.
28-37: LGTM! Constants are well-defined and follow naming conventions.The model-specific constants and dictionaries provide clear configuration for the test scenarios.
44-87: LGTM! Comprehensive parameterization covers all deployment scenarios.The test parameters properly cover the matrix of:
- REST and gRPC protocols
- Raw and serverless deployment modes
- Appropriate configuration dictionaries for each scenario
The parameter structure correctly matches the fixture names in the decorator.
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
fe6b8bd to
df5557d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_serving/model_runtime/mlserver/conftest.py (1)
183-189: GPU resource configuration has been properly implemented.The conditional check for
gpu_count > 0correctly addresses the previous review concern about setting GPU resources when no GPUs are requested. The implementation now properly handles both GPU and non-GPU scenarios.
🧹 Nitpick comments (1)
tests/model_serving/model_runtime/mlserver/utils.py (1)
183-187: Remove unnecessary else clause for cleaner control flow.The else clause after the return statement is unnecessary and can be simplified.
Apply this diff to improve the control flow:
if is_rest: return send_rest_request(f"{base_url}{rest_endpoint}", input_data) -else: - grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) - return send_grpc_request(grpc_url, input_data, root_dir, insecure=True) + +grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) +return send_grpc_request(grpc_url, input_data, root_dir, insecure=True)🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- utilities/constants.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/constant.py
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json
- tests/conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json
- conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.214Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (4)
tests/model_serving/model_runtime/mlserver/utils.py (4)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (2)
admin_client(53-54)aws_secret_access_key(137-144)tests/model_serving/model_runtime/mlserver/conftest.py (2)
root_dir(42-52)protocol(96-106)tests/model_serving/model_runtime/vllm/conftest.py (1)
response_snapshot(141-142)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/utils.py
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
[refactor] 138-138: Too many arguments (6/5)
(R0913)
[refactor] 138-138: Too many positional arguments (6/5)
(R0917)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/conftest.py
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
[refactor] 223-223: Too many arguments (6/5)
(R0913)
[refactor] 223-223: Too many positional arguments (6/5)
(R0917)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (12)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (1)
1-127: Excellent test module structure and implementation!This test module follows pytest best practices with clear documentation, proper parameterization covering all deployment scenarios, and correct use of fixtures for MLServer testing. The logic correctly handles protocol-specific input queries and integrates well with the broader MLServer testing framework.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (1)
1-127: Consistent and well-structured test implementation!This XGBoost test module maintains excellent consistency with the testing framework pattern, properly using XGBoost-specific constants and following the same robust parameterization approach as the other ML framework tests.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
1-127: Excellent consistency with the MLServer testing framework!This sklearn test module completes the comprehensive test suite for MLServer runtime, maintaining perfect consistency with the established patterns while properly handling sklearn-specific constants and queries.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/utils.py (1)
1-240: Excellent utility module implementation!This module provides comprehensive and well-designed utilities for MLServer testing. The functions are properly documented, handle complex inference scenarios effectively, and integrate seamlessly with the Kubernetes and MLServer infrastructure. The code demonstrates good practices with proper error handling, timeout management, and resource cleanup.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
[refactor] 138-138: Too many arguments (6/5)
(R0913)
[refactor] 138-138: Too many positional arguments (6/5)
(R0917)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
tests/model_serving/model_runtime/mlserver/conftest.py (4)
41-52: Well-structured session-scoped fixture.The
root_dirfixture provides a clean way to access the pytest project root directory across the test session.
55-92: Protocol-specific template fixtures are well-designed.Both gRPC and REST template fixtures follow consistent patterns and properly use context managers for resource cleanup.
110-139: ServingRuntime fixture provides flexible configuration.The fixture correctly uses template mapping and supports different deployment types and protocols through parameterization.
271-292: Pod retrieval fixture includes proper error handling.The fixture correctly handles the case where no pods are found and provides a meaningful error message.
utilities/manifests/common/grpc_predict_v2.proto (4)
15-16: Standard KServe V2 inference protocol definition.This Protocol Buffer definition is from the official KServe V2 inference protocol specification and correctly defines the
inferencepackage. The file should remain unchanged to maintain compliance with the standard.Note: The Buf linting error about package directory mismatch should be ignored as this is a reference implementation of the KServe standard that must maintain its original package declaration.
🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
19-45: Comprehensive gRPC service definition.The
GRPCInferenceServiceprovides all essential inference server operations including liveness checks, readiness checks, metadata queries, and model inference capabilities.
135-203: Well-structured inference request message.The
ModelInferRequestmessage supports flexible tensor input specifications with both typed and raw content options, enabling efficient data transfer for various model types.
263-278: Flexible parameter system.The
InferParametermessage uses oneof to support different parameter types (boolean, integer, string), providing extensibility for inference customization.
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
df5557d to
591d3c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (1)
1-127: Excellent test coverage with minor duplication consideration.The test structure is well-designed and comprehensive. However, this module is nearly identical to the LightGBM test file except for model-specific constants.
Consider extracting a base test class or shared test function to reduce code duplication across the three model test files, while maintaining test clarity and isolation.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
1-127: Consistent and comprehensive test implementation.The test module follows the established pattern perfectly and provides thorough coverage of all deployment scenarios.
All three model test files (sklearn, xgboost, lightgbm) share nearly identical structure. Consider creating a parameterized base test class that can handle all model types:
@pytest.mark.parametrize("model_type", ["sklearn", "xgboost", "lightgbm"]) class TestMLServerModel: def test_model_inference(self, model_type, ...): # Generic test logic using model_type to select appropriate constantsThis would significantly reduce code duplication while maintaining comprehensive test coverage.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/conftest.py (2)
143-150: Consider refactoring fixtures to reduce parameter count.Both
mlserver_inference_serviceandkserve_s3_secretfixtures have 6+ parameters, which static analysis flags as potentially complex. While functionally correct, consider grouping related parameters into configuration objects for better maintainability.For example, you could create configuration dataclasses:
@dataclass class InferenceServiceConfig: name: str deployment_type: str gpu_count: int = 0 timeout: Optional[int] = None min_replicas: Optional[int] = None enable_external_route: bool = False @dataclass class S3Config: access_key_id: str secret_access_key: str region: str endpoint: strAlso applies to: 223-230
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
288-292: Enhance error handling with more context.The error message could provide more debugging information to help diagnose deployment issues.
- if not pods: - raise RuntimeError(f"No pods found for InferenceService {mlserver_inference_service.name}") + if not pods: + raise RuntimeError( + f"No pods found for InferenceService {mlserver_inference_service.name} " + f"in namespace {mlserver_inference_service.namespace}. " + f"Check if the InferenceService is properly deployed and running." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json
- utilities/constants.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/constant.py
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.214Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (2)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
tests/model_serving/model_runtime/mlserver/utils.py (4)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (2)
admin_client(53-54)aws_secret_access_key(137-144)tests/model_serving/model_runtime/mlserver/conftest.py (1)
root_dir(42-52)tests/model_serving/model_runtime/vllm/conftest.py (1)
response_snapshot(141-142)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/conftest.py
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
[refactor] 223-223: Too many arguments (6/5)
(R0913)
[refactor] 223-223: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/utils.py
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
[refactor] 138-138: Too many arguments (6/5)
(R0913)
[refactor] 138-138: Too many positional arguments (6/5)
(R0917)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
🔇 Additional comments (5)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (1)
1-127: Well-structured comprehensive test suite.The test module demonstrates excellent organization with:
- Clear documentation and purpose
- Comprehensive parameterization covering all protocol/deployment combinations
- Proper use of pytest fixtures and indirect parametrization
- Consistent naming conventions and type hints
The static analysis warnings about argument count and method count are acceptable for test code context.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/utils.py (1)
1-240: Comprehensive utility module with good documentation.The module provides essential functionality for MLServer testing with:
- Clear module documentation and well-organized imports
- Comprehensive support for both REST and gRPC protocols
- Proper error handling and context management
- Good separation of concerns across utility functions
The static analysis warnings about argument counts are acceptable for utility functions that require flexibility in testing scenarios.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
[refactor] 138-138: Too many arguments (6/5)
(R0913)
[refactor] 138-138: Too many positional arguments (6/5)
(R0917)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
tests/model_serving/model_runtime/mlserver/conftest.py (2)
182-189: Excellent improvement to GPU resource configuration!The conditional GPU resource setting has been properly implemented, addressing the previous review concern. GPU resources are now only configured when
gpu_count > 0, preventing invalid resource specifications.
1-293: Well-designed fixture architecture for comprehensive MLServer testing.The fixture design effectively supports the MLServer test suite with proper separation of concerns, resource management, and protocol flexibility. The integration between fixtures creates a clean testing environment.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
[refactor] 223-223: Too many arguments (6/5)
(R0913)
[refactor] 223-223: Too many positional arguments (6/5)
(R0917)
utilities/manifests/common/grpc_predict_v2.proto (1)
1-327: Standard KServe V2 inference protocol implementation - no changes needed.This proto file implements the official KServe V2 inference protocol specification and should remain unchanged. The Buf linting error about package directory mismatch should be ignored as this is a reference implementation that follows the standard specification structure.
As noted in the retrieved learnings, this file should be excluded from Buf linting since it's a standardized protocol definition that must maintain its original structure.
🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
591d3c7 to
3226a45
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (1)
28-30: Remove unusedLOGGERvariable
LOGGERis instantiated but never referenced. Dead code may trip linting rules (F841) and slightly slows module import.
Delete it or start emitting debug information.-LOGGER = get_logger(name=__name__)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (1)
28-30:LOGGERcreated but never usedSame as in the LightGBM test: drop the variable or actually log something to avoid “defined-but-unused” warnings.
-LOGGER = get_logger(name=__name__)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
28-30: Eliminate unused logger
LOGGERis unused here as well; remove to keep the test module tidy.-LOGGER = get_logger(name=__name__)tests/model_serving/model_runtime/mlserver/utils.py (2)
94-96: Avoid globally disabling TLS verification
requests.post(..., verify=False)skips certificate checks, undermining test authenticity and hiding real TLS issues.
Prefer one of:
- Pass
verify=Falseonly when explicitly required via function arg.- Supply a trusted CA bundle in CI and keep
verify=True(default).-def send_rest_request(url: str, input_data: dict[str, Any]) -> Any: +def send_rest_request(url: str, input_data: dict[str, Any], verify: bool = True) -> Any: @@ - response = requests.post(url=url, json=input_data, verify=False, timeout=60) + response = requests.post(url=url, json=input_data, verify=verify, timeout=60)
181-188: Unnecessaryelse– simplify control flowAfter the early return in the
is_restbranch, theelseblock can be de-indented for cleaner readability.- if is_rest: - return send_rest_request(f"{base_url}{rest_endpoint}", input_data) - else: - grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) - return send_grpc_request(grpc_url, input_data, root_dir, insecure=True) + if is_rest: + return send_rest_request(f"{base_url}{rest_endpoint}", input_data) + + grpc_url = get_grpc_url(base_url=base_url, port=MLSERVER_GRPC_REMOTE_PORT) + return send_grpc_request(grpc_url, input_data, root_dir, insecure=True)🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
utilities/manifests/common/grpc_predict_v2.proto (3)
15-17: Exclude official spec from Buf linting
Buf lint errors aroundpackage inferenceare expected because this file originates from the KServe V2 inference protocol spec (per retrieved learnings) and must remain unmodified. Please update your Buf configuration (e.g.buf.work.yamlorbuf.yaml) to exclude this path:workspace: - excludes: [] + excludes: + - "utilities/manifests/common/grpc_predict_v2.proto"🧰 Tools
🪛 Buf (1.54.0)
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
31-34: Clarify error handling semantics
The comments referencegoogle.rpc.Status, but the RPC methods rely on standard gRPC status codes rather than an embeddedStatusmessage. To avoid confusion, update the comments to mention gRPC statuses:- // The ServerMetadata API provides information about the server. Errors are - // indicated by the google.rpc.Status returned for the request. + // The ServerMetadata API provides information about the server. + // Errors are indicated by gRPC status codes on the RPC.
327-327: Add trailing newline
Ensure the file ends with a newline character to conform with POSIX text file conventions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- tests/conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json
- utilities/constants.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
- conftest.py
- tests/model_serving/model_runtime/mlserver/constant.py
🧰 Additional context used
🧠 Learnings (1)
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.214Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🧬 Code Graph Analysis (2)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (3)
utilities/constants.py (1)
Protocols(89-96)tests/model_serving/model_runtime/mlserver/utils.py (1)
validate_inference_request(206-239)tests/model_serving/model_runtime/mlserver/conftest.py (5)
mlserver_inference_service(143-198)mlserver_pod_resource(272-292)mlserver_response_snapshot(258-268)protocol(96-106)root_dir(42-52)
tests/model_serving/model_runtime/mlserver/utils.py (3)
utilities/constants.py (2)
KServeDeploymentType(6-9)Protocols(89-96)tests/conftest.py (1)
admin_client(50-51)tests/model_serving/model_runtime/mlserver/conftest.py (2)
root_dir(42-52)protocol(96-106)
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/conftest.py
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
[refactor] 223-223: Too many arguments (6/5)
(R0913)
[refactor] 223-223: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py
[refactor] 98-98: Too many arguments (6/5)
(R0913)
[refactor] 98-98: Too many positional arguments (6/5)
(R0917)
[refactor] 88-88: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/utils.py
[refactor] 35-35: Too many arguments (7/5)
(R0913)
[refactor] 35-35: Too many positional arguments (7/5)
(R0917)
[refactor] 138-138: Too many arguments (6/5)
(R0913)
[refactor] 138-138: Too many positional arguments (6/5)
(R0917)
[refactor] 183-187: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 206-206: Too many arguments (7/5)
(R0913)
[refactor] 206-206: Too many positional arguments (7/5)
(R0917)
🪛 Buf (1.54.0)
utilities/manifests/common/grpc_predict_v2.proto
16-16: Files with package "inference" must be within a directory "inference" relative to root but were in directory "utilities/manifests/common".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (3)
tests/model_serving/model_runtime/mlserver/utils.py (1)
119-129:grpcurldependency might be missingThe tests rely on an external
grpcurlbinary. If it’s absent in the CI container the gRPC paths will silently fail.
Consider:
- Adding a quick availability check and a meaningful error, or
- Documenting the requirement in
README.md/ test setup.if shutil.which("grpcurl") is None: # at top of function raise RuntimeError("grpcurl executable not found in PATH – install to run gRPC tests")tests/model_serving/model_runtime/mlserver/conftest.py (1)
178-190: GPU resources conditional looks goodNice fix – resources for GPUs are now only added when
gpu_count > 0, preventing invalid specs on CPU-only clusters.utilities/manifests/common/grpc_predict_v2.proto (1)
1-13: License header is correct
The Apache 2.0 license boilerplate is properly applied.
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
3226a45 to
bdb89b0
Compare
…r runtime deployment Signed-off-by: Snomaan6846 <syedali@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
for more information, see https://pre-commit.ci
…r runtime deployment Signed-off-by: Snomaan6846 <syedali@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
for more information, see https://pre-commit.ci
…er runtime deployment Signed-off-by: Snomaan6846 <syedali@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
for more information, see https://pre-commit.ci
…er runtime deployment Signed-off-by: Snomaan6846 <syedali@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
for more information, see https://pre-commit.ci
… runtime deployment Signed-off-by: Snomaan6846 <syedali@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
for more information, see https://pre-commit.ci
…time incompatibility Signed-off-by: Snomaan6846 <syedali@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…erver runtime deployment Signed-off-by: Snomaan6846 <syedali@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
for more information, see https://pre-commit.ci
Signed-off-by: Snomaan6846 <syedali@redhat.com>
Snomaan6846
force-pushed
the
RHOAIENG-25026
branch
from
fdbb62b to
2f24f21
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (1)
1-131: Code duplication acknowledged - functional implementation looks good.This test module follows the established pattern for MLServer framework testing and appears functionally correct. The code duplication across framework test files has already been discussed in previous reviews and will be addressed in JIRA ticket RHOAIENG-27991.
The parameterization properly covers all deployment scenarios (REST/gRPC with raw/serverless), and the test logic is sound.
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (1)
1-131: Functionally correct implementation following established pattern.This XGBoost test module mirrors the structure of other framework tests and appears functionally sound. The code duplication issue has been previously discussed and will be addressed per JIRA ticket RHOAIENG-27991.
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_huggingface_basic_model_deployment.py (1)
1-131: Code duplication previously discussed - implementation is correct.The HuggingFace test implementation follows the same pattern as other framework tests. The use of
NON_DETERMINISTIC_OUTPUTis appropriate for generative models. As noted in previous reviews, the duplication across framework files will be addressed in the planned refactoring (JIRA RHOAIENG-27991).tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlflow_basic_model_deployment.py (1)
1-131: MLflow test implementation follows established pattern correctly.This test module properly implements MLflow model testing using the established MLServer test framework. The code duplication across framework files has been previously acknowledged and will be consolidated per the refactoring plan.
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_catboost_basic_model_deployment.py (1)
1-131: CatBoost test implementation completes the MLServer framework test suite.This final framework test module correctly implements CatBoost model testing following the established pattern. The comprehensive test suite now covers all six ML frameworks (sklearn, XGBoost, LightGBM, CatBoost, MLflow, and HuggingFace) as outlined in the PR objectives.
While code duplication exists across framework files, this has been previously discussed and planned for future consolidation.
Would you like me to help draft a consolidated test structure for the planned refactoring in JIRA ticket RHOAIENG-27991?
🧹 Nitpick comments (1)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py (1)
100-107: Consider consolidating method parameters to address pylint warnings.The test method has 6 parameters, which triggers pylint warnings. While acceptable for test code, you could consider consolidating related parameters into a configuration object if this pattern is repeated across multiple test methods.
- def test_lightgbm_model_inference( - self, - mlserver_inference_service: InferenceService, - mlserver_pod_resource: Pod, - mlserver_response_snapshot: Any, - protocol: str, - root_dir: str, - ) -> None: + def test_lightgbm_model_inference( + self, + mlserver_inference_service: InferenceService, + mlserver_pod_resource: Pod, + mlserver_response_snapshot: Any, + protocol: str, + root_dir: str, + ) -> None:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_catboost_basic_model_deployment/TestCatBoostModel.test_catboost_model_inference[catboost-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_catboost_basic_model_deployment/TestCatBoostModel.test_catboost_model_inference[catboost-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_catboost_basic_model_deployment/TestCatBoostModel.test_catboost_model_inference[catboost-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_catboost_basic_model_deployment/TestCatBoostModel.test_catboost_model_inference[catboost-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_mlflow_basic_model_deployment/TestMLflowModel.test_mlflow_model_inference[mlflow-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_mlflow_basic_model_deployment/TestMLflowModel.test_mlflow_model_inference[mlflow-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_mlflow_basic_model_deployment/TestMLflowModel.test_mlflow_model_inference[mlflow-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_mlflow_basic_model_deployment/TestMLflowModel.test_mlflow_model_inference[mlflow-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/__snapshots__/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_catboost_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_huggingface_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlflow_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py(1 hunks)tests/model_serving/model_runtime/mlserver/conftest.py(1 hunks)tests/model_serving/model_runtime/mlserver/constant.py(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml(1 hunks)tests/model_serving/model_runtime/mlserver/utils.py(1 hunks)utilities/constants.py(1 hunks)utilities/manifests/common/grpc_predict_v2.proto(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_catboost_basic_model_deployment/TestCatBoostModel.test_catboost_model_inference[catboost-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_catboost_basic_model_deployment/TestCatBoostModel.test_catboost_model_inference[catboost-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_mlflow_basic_model_deployment/TestMLflowModel.test_mlflow_model_inference[mlflow-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/constant.py
🚧 Files skipped from review as they are similar to previous changes (21)
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-rest-deployment].json
- utilities/constants.py
- tests/conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_mlflow_basic_model_deployment/TestMLflowModel.test_mlflow_model_inference[mlflow-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_catboost_basic_model_deployment/TestCatBoostModel.test_catboost_model_inference[catboost-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-serverless-grpc-deployment].json
- conftest.py
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_xgboost_basic_model_deployment/TestXGBoostModel.test_xgboost_model_inference[xgboost-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_mlflow_basic_model_deployment/TestMLflowModel.test_mlflow_model_inference[mlflow-raw-rest-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_catboost_basic_model_deployment/TestCatBoostModel.test_catboost_model_inference[catboost-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_mlflow_basic_model_deployment/TestMLflowModel.test_mlflow_model_inference[mlflow-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_sklearn_basic_model_deployment/TestSkLearnModel.test_sklearn_model_inference[sklearn-iris-serverless-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/basic_model_deployment/snapshots/test_lightgbm_basic_model_deployment/TestLightGBMModel.test_lightgbm_model_inference[lightgbm-iris-raw-grpc-deployment].json
- tests/model_serving/model_runtime/mlserver/mlserver_grpc_serving_template.yaml
- tests/model_serving/model_runtime/mlserver/mlserver_rest_serving_template.yaml
🧰 Additional context used
🧠 Learnings (2)
tests/model_serving/model_runtime/mlserver/conftest.py (1)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#328
File: tests/model_serving/model_runtime/mlserver/conftest.py:182-189
Timestamp: 2025-06-16T05:39:15.070Z
Learning: The create_isvc function in utilities/inference_utils.py uses the parameter name `volumes_mounts` (with underscore), not `volume_mounts`, for passing volume mount configurations to InferenceService resources.
utilities/manifests/common/grpc_predict_v2.proto (3)
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:16-16
Timestamp: 2025-06-03T06:31:46.614Z
Learning: The grpc_predict_v2.proto file in utilities/manifests/common/ is from the official KServe V2 inference protocol specification and should not be modified. The package declaration "inference" is standardized and should remain unchanged even if it triggers Buf linting errors about package directory mismatch. This file should be excluded from Buf linting as it's a reference implementation.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#325
File: utilities/manifests/common/grpc_predict_v2.proto:15-17
Timestamp: 2025-06-03T06:31:57.214Z
Learning: The file utilities/manifests/common/grpc_predict_v2.proto is from the KServe V2 inference protocol standard and should not be modified to comply with local directory structure conventions.
🪛 Pylint (3.3.7)
tests/model_serving/model_runtime/mlserver/conftest.py
[refactor] 153-153: Too many arguments (6/5)
(R0913)
[refactor] 153-153: Too many positional arguments (6/5)
(R0917)
[refactor] 233-233: Too many arguments (6/5)
(R0913)
[refactor] 233-233: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_catboost_basic_model_deployment.py
[refactor] 100-100: Too many arguments (6/5)
(R0913)
[refactor] 100-100: Too many positional arguments (6/5)
(R0917)
[refactor] 90-90: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_huggingface_basic_model_deployment.py
[refactor] 100-100: Too many arguments (6/5)
(R0913)
[refactor] 100-100: Too many positional arguments (6/5)
(R0917)
[refactor] 90-90: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_lightgbm_basic_model_deployment.py
[refactor] 100-100: Too many arguments (6/5)
(R0913)
[refactor] 100-100: Too many positional arguments (6/5)
(R0917)
[refactor] 90-90: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlflow_basic_model_deployment.py
[refactor] 100-100: Too many arguments (6/5)
(R0913)
[refactor] 100-100: Too many positional arguments (6/5)
(R0917)
[refactor] 90-90: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py
[refactor] 100-100: Too many arguments (6/5)
(R0913)
[refactor] 100-100: Too many positional arguments (6/5)
(R0917)
[refactor] 90-90: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py
[refactor] 100-100: Too many arguments (6/5)
(R0913)
[refactor] 100-100: Too many positional arguments (6/5)
(R0917)
[refactor] 90-90: Too few public methods (1/2)
(R0903)
tests/model_serving/model_runtime/mlserver/utils.py
[refactor] 39-39: Too many arguments (7/5)
(R0913)
[refactor] 39-39: Too many positional arguments (7/5)
(R0917)
[refactor] 143-143: Too many arguments (6/5)
(R0913)
[refactor] 143-143: Too many positional arguments (6/5)
(R0917)
[refactor] 188-192: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 211-211: Too many arguments (9/5)
(R0913)
[refactor] 211-211: Too many positional arguments (9/5)
(R0917)
🔇 Additional comments (12)
tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_xgboost_basic_model_deployment.py (1)
34-34: Verify model version consistency across frameworks.The XGBoost model uses version "v1.0.0" while other frameworks use "v0.1.0". Ensure this version difference is intentional and matches the actual model artifacts.
#!/bin/bash # Description: Check model version consistency across MLServer test files # Search for MODEL_VERSION definitions across all test files rg "MODEL_VERSION.*=" tests/model_serving/model_runtime/mlserver/basic_model_deployment/tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_sklearn_basic_model_deployment.py (1)
1-131: LGTM! Well-structured parameterized test implementation.This test module effectively covers the required sklearn model inference scenarios across different protocols and deployment modes. The comprehensive parameterization covers all four combinations (REST/gRPC × raw/serverless), and the test logic is cleanly separated into utility functions.
The documentation is thorough and the import organization follows good practices.
tests/model_serving/model_runtime/mlserver/utils.py (5)
38-82: Well-implemented S3 secret context manager.The function correctly handles S3 endpoint configuration and AWS credential management. The high argument count is justified given the comprehensive S3 setup requirements, and the implementation properly strips URL schemes and uses appropriate context management.
84-101: REST request function looks good.The function properly handles REST requests with configurable TLS verification and appropriate timeout settings. Good improvement from previous reviews.
103-141: gRPC request implementation is solid.The function properly handles gRPC requests using grpcurl with improved TLS flag handling. The error handling approach through string returns is acceptable given the snapshot validation pattern used in tests.
143-195: Comprehensive inference function with good protocol abstraction.The function effectively handles both REST and gRPC protocols across different deployment modes. The complexity is well-managed, and the logic flow is clear. The remaining style improvements (unnecessary else clause) are acknowledged to be addressed in the next release.
197-314: Excellent validation utilities with clear separation of concerns.The validation functions provide a clean abstraction for different output types. The deterministic validation uses exact matching while the non-deterministic validation intelligently extracts and validates generated content. The error handling and protocol-specific parsing are well-implemented.
tests/model_serving/model_runtime/mlserver/conftest.py (4)
51-117: Well-designed template and protocol fixtures.The fixtures provide clean abstractions for runtime templates with appropriate scoping (session for root_dir, class for templates and protocol). The implementations are straightforward and well-documented.
119-150: Solid serving runtime fixture implementation.The fixture correctly creates ServingRuntime resources using template mapping and deployment type configuration. The parameter extraction and context management are well-handled.
152-209: Excellent inference service fixture with proper resource management.The fixture handles complex InferenceService configuration including GPU resources, timeout settings, and volume mounts. The use of
copy.deepcopy()for shared resources prevents test interference, and the conditional GPU configuration is well-implemented. The parameter extraction and service configuration building are clean and maintainable.
211-303: Clean and well-structured supporting fixtures.The remaining fixtures provide essential test infrastructure: service account creation, S3 secret management, snapshot testing support, and pod retrieval. All implementations follow good practices with proper error handling and resource management.
utilities/manifests/common/grpc_predict_v2.proto (1)
1-327: Standard KServe V2 gRPC protocol definition - no modifications needed.This is the official KServe V2 inference protocol specification that defines the standard gRPC API contract. Based on previous learnings, this file should remain unchanged as it's a reference implementation from the KServe project. The comprehensive service definition with 6 RPC methods and supporting message types provides the foundation for MLServer's gRPC interface.
|
Status of building tag latest: success. |
6 participants
Signed-off-by: Snomaan6846 syedali@redhat.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Description
This PR introduces a new test suite for the Seldon MLServer runtime. The suite includes both REST and gRPC interface tests to validate end-to-end model serving functionality.
Below mentioned frameworks have been included to Seldon MLserver test suite.
Summary of Changes:
How Has This Been Tested?
I have verified that the pytest execution was successful, all test cases passed, and the respective snapshots of the test results are included as part of this PR. I followed the steps below for execution.
JIRA Links :