Added evaluation of multiple validators together#66
Added evaluation of multiple validators together#66rkritika1508 wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request introduces conditional validation logic in the guardrails API route that defers ban-list word resolution to only when required, and adds a new evaluation script that orchestrates guardrails-based validation over a WhatsApp dataset with API integration and CSV output. Changes
Sequence DiagramsequenceDiagram
participant Script as WhatsApp Eval Script
participant CSV as CSV Dataset
participant API as Guardrails API
participant Output as Output CSV
Script->>CSV: Load multi_validator_whatsapp_dataset.csv
CSV-->>Script: Dataset rows
loop For each row
Script->>Script: Build request payload with validators
Script->>API: POST /guardrails with input text + validators
API-->>Script: Response with safe_text
Script->>Script: Extract and store result
end
Script->>Output: Write results to predictions.csv
Output-->>Script: File written
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (2)
backend/app/api/routes/guardrails.py (1)
48-53: LGTM – the conditional guard is correct and well-motivated.The
any()short-circuit correctly avoids an unnecessary DB round-trip when allBanListSafetyValidatorConfigvalidators already havebanned_wordspopulated (as in the evaluation script, where they're inlined). The inner_resolve_ban_list_banned_wordsalso retains its own guard (banned_words is not None), so behavior is unchanged for callers that always suppliedbanned_words.One existing gap worth noting:
_resolve_ban_list_banned_wordsis invoked before_validate_with_guard'stry/except, so any exception fromban_list_crud.get(e.g. record not found, DB error) escapes unhandled and falls through to FastAPI's default 500 handler rather than returning a structuredAPIResponse. This pre-dates this PR, but wrapping the call — or moving ban-list resolution inside_validate_with_guard'stryblock — would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/guardrails.py` around lines 48 - 53, The call to _resolve_ban_list_banned_words(payload, session) can raise exceptions from ban_list_crud.get which escape the current try/except in _validate_with_guard; wrap the resolution call in the same error handling as _validate_with_guard (or move the call inside _validate_with_guard's try block) so DB errors are caught and converted to the same structured APIResponse/error handling path. Specifically, ensure exceptions from ban_list_crud.get (raised via _resolve_ban_list_banned_words) are caught and handled consistently with _validate_with_guard rather than propagating to FastAPI (reference symbols: _resolve_ban_list_banned_words, _validate_with_guard, ban_list_crud.get, payload.validators, BanListSafetyValidatorConfig).backend/app/evaluation/multi_validator_whatsapp/run.py (1)
16-16:float()conversion at module level will crash on a malformed env var.
TIMEOUT_SECONDS = float(os.getenv("GUARDRAILS_TIMEOUT_SECONDS", "60"))raisesValueErrorat import time if the variable is set to a non-numeric string (e.g. a mistyped value), giving a confusing traceback instead of a meaningful error.🔧 Proposed fix
-TIMEOUT_SECONDS = float(os.getenv("GUARDRAILS_TIMEOUT_SECONDS", "60")) +_raw_timeout = os.getenv("GUARDRAILS_TIMEOUT_SECONDS", "60") +try: + TIMEOUT_SECONDS = float(_raw_timeout) +except ValueError: + raise ValueError( + f"GUARDRAILS_TIMEOUT_SECONDS must be a number, got: {_raw_timeout!r}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/evaluation/multi_validator_whatsapp/run.py` at line 16, The module-level float conversion of TIMEOUT_SECONDS from GUARDRAILS_TIMEOUT_SECONDS can raise ValueError at import time; change it to a safe parse: read os.getenv("GUARDRAILS_TIMEOUT_SECONDS") into a string, attempt float() inside a try/except (or a small helper like parse_timeout_env/get_timeout_seconds) and on failure fall back to the default 60 (and optionally log a warning) so import won't crash; assign the resulting numeric value to TIMEOUT_SECONDS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/evaluation/multi_validator_whatsapp/run.py`:
- Around line 28-32: The current "ban_list" template contains a hardcoded
"banned_words": ["sonography"] which restricts use; change run.py to accept a
banned-words list from the CLI (or env var) and build the template dynamically
instead of embedding the literal array. Specifically, add a CLI flag (e.g.,
--banned-words) or read an env var, parse it into a list, then replace the
static "banned_words" entry in the "ban_list" template with that parsed list
before the validator is used; update any code that references the "ban_list"
template to consume the newly built template.
- Around line 58-61: The current code in run.py that extracts safe_text
(safe_text = body.get("data", {}).get("safe_text") and returns "" when missing)
conflates missing/validation-failed responses with legitimately empty-string
outputs; change the return to a clear sentinel instead of "" — e.g., return None
(preferred) or the string "VALIDATION_FAILED" from the call_guardrails flow
where safe_text is absent, and update any CSV/export logic that writes these
results to map that sentinel to a distinct marker (e.g., "VALIDATION_FAILED") so
missing/validation failures are distinguishable from empty valid outputs; ensure
references to safe_text and call_guardrails are updated accordingly.
- Around line 86-90: In main(), instead of raising ValueError when invalid
validator names are found (the block that checks selected_validators or
unknown), call parser.error(...) with the same message so argparse prints a
usage and exits with code 2; use the existing parser object and include the
supported values (f"{', '.join(VALIDATOR_TEMPLATES.keys())}") in the error text
so the behavior for selected_validators/unknown mirrors argparse's idiomatic
validation.
- Around line 43-44: The payload in run.py currently hardcodes "organization_id"
and "project_id" to 1; update main() to accept CLI args (e.g., --organization-id
and --project-id) with env-var fallbacks similar to API_URL and TIMEOUT_SECONDS,
parse them to integers, pass them into call_guardrails, and replace the literal
values in the payload construction (the dictionary with keys "organization_id"
and "project_id") so calls target the correct tenant instead of always tenant 1.
---
Nitpick comments:
In `@backend/app/api/routes/guardrails.py`:
- Around line 48-53: The call to _resolve_ban_list_banned_words(payload,
session) can raise exceptions from ban_list_crud.get which escape the current
try/except in _validate_with_guard; wrap the resolution call in the same error
handling as _validate_with_guard (or move the call inside _validate_with_guard's
try block) so DB errors are caught and converted to the same structured
APIResponse/error handling path. Specifically, ensure exceptions from
ban_list_crud.get (raised via _resolve_ban_list_banned_words) are caught and
handled consistently with _validate_with_guard rather than propagating to
FastAPI (reference symbols: _resolve_ban_list_banned_words,
_validate_with_guard, ban_list_crud.get, payload.validators,
BanListSafetyValidatorConfig).
In `@backend/app/evaluation/multi_validator_whatsapp/run.py`:
- Line 16: The module-level float conversion of TIMEOUT_SECONDS from
GUARDRAILS_TIMEOUT_SECONDS can raise ValueError at import time; change it to a
safe parse: read os.getenv("GUARDRAILS_TIMEOUT_SECONDS") into a string, attempt
float() inside a try/except (or a small helper like
parse_timeout_env/get_timeout_seconds) and on failure fall back to the default
60 (and optionally log a warning) so import won't crash; assign the resulting
numeric value to TIMEOUT_SECONDS.
| API_URL = os.getenv("GUARDRAILS_API_URL", "http://localhost:8001/api/v1/guardrails/") | ||
| TIMEOUT_SECONDS = float(os.getenv("GUARDRAILS_TIMEOUT_SECONDS", "60")) | ||
|
|
||
| VALIDATOR_TEMPLATES = { |
There was a problem hiding this comment.
making the script like this is not exactly scalable as not only are there going to be more validators in the future, we will be adding support to create custom validator also, so what would be better would be that you add support to read a json that will have configurations of all these validators that you want to run together. you can make one json file, add instuctions to add config there and run that file
| DATASET_PATH = BASE_DIR / "datasets" / "multi_validator_whatsapp_dataset.csv" | ||
| OUT_PATH = BASE_DIR / "outputs" / "multi_validator_whatsapp" / "predictions.csv" | ||
|
|
||
| API_URL = os.getenv("GUARDRAILS_API_URL", "http://localhost:8001/api/v1/guardrails/") |
There was a problem hiding this comment.
this should not be hardcoded
|
|
||
| payload = { | ||
| "request_id": str(uuid4()), | ||
| "organization_id": 1, |
There was a problem hiding this comment.
these values should not be hardcoded either
2 participants
Summary
Target issue is #67.
Explain the motivation for making this change. What existing problem does the pull request solve?
python3 app/evaluation/multi_validator_whatsapp/run.py --validators_payload uli_slur_match,ban_list,pii_remover --auth_token <auth-token>Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
Bug Fixes
New Features