-
Notifications
You must be signed in to change notification settings - Fork 798
refactor(library): support canonical conversation history in topic safety #2237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
1e4e141
d74395e
69ef27f
d1448af
3a8faa1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,8 @@ | |
| # limitations under the License. | ||
|
|
||
| import logging | ||
| from typing import Dict, List, Optional | ||
| from collections.abc import Mapping, Sequence | ||
| from typing import Any, Dict, List, Optional | ||
|
|
||
| from nemoguardrails.actions.actions import action | ||
| from nemoguardrails.actions.llm.utils import llm_call | ||
|
|
@@ -50,8 +51,12 @@ async def topic_safety_check_input( | |
| llm_task_manager: LLMTaskManager, | ||
| model_name: Optional[str] = None, | ||
| context: Optional[dict] = None, | ||
| # LLMRails injects Colang events only as a compatibility source of | ||
| # conversation history. Manifests omit events because engine-neutral | ||
| # executors provide canonical `messages` and must not synthesize events. | ||
| events: Optional[List[dict]] = None, | ||
| model_caches: Optional[Dict[str, CacheInterface]] = None, | ||
| messages: Optional[Sequence[Mapping[str, Any]]] = None, | ||
| **kwargs, | ||
| ) -> RailOutcome: | ||
| _MAX_TOKENS = TOPIC_SAFETY_MAX_TOKENS | ||
|
|
@@ -61,10 +66,12 @@ async def topic_safety_check_input( | |
| user_input = context.get("user_message", "") | ||
| model_name = model_name or context.get("model", None) | ||
|
|
||
| if events is not None: | ||
| if messages is not None: | ||
| conversation_history = [dict(message) for message in messages] | ||
| else: | ||
| # convert InternalEvent objects to dictionary format for compatibility with to_chat_messages | ||
| dict_events = [] | ||
| for event in events: | ||
| for event in events or []: | ||
| if hasattr(event, "name") and hasattr(event, "arguments"): | ||
| dict_event = {"type": event.name} | ||
| dict_event.update(event.arguments) | ||
|
|
@@ -108,21 +115,27 @@ async def topic_safety_check_input( | |
|
|
||
| max_tokens = max_tokens or _MAX_TOKENS | ||
|
|
||
| messages = [] | ||
| messages.append({"type": "system", "content": system_prompt}) | ||
| messages.extend(conversation_history) | ||
| messages.append({"type": "user", "content": user_input}) | ||
| prompt_messages = [] | ||
| prompt_messages.append({"type": "system", "content": system_prompt}) | ||
| prompt_messages.extend(conversation_history) | ||
| if messages is None: | ||
| prompt_messages.append({"type": "user", "content": user_input}) | ||
|
|
||
| cache = model_caches.get(model_name) if model_caches else None | ||
|
|
||
| if cache: | ||
| cache_key = create_normalized_cache_key(messages) | ||
| cache_key = create_normalized_cache_key(prompt_messages) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Canonical messages now pass volatile fields such as provider request IDs into Knowledge Base Used: Library Rails Prompt To Fix With AIThis 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. |
||
| cached_result = get_from_cache_and_restore_stats(cache, cache_key) | ||
| if cached_result is not None: | ||
| log.debug(f"Topic safety cache hit for model '{model_name}'") | ||
| return cached_result | ||
|
|
||
| response = await llm_call(llm, messages, stop=stop, llm_params={"temperature": TOPIC_SAFETY_TEMPERATURE}) | ||
| response = await llm_call( | ||
| llm, | ||
| prompt_messages, | ||
| stop=stop, | ||
| llm_params={"temperature": TOPIC_SAFETY_TEMPERATURE}, | ||
| ) | ||
| result = response.content | ||
|
|
||
| if result.lower().strip() == "off-topic": | ||
|
|
@@ -133,7 +146,7 @@ async def topic_safety_check_input( | |
| final_result = RailOutcome.allow() if on_topic else RailOutcome.block() | ||
|
|
||
| if cache: | ||
| cache_key = create_normalized_cache_key(messages) | ||
| cache_key = create_normalized_cache_key(prompt_messages) | ||
| cache_entry: CacheEntry = { | ||
| "result": final_result, | ||
| "llm_stats": extract_llm_stats_for_cache(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from unittest.mock import AsyncMock, MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
||
| from nemoguardrails.library.topic_safety.actions import ( | ||
| TOPIC_SAFETY_OUTPUT_RESTRICTION, | ||
| topic_safety_check_input, | ||
| ) | ||
| from nemoguardrails.types import LLMResponse | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def task_manager(): | ||
| manager = MagicMock() | ||
| manager.render_task_prompt.return_value = "Stay on topic." | ||
| manager.get_stop_tokens.return_value = [] | ||
| manager.get_max_tokens.return_value = 10 | ||
| return manager | ||
|
|
||
|
|
||
| async def _run_topic_safety(task_manager, **kwargs): | ||
| with patch( | ||
| "nemoguardrails.library.topic_safety.actions.llm_call", | ||
| new_callable=AsyncMock, | ||
| ) as mock_llm_call: | ||
| mock_llm_call.return_value = LLMResponse(content="on-topic") | ||
| result = await topic_safety_check_input( | ||
| llms={"topic_control": "topic model"}, | ||
| llm_task_manager=task_manager, | ||
| model_name="topic_control", | ||
| context={"user_message": "current question"}, | ||
| **kwargs, | ||
| ) | ||
|
|
||
| return result, mock_llm_call | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_canonical_messages_are_authoritative_and_preserve_metadata(task_manager): | ||
| canonical_messages = [ | ||
| {"role": "user", "content": "earlier question"}, | ||
| { | ||
| "role": "assistant", | ||
| "content": "earlier answer", | ||
| "provider_metadata": {"request_id": "response-1"}, | ||
| }, | ||
| { | ||
| "role": "user", | ||
| "content": [{"type": "text", "text": "current question"}], | ||
| "name": "customer", | ||
| }, | ||
| ] | ||
|
|
||
| result, mock_llm_call = await _run_topic_safety( | ||
| task_manager, | ||
| messages=canonical_messages, | ||
| events=[{"type": "UserMessage", "text": "ignored event"}], | ||
| ) | ||
|
|
||
| assert result.is_blocked is False | ||
| assert mock_llm_call.await_args.args[1] == [ | ||
| { | ||
| "type": "system", | ||
| "content": f"Stay on topic.\n\n{TOPIC_SAFETY_OUTPUT_RESTRICTION}", | ||
| }, | ||
| *canonical_messages, | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_legacy_events_preserve_conversation_order_and_append_current_input(task_manager): | ||
| result, mock_llm_call = await _run_topic_safety( | ||
| task_manager, | ||
| events=[ | ||
| {"type": "UserMessage", "text": "earlier question"}, | ||
| {"type": "StartUtteranceBotAction", "script": "earlier answer"}, | ||
| ], | ||
| ) | ||
|
|
||
| assert result.is_blocked is False | ||
| assert mock_llm_call.await_args.args[1] == [ | ||
| { | ||
| "type": "system", | ||
| "content": f"Stay on topic.\n\n{TOPIC_SAFETY_OUTPUT_RESTRICTION}", | ||
| }, | ||
| {"role": "user", "content": "earlier question"}, | ||
| {"role": "assistant", "content": "earlier answer"}, | ||
| {"type": "user", "content": "current question"}, | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_absent_history_still_checks_current_input(task_manager): | ||
| result, mock_llm_call = await _run_topic_safety(task_manager) | ||
|
|
||
| assert result.is_blocked is False | ||
| assert mock_llm_call.await_args.args[1] == [ | ||
| { | ||
| "type": "system", | ||
| "content": f"Stay on topic.\n\n{TOPIC_SAFETY_OUTPUT_RESTRICTION}", | ||
| }, | ||
| {"type": "user", "content": "current question"}, | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_empty_canonical_messages_take_precedence_over_legacy_history(task_manager): | ||
| result, mock_llm_call = await _run_topic_safety( | ||
| task_manager, | ||
| messages=[], | ||
| events=[{"type": "UserMessage", "text": "ignored event"}], | ||
| ) | ||
|
|
||
| assert result.is_blocked is False | ||
| assert mock_llm_call.await_args.args[1] == [ | ||
| { | ||
| "type": "system", | ||
| "content": f"Stay on topic.\n\n{TOPIC_SAFETY_OUTPUT_RESTRICTION}", | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| """ | ||
|
|
||
| import textwrap | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| from nemoguardrails import RailsConfig | ||
| from tests.utils import FakeLLMModel, TestChat | ||
|
|
@@ -69,3 +70,24 @@ def test_on_topic_input_passes_through_railoutcome(): | |
| chat = _chat_with_verdict("on-topic") | ||
| response = chat.app.generate(messages=[{"role": "user", "content": "a relevant question"}]) | ||
| assert response["content"] == "Hello! How can I help you?" | ||
|
|
||
|
|
||
| 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"), | ||
| ] | ||
|
Comment on lines
+75
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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.
🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 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
Source: Coding guidelines