DB Storage test to verify TrustyAIService works with multiple namespaces#455
DB Storage test to verify TrustyAIService works with multiple namespaces#455kpunwatk wants to merge 1 commit intoopendatahub-io:mainfrom
Conversation
new file: tests/model_explainability/trustyai_service/service/multi_ns/conftest.py new file: tests/model_explainability/trustyai_service/service/multi_ns/test_trustyai_service_multi_ns.py modified: tests/model_explainability/trustyai_service/utils.py
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis update introduces a new multi-namespace fixture module for TrustyAI service integration testing, adds comprehensive multi-namespace test classes for both PVC and MariaDB storage backends, and enhances utility functions with explicit context manager declarations and improved MariaDB pod readiness logic. The changes focus on robust, automated multi-namespace test orchestration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
tests/model_explainability/trustyai_service/service/multi_ns/test_trustyai_service_multi_ns.py (3)
9-9: Fix import formattingAdd a space after the comma in the import statement.
- verify_trustyai_service_metric_delete_request, verify_trustyai_service_metric_request, + verify_trustyai_service_metric_delete_request, + verify_trustyai_service_metric_request,
120-121: Use explicit list construction for better readabilityInstead of multiplying a list by 2, consider being more explicit about the bucket configuration.
- {"bucket": MinIo.Buckets.MODELMESH_EXAMPLE_MODELS} - ]*2, + {"bucket": MinIo.Buckets.MODELMESH_EXAMPLE_MODELS}, + {"bucket": MinIo.Buckets.MODELMESH_EXAMPLE_MODELS}, + ],
142-142: Remove extra space before opening parenthesisPython style guide recommends no space before opening parenthesis in function calls.
Line 142:
- for tai, inference_model, inference_token in zip ( trustyai_service_with_db_storage_multi_ns, gaussian_credit_model_multi_ns, isvc_getter_token_multi_ns): + for tai, inference_model, inference_token in zip(trustyai_service_with_db_storage_multi_ns, gaussian_credit_model_multi_ns, isvc_getter_token_multi_ns):Line 176:
- for tai, model in zip( trustyai_service_with_db_storage_multi_ns, gaussian_credit_model_multi_ns): + for tai, model in zip(trustyai_service_with_db_storage_multi_ns, gaussian_credit_model_multi_ns):Line 196:
- for tai, model in zip (trustyai_service_with_db_storage_multi_ns, gaussian_credit_model_multi_ns): + for tai, model in zip(trustyai_service_with_db_storage_multi_ns, gaussian_credit_model_multi_ns):Also applies to: 176-176, 196-196
tests/model_explainability/trustyai_service/service/multi_ns/conftest.py (3)
239-270: Remove large commented code blockThis large commented block for
trustyai_db_secret_multi_nsshould be removed if not needed, or implemented if required.If this fixture is not needed, remove the entire commented block. If it might be needed later, consider moving it to a separate file or documenting why it's commented out.
413-413: Use logger instead of printFor consistency with the rest of the codebase, use the logger instead of print statements.
- print(f"Waiting for MariaDB pods in namespace {ns.name} ...") + LOGGER.info(f"Waiting for MariaDB pods in namespace {ns.name} ...")Note: You'll need to import the logger at the top of the file:
from simple_logger.logger import get_logger LOGGER = get_logger(name=__name__)
146-146: Remove unused fixture parameterThe
trustyai_service_with_db_storage_multi_nsparameter ingaussian_credit_model_multi_nsfixture appears to be unused and has a comment suggesting it should be checked and removed.minio_data_connection_multi_ns, mlserver_runtime_multi_ns, - trustyai_service_with_db_storage_multi_ns, #check this and remove if not needed ) -> Generator[List[InferenceService], Any, None]:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_explainability/trustyai_service/service/multi_ns/conftest.py(1 hunks)tests/model_explainability/trustyai_service/service/multi_ns/test_trustyai_service_multi_ns.py(1 hunks)tests/model_explainability/trustyai_service/utils.py(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
tests/model_explainability/trustyai_service/utils.py (4)
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, Service resources can be created without explicitly passing the admin_client parameter when using the context manager approach with "with Service()". The client parameter is optional for Service resource creation.
Learnt from: adolfo-ab
PR: #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: jiripetrlik
PR: #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: #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.
tests/model_explainability/trustyai_service/service/multi_ns/conftest.py (11)
Learnt from: adolfo-ab
PR: #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: lugi0
PR: #446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
Learnt from: Snomaan6846
PR: #444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:666-676
Timestamp: 2025-07-17T15:41:54.284Z
Learning: In tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the admin_client parameter in their signatures, unlike some other Secret fixtures in the codebase. The user lugi0 confirmed this is the correct pattern for these specific fixtures.
Learnt from: dbasunag
PR: #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.
Learnt from: dbasunag
PR: #401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, Service resources can be created without explicitly passing the admin_client parameter when using the context manager approach with "with Service()". The client parameter is optional for Service resource creation.
Learnt from: dbasunag
PR: #354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
tests/model_explainability/trustyai_service/service/multi_ns/test_trustyai_service_multi_ns.py (2)
Learnt from: adolfo-ab
PR: #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: Snomaan6846
PR: #444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.
🪛 Flake8 (7.2.0)
tests/model_explainability/trustyai_service/service/multi_ns/test_trustyai_service_multi_ns.py
[error] 218-218: continuation line missing indentation or outdented
(E122)
[error] 219-219: continuation line missing indentation or outdented
(E122)
[error] 220-220: continuation line missing indentation or outdented
(E122)
[error] 221-221: continuation line missing indentation or outdented
(E122)
🔇 Additional comments (4)
tests/model_explainability/trustyai_service/utils.py (3)
1-1: LGTM! Proper context manager declarationThe addition of
contextmanagerimport and decorators correctly formalizes these functions as context managers, which aligns with their usage pattern in the new multi-namespace fixtures.Also applies to: 119-119, 160-160, 178-178, 205-205, 233-233
49-49: Good improvement: Dynamic label selectorUsing
mariadb.nameinstead of a hardcoded string makes the function more flexible and reusable across different MariaDB instances.
42-42: Verify 5-minute timeout for MariaDB pod readinessI didn’t find any existing references to a shorter startup window for MariaDB—other fixtures (e.g. in
inference_utils.py, top-level conftests) still useTimeout.TIMEOUT_15MIN. Unless you’ve validated that MariaDB consistently reachesREADYwithin 5 minutes in all target environments, please:
- Run the test suite on slower clusters (e.g. CI with lower resources) to confirm no flakes.
- If you observe timeouts, consider reverting to
Timeout.TIMEOUT_15MINor making this timeout configurable.Locations using the default 5 min timeout:
- utilities/mariadb_utils.py: wait_for_mariadb_pods(…, timeout=Timeout.TIMEOUT_5MIN)
- tests/model_explainability/trustyai_service/utils.py: same call
- All conftests invoking
wait_for_mariadb_podswithout an explicit timeouttests/model_explainability/trustyai_service/service/multi_ns/conftest.py (1)
58-60: Remove or uncomment the modelmesh checkThis commented code suggests modelmesh support might be needed. Either implement it or remove the commented code.
Is modelmesh support required for these multi-namespace tests? If not, please remove the commented code. If yes, implement the check.
| verify_trustyai_service_metric_delete_request( | ||
| client=admin_client, | ||
| trustyai_service=tai, | ||
| token=current_client_token, | ||
| metric_name=TrustyAIServiceMetrics.Drift.MEANSHIFT, | ||
| ) |
There was a problem hiding this comment.
Fix indentation error in function call
The function call has incorrect indentation that will cause a syntax error.
verify_trustyai_service_metric_delete_request(
- client=admin_client,
- trustyai_service=tai,
- token=current_client_token,
- metric_name=TrustyAIServiceMetrics.Drift.MEANSHIFT,
+ client=admin_client,
+ trustyai_service=tai,
+ token=current_client_token,
+ metric_name=TrustyAIServiceMetrics.Drift.MEANSHIFT,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| verify_trustyai_service_metric_delete_request( | |
| client=admin_client, | |
| trustyai_service=tai, | |
| token=current_client_token, | |
| metric_name=TrustyAIServiceMetrics.Drift.MEANSHIFT, | |
| ) | |
| verify_trustyai_service_metric_delete_request( | |
| client=admin_client, | |
| trustyai_service=tai, | |
| token=current_client_token, | |
| metric_name=TrustyAIServiceMetrics.Drift.MEANSHIFT, | |
| ) |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 218-218: continuation line missing indentation or outdented
(E122)
[error] 219-219: continuation line missing indentation or outdented
(E122)
[error] 220-220: continuation line missing indentation or outdented
(E122)
[error] 221-221: continuation line missing indentation or outdented
(E122)
🤖 Prompt for AI Agents
In
tests/model_explainability/trustyai_service/service/multi_ns/test_trustyai_service_multi_ns.py
around lines 217 to 222, the function call to
verify_trustyai_service_metric_delete_request is incorrectly indented, causing a
syntax error. Adjust the indentation so that the function name and its
parameters align properly, ensuring consistent indentation for all arguments
within the call.
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/verified', '/lgtm', '/cherry-pick', '/build-push-pr-image', '/wip'} |
|
/wip |
1 participant
Description
How Has This Been Tested?
Merge criteria: