Add guardrails healthcheck to each test before sending out requests#1014
Conversation
sheltoncyril
requested review from
a team,
adolfo-ab and
kpunwatk
as code owners
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/verified', '/hold', '/lgtm', '/build-push-pr-image', '/wip'} |
📝 WalkthroughWalkthroughAdds a centralized Guardrails health-check utility with retry and auth headers, a pytest fixture that invokes it, refactors tests to use the fixture instead of explicit health-endpoint tests, updates detector utils error handling, and modifies an inference fixture to conditionally enable DSC "Headed" mode with guarded readiness waiting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)tests/fixtures/inference.py (2)
tests/model_explainability/guardrails/utils.py (2)
🪛 Ruff (0.14.11)tests/fixtures/inference.py173-173: Unused function argument: (ARG001) tests/model_explainability/guardrails/utils.py131-131: Avoid specifying long messages outside the exception class (TRY003) 137-137: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting 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. Comment |
![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
🤖 Fix all issues with AI agents
In @tests/fixtures/guardrails.py:
- Around line 164-172: The fixture guardrails_healthcheck ignores the boolean
returned by check_guardrails_health_endpoint so failures are silent; change the
fixture to assert the return value (e.g., assert
check_guardrails_health_endpoint(...) , "Guardrails healthcheck failed") or
raise a RuntimeError when it returns False, and also make the fixture scope
consistent by adding scope="class" to the @pytest.fixture decorator (unless the
per-test execution is intentional—if so, add a comment explaining why). Ensure
you reference guardrails_healthcheck and check_guardrails_health_endpoint when
making these changes.
In @tests/fixtures/inference.py:
- Line 212: The line using time.sleep(20) contains an invalid noqa directive "#
noqa: FCN001"; remove this invalid directive and either leave no noqa or replace
it with the correct linter code if you intentionally want to silence a
security/time-sleep warning (e.g., use Bandit's S113) — update the statement in
tests/fixtures/inference.py where time.sleep is called (the time.sleep(20)
invocation) accordingly so the file no longer references the unknown FCN001
code.
In @utilities/guardrails.py:
- Around line 12-19: Add a timeout to the requests.get call in
check_guardrails_health_endpoint and make non-200 responses raise a TimeoutError
so the @retry decorator can trigger; specifically update
check_guardrails_health_endpoint to call requests.get(..., timeout=some_seconds)
and if response.status_code is not http.HTTPStatus.OK raise a TimeoutError
(including response details) instead of returning False so transient failures
and hangs are retried.
🧹 Nitpick comments (1)
tests/model_explainability/guardrails/utils.py (1)
287-287: Consider documenting or aligning thesleepvalues across similar retry functions.The
sleepparameter forsend_and_verify_unsuitable_input_detectionwas increased to4seconds, while similar functions (send_and_verify_unsuitable_output_detection,send_and_verify_negative_detection,send_and_verify_standalone_detection) retainsleep=1. If this difference is intentional due to different endpoint characteristics, consider adding a brief comment explaining the rationale.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/fixtures/guardrails.pytests/fixtures/inference.pytests/model_explainability/guardrails/test_guardrails.pytests/model_explainability/guardrails/utils.pyutilities/guardrails.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/model_explainability/guardrails/utils.py (1)
utilities/guardrails.py (1)
get_auth_headers(8-9)
tests/fixtures/inference.py (2)
utilities/infra.py (2)
get_data_science_cluster(883-884)wait_for_dsc_status_ready(1063-1069)tests/conftest.py (1)
dsc_resource(491-492)
tests/fixtures/guardrails.py (3)
utilities/guardrails.py (1)
check_guardrails_health_endpoint(13-19)tests/conftest.py (1)
current_client_token(93-94)tests/model_explainability/guardrails/conftest.py (1)
openshift_ca_bundle_file(105-108)
🪛 Ruff (0.14.10)
utilities/guardrails.py
18-18: Probable use of requests call without timeout
(S113)
tests/fixtures/inference.py
172-172: Unused function argument: patched_dsc_kserve_headed
(ARG001)
212-212: Unused noqa directive (unknown: FCN001)
Remove unused noqa directive
(RUF100)
tests/model_explainability/guardrails/test_guardrails.py
146-146: Unused method argument: guardrails_healthcheck
(ARG002)
161-161: Unused method argument: guardrails_healthcheck
(ARG002)
178-178: Unused method argument: guardrails_healthcheck
(ARG002)
208-208: Unused method argument: guardrails_healthcheck
(ARG002)
322-322: Unused method argument: guardrails_healthcheck
(ARG002)
345-345: Unused method argument: guardrails_healthcheck
(ARG002)
366-366: Unused method argument: guardrails_healthcheck
(ARG002)
387-387: Unused method argument: guardrails_healthcheck
(ARG002)
438-438: Unused method argument: guardrails_healthcheck
(ARG002)
452-452: Unused method argument: guardrails_healthcheck
(ARG002)
470-470: Unused method argument: guardrails_healthcheck
(ARG002)
512-512: Unused method argument: hap_detector_isvc
(ARG002)
513-513: Unused method argument: prompt_injection_detector_isvc
(ARG002)
515-515: Unused method argument: guardrails_healthcheck
(ARG002)
528-528: Unused method argument: prompt_injection_detector_isvc
(ARG002)
529-529: Unused method argument: hap_detector_isvc
(ARG002)
531-531: Unused method argument: guardrails_healthcheck
(ARG002)
558-558: Unused method argument: prompt_injection_detector_isvc
(ARG002)
559-559: Unused method argument: hap_detector_isvc
(ARG002)
564-564: Unused method argument: guardrails_healthcheck
(ARG002)
🔇 Additional comments (7)
utilities/guardrails.py (1)
8-9: LGTM!Simple and reusable helper for constructing authorization headers.
tests/model_explainability/guardrails/utils.py (2)
10-10: LGTM!Good refactoring to centralize the
get_auth_headersutility inutilities/guardrails.py.
130-135: LGTM!Good defensive check handling the case where
warningsisNoneinstead of an empty list, preventing aTypeErroron the subsequentlen()call.tests/fixtures/inference.py (2)
172-172: This unused parameter warning is a false positive.The
patched_dsc_kserve_headedparameter is a pytest fixture dependency that ensures the DSC is configured for headed mode before theInferenceServiceis created. This is the idiomatic pytest pattern for declaring fixture dependencies.
203-218: No action required. The fixture is correctly scoped at the class level (@pytest.fixture(scope="class")), which means theResourceEditorpatch is applied once per test class and reverted after all tests in that class complete. This is the correct behavior for test isolation and is not a problem.Likely an incorrect or invalid review comment.
tests/model_explainability/guardrails/test_guardrails.py (2)
139-152: LGTM!The
guardrails_healthcheckfixture is correctly added as a dependency to ensure health verification runs before each test. The fixture parameter being "unused" in the test body is the standard pytest pattern for declaring fixture dependencies.
507-521: LGTM!The
hap_detector_isvcandprompt_injection_detector_isvcfixtures are correctly added as dependencies to ensure the HuggingFace detector services are deployed before the gateway tests run. This is the standard pytest fixture dependency pattern.
rhods-ci-bot
added
commented-by-sheltoncyril
and removed
changes-requested-by-dbasunag
commented-by-sheltoncyril
labels
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/fixtures/inference.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/fixtures/inference.py (2)
utilities/infra.py (2)
get_data_science_cluster(883-884)wait_for_dsc_status_ready(1063-1069)tests/conftest.py (2)
dsc_resource(491-492)admin_client(79-80)
🪛 Ruff (0.14.11)
tests/fixtures/inference.py
173-173: Unused function argument: patched_dsc_kserve_headed
(ARG001)
🔇 Additional comments (4)
tests/fixtures/inference.py (4)
15-29: LGTM!New imports and module-level logger are appropriately added and used within the fixture changes.
138-139: LGTM!Image pinned by SHA256 digest ensures reproducibility.
168-174: Intentional fixture dependency pattern.The static analysis warning (ARG001) about
patched_dsc_kserve_headedbeing unused is a false positive. This is a standard pytest pattern where the parameter declares a fixture dependency to ensure DSC is configured in Headed mode before this ISVC is created, even though the return value isn't directly used.
222-233: Good optimization to skip redundant patching.The guarded flow correctly avoids re-applying the patch when already in Headed mode. The transition time capture before entering
ResourceEditorcontext is the right approach to detect when the patch takes effect.
sheltoncyril
changed the title
|
/verified |
rhods-ci-bot
added
lgtm-by-dbasunag
and removed
Verified
|
Status of building tag latest: success. |
|
/cherry-pick 3.2 |
* feat: add guardrails healthcheck to each endpoint test * feat: remove time.sleep call
|
Cherry pick action created PR #1022 successfully 🎉! |
|
/cherry-pick 2.25 |
|
Error cherry-picking. |
|
|
…o#1014) * feat: add guardrails healthcheck to each endpoint test * feat: remove time.sleep call
4 participants
This PR addresses issues found with some tests where DSC updates were not caught after a patch and also adds a healthcheck of the guardrails resource before sending out requests.
Summary by CodeRabbit
Tests
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.