refactor(library): support canonical conversation history in topic safety - #2237
refactor(library): support canonical conversation history in topic safety#2237Pouyanpi wants to merge 5 commits into
Conversation
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
|
|
||
| if cache: | ||
| cache_key = create_normalized_cache_key(messages) | ||
| cache_key = create_normalized_cache_key(prompt_messages) |
There was a problem hiding this comment.
Exclude metadata from cache identity
Canonical messages now pass volatile fields such as provider request IDs into create_normalized_cache_key, even though those fields are removed before model inference. Model-equivalent conversations therefore receive different cache keys, causing unnecessary topic-safety model calls and reducing cache effectiveness.
Knowledge Base Used: Library Rails
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/library/topic_safety/actions.py
Line: 127
Comment:
**Exclude metadata from cache identity**
Canonical messages now pass volatile fields such as provider request IDs into `create_normalized_cache_key`, even though those fields are removed before model inference. Model-equivalent conversations therefore receive different cache keys, causing unnecessary topic-safety model calls and reducing cache effectiveness.
**Knowledge Base Used:** [Library Rails](https://app.greptile.com/nvidia-public-github/-/custom-context/knowledge-base/nvidia-nemo/guardrails/-/docs/library-rails.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Greptile SummaryThe PR teaches the topic-safety action to consume canonical conversation histories while preserving event-based compatibility.
|
| Filename | Overview |
|---|---|
| nemoguardrails/library/topic_safety/actions.py | Adds canonical-message handling and preserves the legacy event fallback, but volatile provider metadata now participates in cache identity. |
| nemoguardrails/library/self_check/input_check/actions.py | Adds explanatory comments distinguishing legacy event injection from manifest-bound variants without changing behavior. |
| nemoguardrails/library/self_check/output_check/actions.py | Adds explanatory comments distinguishing legacy event injection from manifest-bound variants without changing behavior. |
| tests/test_topic_safety_actions.py | Adds focused coverage for canonical metadata, event-derived history, absent history, and empty canonical history. |
| tests/test_topic_safety_railoutcome_flow.py | Adds integration coverage proving multi-turn topic-safety prompt ordering without duplication. |
| tests/test_multiple_self_check_rails.py | Extends task-resolution coverage to prove explicit variants override both legacy context and event-derived variants. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Topic-safety action] --> B{Canonical messages supplied?}
B -->|Yes| C[Copy canonical conversation]
B -->|No| D[Convert legacy Colang events]
D --> E[Append current user input]
C --> F[Prepend system prompt]
E --> F
F --> G[Topic-safety LLM call]
G --> H[Allow or block RailOutcome]
Prompt To Fix All With AI
### Issue 1
nemoguardrails/library/topic_safety/actions.py:127
**Exclude metadata from cache identity**
Canonical messages now pass volatile fields such as provider request IDs into `create_normalized_cache_key`, even though those fields are removed before model inference. Model-equivalent conversations therefore receive different cache keys, causing unnecessary topic-safety model calls and reducing cache effectiveness.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "docs(library): explain topic safety even..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoguardrails/library/self_check/input_check/actions.py`:
- Around line 46-48: Remove the added inline comment blocks from both self-check
actions: nemoguardrails/library/self_check/input_check/actions.py lines 46-48
and nemoguardrails/library/self_check/output_check/actions.py lines 46-48. No
code behavior changes are required.
In `@tests/test_topic_safety_railoutcome_flow.py`:
- Around line 75-93: Update
test_multiturn_history_reaches_topic_safety_once_and_in_order to assert
topic_model.generate_async was awaited exactly once before accessing await_args.
Keep the existing prompt ordering assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1d9b25a5-71c4-453f-b30f-43c4f0769aaa
📒 Files selected for processing (6)
nemoguardrails/library/self_check/input_check/actions.pynemoguardrails/library/self_check/output_check/actions.pynemoguardrails/library/topic_safety/actions.pytests/test_multiple_self_check_rails.pytests/test_topic_safety_actions.pytests/test_topic_safety_railoutcome_flow.py
| # LLMRails injects Colang events only to recover variants for legacy 1.0 | ||
| # flows, including parallel rails. Manifests bind `variant` instead so | ||
| # engine-neutral callers do not provide or synthesize events. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unrequested inline comments in both self-check actions.
The repository guideline prohibits adding comments in Python files unless explicitly requested.
nemoguardrails/library/self_check/input_check/actions.py#L46-L48: remove the added comment block or confirm explicit maintainer approval.nemoguardrails/library/self_check/output_check/actions.py#L46-L48: remove the added comment block or confirm explicit maintainer approval.
📍 Affects 2 files
nemoguardrails/library/self_check/input_check/actions.py#L46-L48(this comment)nemoguardrails/library/self_check/output_check/actions.py#L46-L48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemoguardrails/library/self_check/input_check/actions.py` around lines 46 -
48, Remove the added inline comment blocks from both self-check actions:
nemoguardrails/library/self_check/input_check/actions.py lines 46-48 and
nemoguardrails/library/self_check/output_check/actions.py lines 46-48. No code
behavior changes are required.
Source: Coding guidelines
| def test_multiturn_history_reaches_topic_safety_once_and_in_order(): | ||
| chat = _chat_with_verdict("on-topic") | ||
| topic_model = chat.app.runtime.registered_action_params["llms"]["topic_control"] | ||
| topic_model.generate_async = AsyncMock(wraps=topic_model.generate_async) | ||
|
|
||
| chat.app.generate( | ||
| messages=[ | ||
| {"role": "user", "content": "earlier question"}, | ||
| {"role": "assistant", "content": "earlier answer"}, | ||
| {"role": "user", "content": "current question"}, | ||
| ] | ||
| ) | ||
|
|
||
| prompt = topic_model.generate_async.await_args.args[0] | ||
| assert [(message.role, message.content) for message in prompt[1:]] == [ | ||
| ("user", "earlier question"), | ||
| ("assistant", "earlier answer"), | ||
| ("user", "current question"), | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
uv run --locked python - <<'PY'
from unittest.mock import AsyncMock
assert hasattr(AsyncMock(), "assert_awaited_once")
PYRepository: NVIDIA-NeMo/Guardrails
Length of output: 200
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from unittest.mock import AsyncMock
print("assert_awaited_once", hasattr(AsyncMock(), "assert_awaited_once"))
PY
printf '\n--- file outline ---\n'
ast-grep outline tests/test_topic_safety_railoutcome_flow.py --view expanded || true
printf '\n--- relevant lines ---\n'
cat -n tests/test_topic_safety_railoutcome_flow.py | sed -n '1,180p'
PYRepository: NVIDIA-NeMo/Guardrails
Length of output: 4622
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from unittest.mock import AsyncMock
print("assert_awaited_once", hasattr(AsyncMock(), "assert_awaited_once"))
PY
printf '\n--- file outline ---\n'
ast-grep outline tests/test_topic_safety_railoutcome_flow.py --view expanded || true
printf '\n--- relevant lines ---\n'
cat -n tests/test_topic_safety_railoutcome_flow.py | sed -n '1,180p'Repository: NVIDIA-NeMo/Guardrails
Length of output: 4581
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from unittest.mock import AsyncMock
print("assert_awaited_once", hasattr(AsyncMock(), "assert_awaited_once"))
PY
printf '\n--- file outline ---\n'
ast-grep outline tests/test_topic_safety_railoutcome_flow.py --view expanded || true
printf '\n--- relevant lines ---\n'
sed -n '1,180p' tests/test_topic_safety_railoutcome_flow.py | cat -nRepository: NVIDIA-NeMo/Guardrails
Length of output: 4581
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from unittest.mock import AsyncMock
print("assert_awaited_once", hasattr(AsyncMock(), "assert_awaited_once"))
PY
echo '--- relevant lines ---'
nl -ba tests/test_topic_safety_railoutcome_flow.py | sed -n '70,100p'Repository: NVIDIA-NeMo/Guardrails
Length of output: 249
Assert the topic model is awaited exactly once.
await_args only reflects the last await, so this test can still pass if generate_async runs multiple times. Add assert_awaited_once() before reading await_args.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_topic_safety_railoutcome_flow.py` around lines 75 - 93, Update
test_multiturn_history_reaches_topic_safety_once_and_in_order to assert
topic_model.generate_async was awaited exactly once before accessing await_args.
Keep the existing prompt ordering assertions unchanged.
Description
Related Issue(s)
Verification
AI Assistance
Checklist
Summary by CodeRabbit
Improvements
Documentation
Tests