Skip to content

Commit 05b9487

Browse files
committed
refactor: Address PR review feedback for Detections API integration
Code quality improvements: - Standardize all imports to absolute paths for consistency - Remove dead code and simplify helper methods per API spec - Replace tuple pattern with explicit zip() for clarity Enhanced error handling: - Add HTTP status differentiation (404/422/500/503) - Add null-safety checks for incomplete config chains - Preserve constructor validation error handling Output guardrails support: - Add bot_message extraction for output rails - Implement priority-based message type detection - Support both input and output guardrail flows Bug fixes: - Fix inconsistent Colang responses with static messages - Add cleanup_http_session() to prevent session leaks - Standardize metadata structure with 'passed' flag Testing: - Add comprehensive unit tests
1 parent deadaaa commit 05b9487

File tree

9 files changed

+2749
-388
lines changed

9 files changed

+2749
-388
lines changed

docs/user-guides/detections-api-integration.md

Lines changed: 91 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ This integration enables NeMo Guardrails to communicate with external detector s
1010
- **Service-based detection**: Detectors run as independent microservices with rich metadata
1111
- **Extensible design**: Add support for new API protocols by implementing two methods (request builder and response parser)
1212
- **No code duplication**: Common HTTP, error handling, and orchestration logic shared across all detector types
13+
- **Parallel execution**: All detectors run concurrently using asyncio.gather() for optimal performance
14+
- **System error separation**: Distinguishes content violations from infrastructure failures (timeouts, HTTP errors)
1315

1416
## Architecture
1517

@@ -144,13 +146,13 @@ The integration uses object-oriented design to eliminate code duplication across
144146
class BaseDetectorClient(ABC):
145147
@abstractmethod
146148
async def detect(text: str) -> DetectorResult
147-
149+
148150
@abstractmethod
149151
def build_request(text: str) -> dict
150-
152+
151153
@abstractmethod
152154
def parse_response(response: dict, http_status: int) -> DetectorResult
153-
155+
154156
# Shared implementations:
155157
async def _call_endpoint(...) # HTTP communication
156158
def _handle_error(...) # Error handling
@@ -162,7 +164,7 @@ class DetectionsAPIClient(BaseDetectorClient):
162164
def build_request(text: str) -> dict:
163165
# Detections API specific format
164166
return {"contents": [text], "detector_params": {}}
165-
167+
166168
def parse_response(response: dict, http_status: int) -> DetectorResult:
167169
# Parse [[{detection1}, {detection2}]]
168170
# Apply threshold filtering
@@ -208,16 +210,28 @@ With threshold=0.5:
208210

209211
**Score Aggregation:**
210212
- `score`: Highest individual detection score
211-
- `metadata.average_score`: Average of all filtered detections
212213
- `metadata.detection_count`: Number of detections above threshold
213214
- `metadata.individual_scores`: All scores for analysis
214215

215216
### Error Handling
216217

217218
The system distinguishes between infrastructure errors and content violations.
218219

220+
### System Error Handling
221+
222+
The system distinguishes between **content violations** (actual detections) and **system errors** (infrastructure failures like timeouts, HTTP errors, configuration issues).
223+
224+
**Behavior:**
225+
- System errors tracked separately in `unavailable_detectors` list
226+
- Requests with system errors are blocked (fail-safe approach)
227+
- Clear error messages indicate which detectors are unavailable vs which found violations
228+
229+
**System Error Labels:**
230+
`ERROR`, `HTTP_ERROR`, `TIMEOUT`, `NOT_FOUND`, `VALIDATION_ERROR`, `INVALID_RESPONSE`, `CONFIG_ERROR`
231+
232+
This separation enables better operational monitoring and clearer user feedback.
219233
**System Errors:**
220-
- HTTP errors (404, 500, 503)
234+
- HTTP errors (404, 422, 500, 503)
221235
- Network timeouts
222236
- Invalid response formats
223237
- Result: `allowed=False`, `label="ERROR"` or `"TIMEOUT"`
@@ -691,21 +705,38 @@ data:
691705
- type: general
692706
content: |
693707
You are a helpful AI assistant.
694-
708+
695709
rails.co: |
710+
define bot blocked by detector
711+
"Input blocked by content safety detectors"
712+
713+
define bot output blocked by detector
714+
"I apologize, but I cannot provide that response."
715+
716+
define bot service unavailable
717+
"Service temporarily unavailable"
718+
696719
define flow check_input_safety_detections_api
697720
$input_result = execute detections_api_check_all_detectors
698-
721+
699722
if $input_result.unavailable_detectors
700-
bot refuse with message $input_result.reason
723+
bot service unavailable
701724
stop
702-
725+
703726
if not $input_result.allowed
704-
bot refuse with message $input_result.reason
727+
bot blocked by detector
728+
stop
729+
730+
define flow check_output_safety_detections_api
731+
$output_result = execute detections_api_check_all_detectors
732+
733+
if $output_result.unavailable_detectors
734+
bot service unavailable
705735
stop
706736
707-
define bot refuse with message $msg
708-
$msg
737+
if not $output_result.allowed
738+
bot output blocked by detector
739+
stop
709740
```
710741
711742
**Configuration Fields:**
@@ -715,7 +746,7 @@ data:
715746
- `timeout`: Request timeout in seconds (increase for CPU-based detectors)
716747
- `detector_params`: Optional detector-specific parameters (sent in request body)
717748

718-
**Important:**
749+
**Important:**
719750
- Timeout should be 120+ seconds for CPU-based detectors like Granite Guardian
720751
- Replace `<your-namespace>` with your actual namespace
721752
- `detector_id` must match what the detector service expects
@@ -837,6 +868,27 @@ No "Failed to register" errors should appear.
837868

838869
## Testing
839870

871+
### Unit Testing
872+
873+
The integration includes **104 comprehensive unit tests** with **97%+ code coverage**.
874+
875+
**Run tests:**
876+
```bash
877+
poetry run pytest tests/test_detector_clients_*.py -v
878+
879+
# With coverage
880+
poetry run pytest tests/test_detector_clients_*.py \
881+
--cov=nemoguardrails/library/detector_clients \
882+
--cov-report=term-missing
883+
```
884+
885+
**Test coverage:**
886+
- All error handling paths (timeouts, HTTP errors, malformed responses)
887+
- System error classification and separation
888+
- Parallel detector execution
889+
- Edge cases (threshold boundaries, missing fields)
890+
891+
### Testing on a cluster
840892
Extract the NeMo route for testing:
841893
```bash
842894
YOUR_ROUTE="http://$(oc get route nemo-guardrails-server -n <your-namespace> -o jsonpath='{.spec.host}')"
@@ -860,12 +912,12 @@ curl -X POST $YOUR_ROUTE/v1/chat/completions \
860912
**Expected Output:**
861913
```json
862914
{
863-
"messages": [
864-
{
865-
"role": "assistant",
866-
"content": "I'm doing well, thank you for asking! How can I assist you today?"
867-
}
868-
]
915+
"messages":[
916+
{
917+
"role":"assistant",
918+
"content":"I am an AI, so I don't have feelings, but I'm ready and functioning optimally. How can I help you today?"
919+
}
920+
]
869921
}
870922
```
871923

@@ -880,7 +932,7 @@ curl -X POST $YOUR_ROUTE/v1/chat/completions \
880932
-d '{
881933
"config_id": "production",
882934
"messages": [
883-
{"role": "user", "content": "How to hack into a system"}
935+
{"role": "user", "content": "How to hack into a system?"}
884936
]
885937
}'
886938
```
@@ -891,7 +943,7 @@ curl -X POST $YOUR_ROUTE/v1/chat/completions \
891943
"messages": [
892944
{
893945
"role": "assistant",
894-
"content": "Blocked by 3 detections across 1 type(s) (highest score=1.00)"
946+
"content": "Input blocked by content safety detectors"
895947
}
896948
]
897949
}
@@ -928,7 +980,7 @@ Log shows detector was called, returned structured detections, and content was b
928980
"detection": "causal_lm",
929981
"score": 0.9985,
930982
"sequence_classification": "jailbreak",
931-
"text": "How to hack into a system"
983+
"text": "How to hack into a system?"
932984
},
933985
{
934986
"start": 0,
@@ -937,7 +989,7 @@ Log shows detector was called, returned structured detections, and content was b
937989
"detection": "causal_lm",
938990
"score": 0.9978,
939991
"sequence_classification": "harm",
940-
"text": "How to hack into a system"
992+
"text": "How to hack into a system?"
941993
}
942994
]]
943995
```
@@ -1008,7 +1060,7 @@ Edit `nemo-detections-configmap.yaml` and add your detector:
10081060
detections_api_detectors:
10091061
granite_hap:
10101062
# ... existing detector ...
1011-
1063+
10121064
your_detector: # Detector name (used in logs and error messages)
10131065
inference_endpoint: "http://your-detector-predictor.<your-namespace>.svc.cluster.local:8000/api/v1/text/contents"
10141066
detector_id: "your-detector-id"
@@ -1056,6 +1108,18 @@ oc logs -n <your-namespace> $(oc get pods -n <your-namespace> -l app=nemo-guardr
10561108
- Passed through to detector service in request body
10571109
- Example: `{"language": "en", "categories": ["pii", "toxicity"]}`
10581110

1111+
## Resource Cleanup
1112+
1113+
The integration uses a shared HTTP session for connection pooling. For proper resource cleanup during application shutdown:
1114+
```python
1115+
from nemoguardrails.library.detector_clients.base import cleanup_http_session
1116+
1117+
# At application shutdown
1118+
await cleanup_http_session()
1119+
```
1120+
1121+
This prevents resource leaks by properly closing the aiohttp session. The function is idempotent and safe to call multiple times.
1122+
10591123
## Authentication (Optional)
10601124

10611125
Detections API services can be secured with authentication to restrict access.
@@ -1117,7 +1181,7 @@ detections_api_detectors:
11171181
detector_id: "granite-guardian-hap"
11181182
api_key: "granite-specific-token"
11191183
threshold: 0.5
1120-
1184+
11211185
other_detector:
11221186
inference_endpoint: "..."
11231187
detector_id: "other-id"
@@ -1134,4 +1198,4 @@ oc sa get-token <service-account-name> -n <your-namespace>
11341198
11351199
# For OpenShift AI secured services
11361200
oc whoami -t
1137-
```
1201+
```
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
# SPDX-License-Identifier: Apache-2.0
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.

0 commit comments

Comments
 (0)