Tests for stopping and resuming an ISVC#370
Tests for stopping and resuming an ISVC#370israel-hdez merged 35 commits intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
## Summary by CodeRabbit
* **New Features**
* Introduced support for controlling model serving pod rollout using a new "stop" annotation for KServe ONNX deployments.
* Added end-to-end tests validating stop and resume behavior for both raw and serverless KServe model serving.
* Provided fixtures to facilitate dynamic patching of the stop annotation during tests.
* **Bug Fixes**
* None.
* **Refactor**
* Improved test fixture structure for consistency and maintainability.
* **Chores**
* Updated constants to centralize KServe annotation keys.
## Walkthrough
This change introduces support for a "stop" annotation in KServe InferenceService deployment and testing utilities. It adds and utilizes new constants for annotation keys, updates the `create_isvc` function to handle the stop annotation, and introduces new end-to-end tests verifying stop/resume behavior for ONNX models in both raw and serverless KServe environments.
## Changes
| Files / Groups | Change Summary |
|-------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------|
| tests/model_serving/model_server/conftest.py | Added support for an optional "stop" parameter in KServe-related pytest fixtures, passing it to `create_isvc` as "stop_resume". |
| tests/model_serving/model_server/stop_resume/conftest.py | Added pytest fixtures patching InferenceService resources with stop annotation set to "true" or "false" for raw and serverless. |
| tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py | New test module: End-to-end tests for raw ONNX model serving with stop/resume annotation behavior. |
| tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py | New test module: End-to-end tests for serverless ONNX model serving with stop/resume annotation behavior. |
| tests/model_serving/model_server/stop_resume/utils.py | Added utility function to verify absence of inference pods within a timeout period for stop/resume tests. |
| utilities/constants.py | Added `KSERVE` API group constant, refactored annotation keys to use it, and introduced `FORCE_STOP_RUNTIME` annotation key. |
| utilities/inference_utils.py | Updated `create_isvc` to accept a `stop_resume` parameter, apply the stop annotation, and adjust pod readiness logic accordingly. |
## Suggested labels
`size/m`
## Suggested reviewers
- dbasunag
- adolfo-abWarning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)tests/model_serving/model_server/stop_resume/utils.py📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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
|
hdefazio
changed the title
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/verified', '/wip', '/lgtm', '/hold', '/cherry-pick'} |
hdefazio
force-pushed
the
feat/stop_resume_model
branch
from
2e71f46 to
342a5e4
Compare
for more information, see https://pre-commit.ci
|
/wip |
…rless failing tests. Signed-off-by: Andres Llausas <allausas@redhat.com>
hdefazio
changed the title
for more information, see https://pre-commit.ci
|
/wip cancel |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
utilities/inference_utils.py (1)
717-717: Consider more robust string comparison for stop_resume checks.The current string comparison
stop_resume != "True"might be brittle. Consider making it case-insensitive or using a boolean conversion for better robustness.Consider this approach for more robust comparison:
- if wait_for_predictor_pods and stop_resume != "True": + if wait_for_predictor_pods and stop_resume.lower() != "true":And similarly for line 731:
- if wait and stop_resume != "True": + if wait and stop_resume.lower() != "true":Also applies to: 731-731
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py (1)
62-63: Improve docstring formatting consistency.Some docstrings are not properly formatted. Consider using consistent triple-quote placement.
Apply this formatting for consistency:
- """Verify pods do not exist""" + # Verify pods do not existOr use proper multi-line docstring format:
- """Verify pods do not exist""" + """ + Verify pods do not exist + """Also applies to: 88-89, 103-104
tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py (2)
62-63: Improve docstring formatting consistency.Same formatting issues as in the raw test file. Consider using consistent docstring formatting.
Apply consistent formatting as suggested in the raw test file review.
Also applies to: 88-89, 103-104
1-111: Consider refactoring to reduce code duplication.This test file is nearly identical to
test_raw_stop_resume_model.py. Consider creating a base test class or shared utilities to reduce duplication and improve maintainability.You could create a base test class with common test methods and have deployment-specific classes inherit from it, or use shared utility functions for common test patterns.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_serving/model_server/conftest.py(2 hunks)tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py(1 hunks)tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py(1 hunks)utilities/constants.py(2 hunks)utilities/inference_utils.py(4 hunks)
🔇 Additional comments (7)
utilities/constants.py (2)
123-123: Good addition of centralized API group constant.Adding the
KSERVEconstant centralizes the API group string and improves maintainability across the codebase.
134-135: Excellent refactoring to use centralized constants.The refactoring of
DEPLOYMENT_MODEto use the centralizedApiGroups.KSERVEconstant and the addition ofFORCE_STOP_RUNTIMEimprove consistency and maintainability. This follows the DRY principle effectively.tests/model_serving/model_server/conftest.py (2)
362-363: Good parameter handling for stop functionality.The addition of stop parameter handling is consistent with the new stop/resume feature requirements.
377-392: Excellent refactoring improves readability and consistency.The refactoring to build the
isvc_kwargsdictionary before callingcreate_isvcimproves readability and maintains consistency with theovms_kserve_inference_servicefixture. The addition of stop parameter handling follows the same pattern.utilities/inference_utils.py (1)
571-571: Good addition of stop_resume parameter.The new parameter supports the stop/resume functionality as intended.
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py (1)
1-111: Comprehensive test coverage for raw deployment stop/resume functionality.The test module provides excellent coverage of the stop/resume functionality for raw deployments. The test scenarios are well-structured and cover all the expected use cases:
- Inference works when stop annotation is "False"
- Updating stop annotation to "true" deletes pods
- No pods exist when stop annotation starts as "True"
- Updating stop annotation to "false" creates pods and enables inference
The use of constants and proper pytest parametrization is commendable.
tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py (1)
1-111: Comprehensive test coverage for serverless deployment stop/resume functionality.The test module mirrors the raw deployment tests effectively, providing comprehensive coverage for serverless deployments. The test scenarios properly validate all aspects of the stop/resume functionality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py
Outdated
Show resolved
Hide resolved
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py
Outdated
Show resolved
Hide resolved
tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py
Outdated
Show resolved
Hide resolved
tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py
Outdated
Show resolved
Hide resolved
|
@hdefazio I created a fixture similar to what you would need here in my last PR. You can create an identical one, but for |
…ching isvcs. Signed-off-by: Andres Llausas <allausas@redhat.com>
I had not seen this, but this is example I used hahaha. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Signed-off-by: Andres Llausas <allausas@redhat.com>
…hub-tests into feat/stop_resume_model
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py(1 hunks)tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py(1 hunks)tests/model_serving/model_server/stop_resume/utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
tests/model_serving/model_server/stop_resume/utils.py (1)
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py (2)
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py (2)
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
🧬 Code Graph Analysis (1)
tests/model_serving/model_server/stop_resume/utils.py (1)
tests/model_serving/model_server/serverless/utils.py (1)
verify_no_inference_pods(20-50)
🪛 Pylint (3.3.7)
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py
[refactor] 54-54: Too many arguments (6/5)
(R0913)
[refactor] 54-54: Too many positional arguments (6/5)
(R0917)
[refactor] 111-111: Too many arguments (6/5)
(R0913)
[refactor] 111-111: Too many positional arguments (6/5)
(R0917)
tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py
[refactor] 55-55: Too many arguments (6/5)
(R0913)
[refactor] 55-55: Too many positional arguments (6/5)
(R0917)
[refactor] 112-112: Too many arguments (6/5)
(R0913)
[refactor] 112-112: Too many positional arguments (6/5)
(R0917)
🔇 Additional comments (4)
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py (1)
64-68: Function usage will be affected by utility fix.This usage of
verify_no_pods_exist_with_timeoutwill need to be reviewed after the critical logic issue in the utility function is resolved (see comment on utils.py).tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py (3)
65-69: Function usage will be affected by utility fix.This usage of
verify_no_pods_exist_with_timeoutwill need to be reviewed after the critical logic issue in the utility function is resolved (see comment on utils.py).
101-105: Function usage will be affected by utility fix.This usage of
verify_no_pods_exist_with_timeoutwill need to be reviewed after the critical logic issue in the utility function is resolved (see comment on utils.py).
20-36: Good consistent parametrization between test classes.Both test classes in this file properly include the
deployment-modeparameter, maintaining consistency. This is better than the raw deployment tests which have inconsistent parametrization.
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py
Show resolved
Hide resolved
Signed-off-by: Andres Llausas <allausas@redhat.com>
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py
Show resolved
Hide resolved
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py (3)
61-62: Remove redundant docstring.The method has two separate docstrings that should be combined into one clear description.
- def test_stop_and_update_to_true_delete_pod_rollout( - self, - unprivileged_client, - unprivileged_model_namespace, - ovms_kserve_serving_runtime, - ovms_raw_inference_service, - patched_raw_inference_service_stop_annotation, - ): - """Verify pod rollout is deleted when the stop annotation updated to true""" - """Verify pods do not exist""" + def test_stop_and_update_to_true_delete_pod_rollout( + self, + unprivileged_client, + unprivileged_model_namespace, + ovms_kserve_serving_runtime, + ovms_raw_inference_service, + patched_raw_inference_service_stop_annotation, + ): + """Verify pod rollout is deleted when the stop annotation is updated to true"""
96-97: Remove redundant docstring.Similar to the previous method, this has two separate docstrings that should be combined.
- def test_stop_and_true_no_pod_rollout( - self, - unprivileged_client, - unprivileged_model_namespace, - ovms_kserve_serving_runtime, - ovms_raw_inference_service, - ): - """Verify no pod rollout when the stop annotation is true""" - """Verify pods do not exist""" + def test_stop_and_true_no_pod_rollout( + self, + unprivileged_client, + unprivileged_model_namespace, + ovms_kserve_serving_runtime, + ovms_raw_inference_service, + ): + """Verify no pod rollout when the stop annotation is true"""
117-118: Remove redundant docstring.This method also has two separate docstrings that should be combined for clarity.
- def test_stop_and_update_to_false_pod_rollout( - self, - unprivileged_client, - unprivileged_model_namespace, - ovms_kserve_serving_runtime, - ovms_raw_inference_service, - patched_raw_inference_service_stop_annotation, - ): - """Verify pod rollout when the stop annotation is updated to false""" - """Verify that kserve Raw ONNX model can be queried using REST""" + def test_stop_and_update_to_false_pod_rollout( + self, + unprivileged_client, + unprivileged_model_namespace, + ovms_kserve_serving_runtime, + ovms_raw_inference_service, + patched_raw_inference_service_stop_annotation, + ): + """Verify pod rollout when the stop annotation is updated to false and model can be queried using REST"""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py (8)
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
Learnt from: jiripetrlik
PR: opendatahub-io/opendatahub-tests#335
File: tests/rag/test_rag.py:0-0
Timestamp: 2025-06-19T14:32:17.658Z
Learning: The `wait_for_replicas()` method from ocp_resources.deployment.Deployment raises an exception when timeout is reached rather than returning a boolean value, so it doesn't need explicit return value checking in tests.
Learnt from: jiripetrlik
PR: opendatahub-io/opendatahub-tests#335
File: tests/rag/test_rag.py:0-0
Timestamp: 2025-06-19T14:32:17.658Z
Learning: The `wait_for_replicas()` method from ocp_resources.deployment.Deployment raises an exception when timeout is reached rather than returning a boolean value, so it doesn't need explicit return value checking in tests.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: brettmthompson
PR: opendatahub-io/opendatahub-tests#269
File: tests/model_serving/model_server/serverless/test_scale_to_zero.py:0-0
Timestamp: 2025-04-29T00:49:40.918Z
Learning: In test cases for the opendatahub-tests repository, when verifying specific behaviors resulting from a sequence of operations, hard-coded validation values like "configurationGeneration=3" can be appropriate assertions to validate expected states, especially when they represent an invariant that should remain consistent after specific operations.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
🪛 Pylint (3.3.7)
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py
[refactor] 53-53: Too many arguments (6/5)
(R0913)
[refactor] 53-53: Too many positional arguments (6/5)
(R0917)
[refactor] 109-109: Too many arguments (6/5)
(R0913)
[refactor] 109-109: Too many positional arguments (6/5)
(R0917)
🔇 Additional comments (1)
tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py (1)
1-126: Well-structured test implementation for stop/resume functionality.The test file effectively covers the KServe stop/resume functionality with appropriate test cases:
TestStopRaw: Tests the workflow from running state (stop=False) to stopped state (stop=True)TestStoppedResumeRaw: Tests the workflow from stopped state (stop=True) to running state (stop=False)The use of pytest parameterization and fixtures follows the established patterns in the codebase, and the test logic correctly validates both the absence of pods when stopped and successful inference when running.
…mplers. It gives false positives. Signed-off-by: Andres Llausas <allausas@redhat.com>
…hub-tests into feat/stop_resume_model
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/model_serving/model_server/serverless/utils.py (1)
34-34: Fix docstring inconsistency.The docstring states "Raises: TimeoutError: If pods exist after the timeout" but the function now returns
Falseinstead of raising an exception in this case.- Raises: - TimeoutError: If pods exist after the timeout. + Note: + No exceptions are raised on timeout. Returns False if pods exist after timeout.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/model_serving/model_server/serverless/test_scale_to_zero.py(2 hunks)tests/model_serving/model_server/serverless/test_zero_initial_scale.py(1 hunks)tests/model_serving/model_server/serverless/utils.py(1 hunks)tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py(1 hunks)tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py(1 hunks)tests/model_serving/model_server/stop_resume/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/model_serving/model_server/stop_resume/utils.py
- tests/model_serving/model_server/stop_resume/test_raw_stop_resume_model.py
- tests/model_serving/model_server/stop_resume/test_serverless_stop_resume_model.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
tests/model_serving/model_server/serverless/test_scale_to_zero.py (1)
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
tests/model_serving/model_server/serverless/test_zero_initial_scale.py (2)
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
tests/model_serving/model_server/serverless/utils.py (4)
Learnt from: israel-hdez
PR: opendatahub-io/opendatahub-tests#346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper `create_isvc` (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/utils.py:40-44
Timestamp: 2025-06-18T06:42:37.705Z
Learning: In tests/model_registry/rest_api/utils.py, the user prefers to keep the default value of verify=False in execute_model_registry_post_command to maintain backward compatibility, rather than changing it to True for security reasons.
Learnt from: jiripetrlik
PR: opendatahub-io/opendatahub-tests#335
File: tests/rag/test_rag.py:0-0
Timestamp: 2025-06-19T14:32:17.658Z
Learning: The `wait_for_replicas()` method from ocp_resources.deployment.Deployment raises an exception when timeout is reached rather than returning a boolean value, so it doesn't need explicit return value checking in tests.
Learnt from: jiripetrlik
PR: opendatahub-io/opendatahub-tests#335
File: tests/rag/test_rag.py:0-0
Timestamp: 2025-06-19T14:32:17.658Z
Learning: The `wait_for_replicas()` method from ocp_resources.deployment.Deployment raises an exception when timeout is reached rather than returning a boolean value, so it doesn't need explicit return value checking in tests.
🧬 Code Graph Analysis (3)
tests/model_serving/model_server/serverless/test_scale_to_zero.py (3)
tests/model_serving/model_server/serverless/utils.py (1)
verify_no_inference_pods(20-55)tests/conftest.py (1)
unprivileged_client(288-328)tests/model_serving/model_server/serverless/conftest.py (1)
inference_service_patched_replicas(22-36)
tests/model_serving/model_server/serverless/test_zero_initial_scale.py (3)
tests/model_serving/model_server/serverless/utils.py (1)
verify_no_inference_pods(20-55)tests/conftest.py (1)
admin_client(55-56)tests/model_serving/model_server/conftest.py (1)
ovms_kserve_inference_service(325-365)
tests/model_serving/model_server/serverless/utils.py (2)
utilities/constants.py (1)
Timeout(209-219)utilities/infra.py (1)
get_pods_by_isvc_label(566-593)
🔇 Additional comments (5)
tests/model_serving/model_server/serverless/test_zero_initial_scale.py (1)
44-44: LGTM! Explicit assertion improves test reliability.The addition of the
assertstatement properly validates the boolean return value from the updatedverify_no_inference_podsfunction, making the test outcome explicit rather than relying on implicit behavior.tests/model_serving/model_server/serverless/test_scale_to_zero.py (2)
64-64: LGTM! Consistent with the updated function signature.The explicit assertion correctly validates the boolean return value from
verify_no_inference_pods, improving test clarity and reliability.
87-87: LGTM! Maintains consistency across test methods.The assertion pattern is consistent with other test methods and properly handles the boolean return value from the updated utility function.
tests/model_serving/model_server/serverless/utils.py (2)
20-22: LGTM! Improved function signature with configurable timeout.The addition of the
wait_timeoutparameter with a sensible default and the boolean return type makes the function more flexible and testable.
49-55: LGTM! Proper boolean return logic.The return logic is well-implemented:
- Returns
Truewhen no pods are found (success case)- Returns
TruewhenResourceNotFoundErroroccurs (also success - no pods exist)- Returns
Falsewhen timeout expires with pods still present (failure case)- Includes appropriate error logging for debugging
Signed-off-by: Andres Llausas <allausas@redhat.com>
…hub-tests into feat/stop_resume_model
for more information, see https://pre-commit.ci
Signed-off-by: Andres Llausas <allausas@redhat.com>
…hub-tests into feat/stop_resume_model
Regarding Milind's comment above. Do we want these markers removed? It looks like they are still present Besides that changes lgtm |
|
|
@brettmthompson @dbasunag @mwaykole For enabling merge, can you mark your comments as resolved, where applicable? |
|
Status of building tag latest: success. |
8 participants
Description
Adds tests for the stop and resume feature added in kserve/kserve#4337 and kserve/kserve#4455
How Has This Been Tested?
Merge criteria: