feat: Added drift tests using DB storage#400
feat: Added drift tests using DB storage#400sheltoncyril merged 2 commits intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughA new test class, Changes
Possibly related PRs
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{'/cherry-pick', '/lgtm', '/wip', '/hold', '/verified', '/build-push-pr-image'} |
![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 (1)
tests/model_explainability/trustyai_service/drift/test_drift.py (1)
156-178: LGTM! Well-structured test class for DB storage.The new test class properly mirrors the existing PVC storage tests with appropriate fixture usage and comprehensive metric coverage.
Minor note: The docstring mentions "Apply name mappings" in step 2, but unlike the fairness tests, there's no explicit name mapping test method in this class.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_explainability/trustyai_service/drift/test_drift.py(1 hunks)tests/model_explainability/trustyai_service/fairness/test_fairness.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
tests/model_explainability/trustyai_service/fairness/test_fairness.py (1)
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_explainability/trustyai_service/drift/test_drift.py (1)
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.
🪛 Pylint (3.3.7)
tests/model_explainability/trustyai_service/drift/test_drift.py
[refactor] 180-180: Too many arguments (7/5)
(R0913)
[refactor] 180-180: Too many positional arguments (7/5)
(R0917)
[refactor] 214-214: Too many arguments (6/5)
(R0913)
[refactor] 214-214: Too many positional arguments (6/5)
(R0917)
[refactor] 234-234: Too many arguments (6/5)
(R0913)
[refactor] 234-234: Too many positional arguments (6/5)
(R0917)
[refactor] 254-254: Too many arguments (7/5)
(R0913)
[refactor] 254-254: Too many positional arguments (7/5)
(R0917)
[refactor] 271-271: Too many arguments (6/5)
(R0913)
[refactor] 271-271: Too many positional arguments (6/5)
(R0917)
🔇 Additional comments (2)
tests/model_explainability/trustyai_service/fairness/test_fairness.py (1)
168-172: LGTM! Documentation accurately reflects test coverage.The docstring updates correctly describe that the tests cover both "spd and dir" fairness metrics, which aligns with the
FAIRNESS_METRICSlist and parametrized test methods.tests/model_explainability/trustyai_service/drift/test_drift.py (1)
180-284: Comprehensive test coverage with consistent patterns.All test methods follow established conventions:
- Proper
_with_db_storagenaming suffix- Correct fixture usage (
trustyai_service_with_db_storage)- Complete parametrization across all drift metrics
- Consistent test flow and data handling
The static analysis warnings about "too many arguments" are false positives - pytest test methods commonly use many fixtures.
kpunwatk
changed the title
|
/verified |
kpunwatk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/model_explainability/trustyai_service/drift/test_drift.py (2)
168-178: Consider the docstring accuracy and missing test coverage.The docstring mentions "Apply name mappings" as step 2, but there's no dedicated test method for name mapping functionality in this class. Either add the missing test or update the docstring to accurately reflect the actual test coverage.
""" Verifies all the basic operations with a drift metric (meanshift, kstest, approxkstest and fouriermmd) available in TrustyAI, using MariaDB storage. 1. Send data to the model and verify that TrustyAI registers the observations. -2. Apply name mappings -3. Send metric request (meanshift, kstest, approxkstest and fouriermmd) and verify the response. -4. Send metric scheduling request and verify the response. -5. Send metric deletion request and verify that the scheduled metric has been deleted. +2. Upload training data to TrustyAI service. +3. Send metric request (meanshift, kstest, approxkstest and fouriermmd) and verify the response. +4. Send metric scheduling request and verify the response. +5. Validate Prometheus metrics are exposed correctly. +6. Send metric deletion request and verify that the scheduled metric has been deleted. """
156-285: Consider reducing code duplication with a parameterized approach.The new
TestDriftMetricsWithDBStorageclass duplicates the entire test logic fromTestDriftMetrics, differing only in the storage fixture used. This creates maintenance overhead as changes need to be applied to both classes.Consider consolidating both classes into a single parameterized test class:
+@pytest.mark.parametrize("storage_type", ["pvc", "db"]) @pytest.mark.parametrize( "model_namespace, minio_pod, minio_data_connection", [ pytest.param( {"name": "test-drift"}, MinIo.PodConfig.MODEL_MESH_MINIO_CONFIG, {"bucket": MinIo.Buckets.MODELMESH_EXAMPLE_MODELS}, ) ], indirect=True, ) @pytest.mark.usefixtures("minio_pod") @pytest.mark.smoke -class TestDriftMetrics: +class TestDriftMetrics: """ - Verifies all the basic operations with a drift metric (meanshift) available in TrustyAI, using PVC storage. + Verifies all the basic operations with drift metrics available in TrustyAI, using PVC or DB storage. """ + @pytest.fixture + def trustyai_service(self, storage_type, trustyai_service_with_pvc_storage, trustyai_service_with_db_storage): + return trustyai_service_with_pvc_storage if storage_type == "pvc" else trustyai_service_with_db_storage + def test_drift_send_inference_and_verify_trustyai_service( self, admin_client, current_client_token, model_namespace, - trustyai_service_with_pvc_storage, + trustyai_service, gaussian_credit_model, isvc_getter_token, ) -> None:This approach would eliminate duplication while maintaining the same test coverage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_explainability/trustyai_service/drift/test_drift.py(1 hunks)tests/model_explainability/trustyai_service/fairness/test_fairness.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_explainability/trustyai_service/fairness/test_fairness.py
🧰 Additional context used
🧠 Learnings (1)
tests/model_explainability/trustyai_service/drift/test_drift.py (1)
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.
🪛 Pylint (3.3.7)
tests/model_explainability/trustyai_service/drift/test_drift.py
[refactor] 180-180: Too many arguments (7/5)
(R0913)
[refactor] 180-180: Too many positional arguments (7/5)
(R0917)
[refactor] 214-214: Too many arguments (6/5)
(R0913)
[refactor] 214-214: Too many positional arguments (6/5)
(R0917)
[refactor] 234-234: Too many arguments (6/5)
(R0913)
[refactor] 234-234: Too many positional arguments (6/5)
(R0917)
[refactor] 254-254: Too many arguments (7/5)
(R0913)
[refactor] 254-254: Too many positional arguments (7/5)
(R0917)
[refactor] 271-271: Too many arguments (6/5)
(R0913)
[refactor] 271-271: Too many positional arguments (6/5)
(R0917)
🔇 Additional comments (1)
tests/model_explainability/trustyai_service/drift/test_drift.py (1)
180-188: Test method structure follows established patterns.The test method correctly uses the database storage fixture and maintains the same parameter structure as the PVC storage equivalent. The implementation properly delegates to the shared utility function.
modified: tests/model_explainability/trustyai_service/drift/test_drift.py modified: tests/model_explainability/trustyai_service/fairness/test_fairness.py modified: tests/model_explainability/trustyai_service/drift/test_drift.py modified: tests/model_explainability/trustyai_service/fairness/test_fairness.py
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/model_explainability/trustyai_service/drift/test_drift.py (2)
168-168: Add smoke test marker for consistency.The original
TestDriftMetricsclass includes@pytest.mark.smokebut this new DB storage test class doesn't. Consider adding it for consistency in test categorization.@pytest.mark.usefixtures("minio_pod") +@pytest.mark.smoke class TestDriftMetricsWithDBStorage:
169-178: Update docstring to accurately reflect test coverage.The docstring mentions "Apply name mappings" as step 2, but there's no specific test method for name mappings in this class. Consider removing this step or adding the corresponding test method.
""" Verifies all the basic operations with a drift metric (meanshift, kstest, approxkstest and fouriermmd) - available in TrustyAI, using MariaDB storage. + available in TrustyAI, using MariaDB storage. 1. Send data to the model and verify that TrustyAI registers the observations. - 2. Apply name mappings - 3. Send metric request (meanshift, kstest, approxkstest and fouriermmd) and verify the response. - 4. Send metric scheduling request and verify the response. - 5. Send metric deletion request and verify that the scheduled metric has been deleted. + 2. Send metric request (meanshift, kstest, approxkstest and fouriermmd) and verify the response. + 3. Send metric scheduling request and verify the response. + 4. Send metric deletion request and verify that the scheduled metric has been deleted. """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_explainability/trustyai_service/drift/test_drift.py(1 hunks)tests/model_explainability/trustyai_service/fairness/test_fairness.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_explainability/trustyai_service/fairness/test_fairness.py
🧰 Additional context used
🧠 Learnings (1)
tests/model_explainability/trustyai_service/drift/test_drift.py (1)
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_explainability/trustyai_service/drift/test_drift.py (5)
utilities/constants.py (3)
MinIo(242-290)PodConfig(258-286)Buckets(254-256)tests/conftest.py (5)
admin_client(55-56)current_client_token(71-72)model_namespace(87-107)minio_data_connection(478-490)prometheus(523-531)tests/model_explainability/trustyai_service/conftest.py (3)
trustyai_service_with_db_storage(97-111)gaussian_credit_model(286-324)isvc_getter_token(371-372)tests/model_explainability/trustyai_service/trustyai_service_utils.py (5)
send_inferences_and_verify_trustyai_service_registered(272-336)verify_upload_data_to_trustyai_service(519-551)verify_trustyai_service_metric_request(434-462)verify_trustyai_service_metric_scheduling_request(465-516)verify_trustyai_service_metric_delete_request(554-595)utilities/monitoring.py (2)
validate_metrics_field(53-88)get_metric_label(27-50)
🪛 Pylint (3.3.7)
tests/model_explainability/trustyai_service/drift/test_drift.py
[refactor] 180-180: Too many arguments (7/5)
(R0913)
[refactor] 180-180: Too many positional arguments (7/5)
(R0917)
[refactor] 214-214: Too many arguments (6/5)
(R0913)
[refactor] 214-214: Too many positional arguments (6/5)
(R0917)
[refactor] 234-234: Too many arguments (6/5)
(R0913)
[refactor] 234-234: Too many positional arguments (6/5)
(R0917)
[refactor] 254-254: Too many arguments (7/5)
(R0913)
[refactor] 254-254: Too many positional arguments (7/5)
(R0917)
[refactor] 271-271: Too many arguments (6/5)
(R0913)
[refactor] 271-271: Too many positional arguments (6/5)
(R0917)
🔇 Additional comments (1)
tests/model_explainability/trustyai_service/drift/test_drift.py (1)
156-284: Excellent implementation of DB storage drift tests.The new test class properly mirrors the existing PVC storage tests while using the appropriate
trustyai_service_with_db_storagefixture. The comprehensive coverage of all drift metrics (meanshift, kstest, approxkstest, fouriermmd) and their lifecycle operations (request, schedule, prometheus validation, delete) ensures thorough testing of the database storage backend.The code structure follows established patterns and correctly utilizes the existing utility functions. The method naming with
_with_db_storagesuffix provides clear differentiation from the PVC storage tests.Note: The pylint warnings about too many arguments are false positives - this is expected and acceptable for pytest test methods that require multiple fixtures.
|
Status of building tag latest: success. |
5 participants
This PR introduces tests for drift metrics including MeanShift, KSTest, ApproxKSTest, and FourierMMD using DB storage for the TrustyAIService.
The test class description in the Fairness test case has been updated to include the missing "dir" metric.
Description
How Has This Been Tested?
Merge criteria: