Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions garak/generators/guardrails.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,22 @@ class NeMoGuardrailsServer(OpenAICompatible):
def __init__(self, name="", config_root=_config):
self.api_key = "not-used" # suppress any api_key from being sent as the server does not utilize one
super().__init__(name, config_root)
if self.extra_params and not self.extra_params.get("extra_body"):
self.extra_params.append("extra_body")
Comment on lines -75 to -76

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, there is definitely a logic bug here. The approach taken though does not look to align with the intent as the original guard logic was flawed and does not have a clear comment on what it was meant to change after the call to super.__init__() completed.

The fix to this should not be to remove support for an existing extra_body found in the extra_params dict, a fix should properly handle when one does exist. The changes here actually would result in the user provided extra_body overriding what this generator is attempting to support as self.extra_body would shadowed by self.extra_params["extra_body"] in the current OpenAICompatible._call_model().

This suggests the the guard clause is checking the wrong condition and should be improved so the action taken will preform the correct addition. This guard exists augment the existing expectations for extra_params in OpenAICompatible to pass thru any keys in the extra_params provided via config as parameters to the OpenAI client method used for the inference request by merging the guardrails specific extra_body values.

        if self.extra_params and "extra_body" in self.extra_params:
            user_extra_body = self.extra_params.pop("extra_body", None)
            if user_extra_body:
                self.extra_body = user_extra_body

This would align with the rest of the initialization here by setting self.extra_body to the configured value and allowing the guardrails specific injections to be added to a configuration provided extra_body.

Note the suggestion above is still somewhat naive and should probably be extended with type checking to ensure self.extra_params["extra_body"] is a dict to avoid a type mis-match and raise an early exception about invalid configuration values.

Consider exploring adding more unit tests that evaluate how self.extra_body and self.extra_params will behave in the various combinations. Also consider looking at what happens if you were to mock the call to the underlying OpenAI client object in _call_model to see how these values end up mapping into that methods arguments.


# A user-provided `extra_body` may arrive nested inside `extra_params` (the generic
# passthrough dict for OpenAICompatible). Left there, it would independently overwrite
# `self.extra_body` in `_call_model()` (extra_params is applied after the instance's own
# attributes), silently dropping the `guardrails` config merged in below. Pop it out and
# fold it into `self.extra_body` instead so the two sources combine rather than one
# shadowing the other.
if self.extra_params and "extra_body" in self.extra_params:
user_extra_body = self.extra_params.pop("extra_body", None)
if user_extra_body:
if not isinstance(user_extra_body, dict):
raise ValueError(
"NeMoGuardrailsServer: `extra_params['extra_body']` must be a dict, got "
f"{type(user_extra_body).__name__}"
)
self.extra_body = user_extra_body

guardrails = None
if self.config_ids:
Expand Down
86 changes: 86 additions & 0 deletions tests/generators/test_guardrails.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0

import json

import httpx
import pytest

Expand Down Expand Up @@ -51,3 +53,87 @@ def test_guardrail_selection(selected_rails, respx_mock, openai_compat_mocks):
assert "guardrails" in content
assert "config_ids" in content
assert rail in content


def test_nonempty_extra_params_does_not_crash_init():
"""Construction with a non-empty extra_params lacking 'extra_body' must not crash.

The removed block ran ``self.extra_params.append("extra_body")`` — .append
on a dict — which raised AttributeError during __init__ for any user who
set extra_params without an 'extra_body' key.
"""
config_root = {
"generators": {
"guardrails": {
"NeMoGuardrailsServer": {
"name": "UnknownModel",
"extra_params": {"foo": 1},
}
}
}
}
g = NeMoGuardrailsServer(config_root=config_root)
assert g.extra_params == {"foo": 1}


@pytest.mark.respx(base_url=NeMoGuardrailsServer.DEFAULT_PARAMS["uri"])
def test_user_extra_body_is_merged_not_shadowed_by_guardrails_config(
respx_mock, openai_compat_mocks
):
"""A user-provided extra_body (via extra_params) must combine with, not be
overwritten by, the guardrails config injected into self.extra_body — and
must not remain in extra_params where it would independently overwrite
self.extra_body again in _call_model().
"""
mock_response = openai_compat_mocks["chat"]
mock_request = respx_mock.post("chat/completions")
mock_request.mock(
return_value=httpx.Response(
mock_response["code"],
json=mock_response["json"],
)
)
config_root = {
"generators": {
"guardrails": {
"NeMoGuardrailsServer": {
"name": "UnknownModel",
"config_ids": ["rail1"],
"extra_params": {"extra_body": {"custom_key": "custom_value"}},
}
}
}
}
g = NeMoGuardrailsServer(config_root=config_root)

assert "extra_body" not in g.extra_params
assert g.extra_body == {
"custom_key": "custom_value",
"guardrails": {"config_ids": ["rail1"]},
}

conv = Conversation(turns=[Turn(role="user", content=Message("Testing text"))])
g.generate(conv)
assert mock_request.called
sent_body = json.loads(mock_request.calls.last.request.content)
assert sent_body["custom_key"] == "custom_value"
assert sent_body["guardrails"] == {"config_ids": ["rail1"]}


def test_non_dict_user_extra_body_raises_value_error():
"""A non-dict extra_params['extra_body'] must raise a clear ValueError at
construction time rather than silently corrupting self.extra_body or
failing later with an unrelated error.
"""
config_root = {
"generators": {
"guardrails": {
"NeMoGuardrailsServer": {
"name": "UnknownModel",
"extra_params": {"extra_body": "not-a-dict"},
}
}
}
}
with pytest.raises(ValueError, match="must be a dict"):
NeMoGuardrailsServer(config_root=config_root)
Loading