test: Add test for kserve-raw model car#337
Conversation
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
WalkthroughThe changes introduce support for multiple deployment modes in the model car inference service fixture, parameterize tests to run with both serverless and raw deployment modes, and centralize the OCI image reference in a new constant. Test logic is updated to accommodate these enhancements, including a relaxed threshold for container restart failures. Changes
Suggested labels
Suggested reviewers
Poem
✨ 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
CodeRabbit Configuration File (
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/hold', '/lgtm', '/verified'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
utilities/constants.py (1)
214-218: Add class docstring and address line length issue.The centralization of the OCI image reference is a good practice. However, please address the following code quality issues:
+class ModelCarImage: + """Constants for model car OCI image references.""" + MNIST_8_1: str = ( + "oci://quay.io/mwaykole/test@sha256:" + "8a3217bcfa2cc5fa3d07496cff8b234acdf2c9725dd307dc0a80401f55e1a11c" + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 216-216: Line too long (123/100)
(C0301)
[convention] 214-214: Missing class docstring
(C0115)
[refactor] 214-214: Too few public methods (0/2)
(R0903)
tests/model_serving/model_server/model_car/test_oci_image.py (1)
5-5: Address line length issue in import statement.-from utilities.constants import ModelCarImage, ModelFormat, ModelName, Protocols, RuntimeTemplates, KServeDeploymentType +from utilities.constants import ( + KServeDeploymentType, + ModelCarImage, + ModelFormat, + ModelName, + Protocols, + RuntimeTemplates, +)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 5-5: Line too long (120/100)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_serving/model_server/model_car/conftest.py(1 hunks)tests/model_serving/model_server/model_car/test_oci_image.py(3 hunks)utilities/constants.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_serving/model_server/model_car/conftest.py (2)
utilities/constants.py (1)
KServeDeploymentType(6-9)utilities/inference_utils.py (1)
create_isvc(517-717)
🪛 Pylint (3.3.7)
utilities/constants.py
[convention] 216-216: Line too long (123/100)
(C0301)
[convention] 214-214: Missing class docstring
(C0115)
[refactor] 214-214: Too few public methods (0/2)
(R0903)
tests/model_serving/model_server/model_car/test_oci_image.py
[convention] 5-5: Line too long (120/100)
(C0301)
[convention] 56-56: Line too long (111/100)
(C0301)
🔇 Additional comments (5)
tests/model_serving/model_server/model_car/test_oci_image.py (2)
12-45: LGTM: Well-structured parameterized tests for multiple deployment modes.The parameterization correctly covers both serverless and raw deployment modes using the same OCI image, with appropriate pytest markers for each case.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 13-13: Line too long (106/100)
(C0301)
56-56:Details
❓ Verification inconclusive
Verify the increased restart threshold is appropriate.
The restart threshold was changed from
> 1to> 2. This change could potentially mask legitimate restart issues and should be justified.Please verify this change is necessary by running the following script to check if there are documented issues with container restarts in model car deployments:
🏁 Script executed:
#!/bin/bash # Description: Search for documentation or issues related to model car container restarts # Expected: Evidence that >2 restarts is a reasonable threshold # Search for restart-related issues in code comments rg -i "restart.*model.*car|model.*car.*restart" --type py -A 3 -B 3 # Search for restart threshold configurations in other tests rg "restartCount.*>" --type py -A 2 -B 2 # Search for any TODO/FIXME comments related to restarts rg -i "todo.*restart|fixme.*restart|restart.*todo|restart.*fixme" --type pyLength of output: 2631
Verify restart threshold change
The test in
tests/model_serving/model_server/model_car/test_oci_image.pynow treats any container withrestartCount > 2as a failure, whereas elsewhere we only guard againstrestartCount > 0. Raising the threshold to 2 could mask legitimate restart issues unless justified by past pod behavior.• Location:
- tests/model_serving/model_server/model_car/test_oci_image.py:56
• Diff snippet:
- container.name for container in pod.instance.status.containerStatuses if container.restartCount > 1 + container.name for container in pod.instance.status.containerStatuses if container.restartCount > 2Please confirm that allowing up to two restarts for model car pods is intentional and supported by deployment metrics or documented incidents.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 56-56: Line too long (111/100)
(C0301)
tests/model_serving/model_server/model_car/conftest.py (3)
21-21: LGTM: Dynamic deployment mode extraction.Good implementation of extracting the deployment mode from request parameters with a sensible default.
24-24: LGTM: Service naming includes deployment mode for better test isolation.Including the deployment mode in the service name helps differentiate between test instances and avoids potential conflicts.
29-30: LGTM: Well-structured parameter passing to create_isvc.The dynamic deployment mode and external route parameters are properly extracted and passed to the inference service creation function.
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_serving/model_server/model_car/test_oci_image.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_serving/model_server/model_car/test_oci_image.py (5)
utilities/constants.py (6)
ModelCarImage(214-217)ModelFormat(12-19)ModelName(22-27)Protocols(87-94)RuntimeTemplates(65-73)KServeDeploymentType(6-9)utilities/inference_utils.py (1)
Inference(44-138)tests/model_serving/model_server/model_car/conftest.py (1)
model_car_inference_service(15-33)utilities/infra.py (1)
get_pods_by_isvc_label(471-498)tests/model_serving/model_server/utils.py (1)
verify_inference_response(20-169)
🔇 Additional comments (6)
tests/model_serving/model_server/model_car/test_oci_image.py (6)
5-5: Good practice: Using constants for deployment types.The import of
KServeDeploymentTypehelps avoid magic strings and improves maintainability.
9-9: LGTM: Global markers align with parameterized tests.The
pytestmarkcorrectly includes bothserverlessandrawdeploymentmarkers to match the deployment types being tested.
13-13: Good refactoring: Generalized fixture name.The fixture name change from
model_car_serverless_inference_servicetomodel_car_inference_servicealigns with the generalized fixture inconftest.pythat supports multiple deployment modes.
24-24: Excellent: Centralized OCI image reference.Using the
ModelCarImage.MNIST_8_1constant instead of a hardcoded string improves maintainability and reduces the risk of typos.
25-42: LGTM: Comprehensive parameterization for deployment modes.The parameterization correctly tests both
SERVERLESSandRAW_DEPLOYMENTmodes with appropriate pytest markers. This ensures the model car functionality works across different deployment scenarios.
49-71: LGTM: Test methods updated to use generalized fixture.Both test methods correctly use the updated
model_car_inference_servicefixture, enabling them to run across different deployment modes.
|
Status of building tag latest: success. |
* test: Add test for kserve-raw model car Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb> * test: Add test for kserve-raw model car Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb> * test: Add test for kserve-raw model car Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb> --------- Signed-off-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb> Co-authored-by: Milind Waykole <mwaykole@mwaykole-thinkpadp1gen4i.bengluru.csb>
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Tests