Add HAP detectors to existing test cases#508
Add HAP detectors to existing test cases#508sheltoncyril merged 3 commits intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds two pytest fixtures to provision a HAP detector InferenceService and its Route, extends guardrails tests with a multi-detector (prompt_injection + hap) test class and related assertions, and adds a MinIO PodConfig constant for a QWEN HAP/BPIV2 test image. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
/wip |
kpunwatk
changed the title
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/lgtm', '/build-push-pr-image', '/wip', '/verified', '/cherry-pick'} |
|
/wip |
adolfo-ab
left a comment
adolfo-ab
left a comment
There was a problem hiding this comment.
I think we need to split into several scenarios, for example:
-
Unsuitable input:
1.1. Prompt that triggers all the detectors at the same time (HAP, prompt injection, pii), assert that the three detectors are triggered.
1.2. Prompt that triggers a single detector (just HAP, for example), assert that only that detector is triggered, and not the other 2 -
Unsuitable output
2.1. Prompt that triggers a single detection at the output (you can use the same one we use for PII output detection in theTestGuardrailsOrchestratorWithBuiltInDetectorstest class) -
No detections (a test with a harmless prompt, assert that no detections are triggered)
kpunwatk
force-pushed
the
several_detectors
branch
from
7911027 to
c323e8b
Compare
kpunwatk
force-pushed
the
several_detectors
branch
from
4658f40 to
d7d4745
Compare
|
/wip cancel |
kpunwatk
changed the title
dbasunag
left a comment
dbasunag
left a comment
There was a problem hiding this comment.
Please rebase and see if tests can be split or simplified.
![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 (1)
tests/model_explainability/guardrails/conftest.py (1)
343-353: Optional: The Route for the HAP detector may be unnecessaryThese tests route traffic via the Guardrails Orchestrator using internal Service names (hap-detector-predictor). The detector Routes are not used in the requests themselves (only pulled in as fixtures to ensure readiness). If not needed elsewhere, consider dropping the detector Route resources to reduce cluster surface and flake potential.
-@pytest.fixture(scope="class") -def hap_detector_route( - admin_client: DynamicClient, - model_namespace: Namespace, - hap_detector_isvc: InferenceService, -) -> Generator[Route, Any, Any]: - yield Route( - name="hap-detector-route", - namespace=model_namespace.name, - service=hap_detector_isvc.name, - wait_for_resource=True, - ) +# If external exposure is not required, you can safely remove this fixture.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_explainability/guardrails/conftest.py(1 hunks)tests/model_explainability/guardrails/test_guardrails.py(2 hunks)utilities/constants.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 Learning: 2025-06-11T16:40:11.593Z
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.
Applied to files:
tests/model_explainability/guardrails/conftest.py
🔇 Additional comments (3)
utilities/constants.py (1)
318-324: Add HAP/BPIV2 MinIO image config — consistent with existing patternThe new QWEN_HAP_BPIV2_MINIO_CONFIG mirrors the existing image+sha256 pinning and reuses MINIO_BASE_CONFIG. This slots in cleanly with other MinIO presets.
tests/model_explainability/guardrails/conftest.py (1)
311-339: HAP detector InferenceService fixture looks correct and aligns with existing prompt-injection fixture
- Reuses the same HF ServingRuntime and creates a dedicated isvc with a clear name, pinned resources, and auth disabled.
- Good call on wait_for_predictor_pods=False since create_isvc already waits for READY=True. Leveraging the existing helper avoids redundant waits. I’m explicitly using the retrieved learning here.
No changes required from my side.
tests/model_explainability/guardrails/test_guardrails.py (1)
325-367: Paramset uses QWEN_HAP_BPIV2_MINIO_CONFIG — good alignment with new MinIO presetThis parametrization correctly switches to MinIo.PodConfig.QWEN_HAP_BPIV2_MINIO_CONFIG for the multi-detector scenario. The orchestrator config also registers both services (prompt-injection-detector-predictor and hap-detector-predictor), matching the fixture names.
No changes needed.
kpunwatk
force-pushed
the
several_detectors
branch
2 times, most recently
from
5a54272 to
506fcbf
Compare
|
Hi @adolfo-ab @sheltoncyril @dbasunag please review again, relevant of the changes are updated, Thanks! |
kpunwatk
force-pushed
the
several_detectors
branch
from
7cb77cf to
50ff02c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/model_explainability/guardrails/test_guardrails.py (2)
375-385: Docstring grammar and clarityTighten wording and fix plural agreement.
- These tests verify that the GuardrailsOrchestrator works as expected when using two HuggingFace detectors - (prompt injection and hap). + These tests verify that the GuardrailsOrchestrator works as expected when using two HuggingFace detectors + (prompt injection and HAP). @@ - - Deploy a prompt injection detector and HAP detectors using the HuggingFace SR. - - Check that the detectors works when we have an unsuitable input. - - Check that the detector works when we have a harmless input (no detection). + - Deploy a Prompt Injection detector and a HAP detector using the HuggingFace SR. + - Check that the detectors work when we have an unsuitable input. + - Check that the detectors work when we have a harmless input (no detection).
404-414: Add timeouts to HTTP calls to avoid indefinite hangs in CIrequests.post defaults to no timeout; a slow or stuck route will stall the test. Add a reasonable timeout (e.g., 30–60s) to each request in this suite.
response_prompt = requests.post( url=f"https://{guardrails_orchestrator_route.host}/{CHAT_COMPLETIONS_DETECTION_ENDPOINT}", headers=get_auth_headers(token=current_client_token), json=get_chat_detections_payload( content=prompt_injection, model=MNT_MODELS, detectors=HF_DETECTORS, ), - verify=openshift_ca_bundle_file, + verify=openshift_ca_bundle_file, + timeout=Timeout.TIMEOUT_1MIN, ) @@ response_hap = requests.post( url=f"https://{guardrails_orchestrator_route.host}/{CHAT_COMPLETIONS_DETECTION_ENDPOINT}", headers=get_auth_headers(token=current_client_token), json=get_chat_detections_payload( content=hap_prompt, model=MNT_MODELS, detectors=HF_DETECTORS, ), - verify=openshift_ca_bundle_file, + verify=openshift_ca_bundle_file, + timeout=Timeout.TIMEOUT_1MIN, ) @@ response = requests.post( url=f"https://{guardrails_orchestrator_route.host}/{CHAT_COMPLETIONS_DETECTION_ENDPOINT}", headers=get_auth_headers(token=current_client_token), json=get_chat_detections_payload(content=HARMLESS_PROMPT, model=MNT_MODELS, detectors=HF_DETECTORS), - verify=openshift_ca_bundle_file, + verify=openshift_ca_bundle_file, + timeout=Timeout.TIMEOUT_1MIN, )Also applies to: 424-433, 453-458
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
tests/model_explainability/guardrails/conftest.py(1 hunks)tests/model_explainability/guardrails/test_guardrails.py(2 hunks)utilities/constants.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_explainability/guardrails/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🧬 Code Graph Analysis (1)
tests/model_explainability/guardrails/test_guardrails.py (4)
utilities/constants.py (3)
MinIo(272-332)PodConfig(255-262)PodConfig(288-328)tests/conftest.py (3)
current_client_token(82-83)minio_pod(490-530)minio_data_connection(556-568)tests/model_explainability/guardrails/conftest.py (4)
guardrails_orchestrator_route(104-113)prompt_injection_detector_route(205-215)hap_detector_route(343-353)guardrails_orchestrator(34-60)tests/model_explainability/guardrails/utils.py (4)
get_auth_headers(11-12)get_chat_detections_payload(15-40)verify_builtin_detector_unsuitable_input_response(110-148)verify_negative_detection_response(190-226)
🔇 Additional comments (2)
utilities/constants.py (1)
318-324: New MinIO image config for HAP/BPIV2 — LGTMThe constant is consistent with existing patterns (digest-pinned image, MINIO_BASE_CONFIG merge). No issues spotted.
tests/model_explainability/guardrails/test_guardrails.py (1)
36-46: Good split between single- and multi-detector constantsThis preserves backward compatibility for single-detector HF tests while enabling the multi-detector scenarios. Naming is clear.
kpunwatk
force-pushed
the
several_detectors
branch
from
35f9f0c to
85bbeae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_explainability/guardrails/test_guardrails.py (1)
331-371: Avoid namespace collision with single-detector HF testsBoth suites use the same model namespace ("test-guardrails-huggingface"); this risks resource conflicts/flakiness when run together. Use a distinct namespace for the multi-detector suite.
Apply this diff:
- {"name": "test-guardrails-huggingface"}, + {"name": "test-guardrails-huggingface-multi"},
🧹 Nitpick comments (2)
utilities/constants.py (1)
318-321: Nit: collapse split string literals for readabilityInline the digest into the image string to avoid relying on implicit string literal concatenation.
Apply this diff:
- QWEN_HAP_BPIV2_MINIO_CONFIG: dict[str, Any] = { - "image": "quay.io/trustyai_testing/qwen2.5-0.5b-instruct-hap-bpiv2-minio@" - "sha256:eac1ca56f62606e887c80b4a358b3061c8d67f0b071c367c0aa12163967d5b2b", + QWEN_HAP_BPIV2_MINIO_CONFIG: dict[str, Any] = { + "image": "quay.io/trustyai_testing/qwen2.5-0.5b-instruct-hap-bpiv2-minio@sha256:eac1ca56f62606e887c80b4a358b3061c8d67f0b071c367c0aa12163967d5b2b",tests/model_explainability/guardrails/test_guardrails.py (1)
375-385: Docstring grammar nitsFix subject-verb agreement and clarify that both detectors are exercised.
Apply this diff:
class TestGuardrailsOrchestratorWithSeveralDetectors: """ - These tests verify that the GuardrailsOrchestrator works as expected when using two HuggingFace detectors - (prompt injection and hap). + These tests verify that the GuardrailsOrchestrator works as expected when using two HuggingFace detectors + (prompt injection and HAP). Steps: - Deploy an LLM (Qwen2.5-0.5B-Instruct) using the vLLM SR. - Deploy the GuardrailsOrchestrator. - - Deploy a prompt injection detector and HAP detectors using the HuggingFace SR. - - Check that the detectors works when we have an unsuitable input. - - Check that the detector works when we have a harmless input (no detection). + - Deploy prompt injection and HAP detectors using the HuggingFace SR. + - Check that the detectors work for unsuitable input. + - Check that the detectors return no detection for harmless input. """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
tests/model_explainability/guardrails/conftest.py(1 hunks)tests/model_explainability/guardrails/test_guardrails.py(2 hunks)utilities/constants.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 Learning: 2025-06-11T16:40:11.593Z
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.
Applied to files:
tests/model_explainability/guardrails/conftest.py
🧬 Code Graph Analysis (2)
tests/model_explainability/guardrails/conftest.py (2)
utilities/inference_utils.py (1)
create_isvc(547-761)utilities/constants.py (1)
KServeDeploymentType(6-9)
tests/model_explainability/guardrails/test_guardrails.py (4)
tests/conftest.py (3)
current_client_token(82-83)minio_pod(490-530)minio_data_connection(556-568)tests/model_explainability/conftest.py (1)
qwen_isvc(181-204)tests/model_explainability/guardrails/conftest.py (6)
guardrails_orchestrator_route(104-113)prompt_injection_detector_route(205-215)hap_detector_route(343-353)openshift_ca_bundle_file(220-223)orchestrator_config(64-73)guardrails_orchestrator(34-60)tests/model_explainability/guardrails/utils.py (4)
get_auth_headers(11-12)get_chat_detections_payload(15-40)verify_builtin_detector_unsuitable_input_response(110-148)verify_negative_detection_response(190-226)
🔇 Additional comments (7)
utilities/constants.py (1)
318-324: Add pinned HAP MinIO image config — LGTMThe new QWEN_HAP_BPIV2_MINIO_CONFIG is consistent with existing MinIO configs and correctly pins the image by digest. This enables reproducible test environments for the new HAP detector scenarios.
tests/model_explainability/guardrails/conftest.py (3)
311-339: HAP detector InferenceService fixture — LGTMParameters mirror the prompt-injection detector, using the HF runtime and MinIO storage as expected. Auth disabled and 1x replica sizing is appropriate for tests.
325-327: Verify MinIO storage path exists in the new imageEnsure the path "granite-guardian-hap-38m" exists in the MinIO bucket provided by QWEN_HAP_BPIV2_MINIO_CONFIG; otherwise, the detector will fail to load.
Would you like me to add a preflight check in the fixture that probes MinIO for the path and skips the test gracefully if missing?
342-353: Route for HAP detector — LGTMThe Route wiring to the detector service aligns with the pattern used for the prompt-injection detector and should work as intended.
tests/model_explainability/guardrails/test_guardrails.py (3)
36-46: Detector constants split is correct and clear — LGTMKeeping PROMPT_INJECTION_DETECTORS for single-detector tests and introducing HF_DETECTORS for the dual-detector suite preserves backward compatibility and clarity.
401-421: Good use of a loop to exercise both detectorsThe looped POST + verification reduces duplication and keeps both detectors’ assertions consistent.
442-449: Negative detection payload correctly uses multi-detector configurationThe harmless prompt uses HF_DETECTORS, ensuring both detectors contribute to the “no detections” assertion.
modified: tests/model_explainability/guardrails/conftest.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: utilities/constants.py modified: tests/model_explainability/guardrails/conftest.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: utilities/constants.py modified: tests/model_explainability/guardrails/conftest.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: utilities/constants.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: tests/model_explainability/guardrails/test_guardrails.py
kpunwatk
force-pushed
the
several_detectors
branch
from
5aa9b6d to
dfcf95d
Compare
sheltoncyril
dismissed
adolfo-ab’s stale review
All comments have been addressed, just looking to merge as requested by @kpunwatk
|
Status of building tag latest: success. |
* Add HAP detectors to existing test cases modified: tests/model_explainability/guardrails/conftest.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: utilities/constants.py modified: tests/model_explainability/guardrails/conftest.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: utilities/constants.py modified: tests/model_explainability/guardrails/conftest.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: utilities/constants.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: tests/model_explainability/guardrails/test_guardrails.py modified: tests/model_explainability/guardrails/test_guardrails.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
5 participants
Addresses: https://issues.redhat.com/browse/RHOAIENG-28245
This PR introduces HAP (Hate speech and profanity) detectors to existing test cases within the guardrail tests.
Description
How Has This Been Tested?
Merge criteria: