Skip to content

Commit

Permalink
Python: Introduce agent name field regex (#9887)
Browse files Browse the repository at this point in the history
### Motivation and Context

The agent name Pydantic field currently does not have any pattern regex
config. If a user creates an agent with an incorrect name (something
other than `0-9A-Za-z_-`) they will get a 400. To validate and error out
earlier, we're introducing the regex check when the field is validated.

Note, this is a non-breaking change because an invalid name errors out
further down in the processing -- we're adding this regex check so that
it can help a user/dev understand earlier in the processing that they
need to update their agent name.

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

### Description

Add the regex field validation for the agent name as `AGENT_NAME_REGEX =
r"^[0-9A-Za-z_-]+$"`. Unlike the SK plugin/function names, this name can
contain a `-`. Updates unit tests.
- Closes #9340 

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [X] The code builds clean without any errors or warnings
- [X] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [X] All unit tests pass, and I have added new tests where possible
- [X] I didn't break anyone 😄
  • Loading branch information
moonbox3 authored Dec 6, 2024
1 parent 6dc7559 commit 7479657
Show file tree
Hide file tree
Showing 9 changed files with 50 additions and 35 deletions.
3 changes: 2 additions & 1 deletion python/semantic_kernel/agents/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from semantic_kernel.kernel_pydantic import KernelBaseModel
from semantic_kernel.utils.experimental_decorator import experimental_class
from semantic_kernel.utils.naming import generate_random_ascii_name
from semantic_kernel.utils.validation import AGENT_NAME_REGEX


@experimental_class
Expand All @@ -32,7 +33,7 @@ class Agent(KernelBaseModel):

id: str = Field(default_factory=lambda: str(uuid.uuid4()))
description: str | None = None
name: str = Field(default_factory=lambda: f"agent_{generate_random_ascii_name()}")
name: str = Field(default_factory=lambda: f"agent_{generate_random_ascii_name()}", pattern=AGENT_NAME_REGEX)
instructions: str | None = None
kernel: Kernel = Field(default_factory=Kernel)
channel_type: ClassVar[type[AgentChannel] | None] = None
Expand Down
1 change: 1 addition & 0 deletions python/semantic_kernel/utils/validation.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Copyright (c) Microsoft. All rights reserved.

AGENT_NAME_REGEX = r"^[0-9A-Za-z_-]+$"
PLUGIN_NAME_REGEX = r"^[0-9A-Za-z_]+$"
FUNCTION_NAME_REGEX = r"^[0-9A-Za-z_]+$"
FULLY_QUALIFIED_FUNCTION_NAME = r"^(?P<plugin>[0-9A-Za-z_]+)[.](?P<function>[0-9A-Za-z_]+)$"
Expand Down
20 changes: 10 additions & 10 deletions python/tests/unit/agents/test_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
class MockAgent(Agent):
"""A mock agent for testing purposes."""

def __init__(self, name: str = "Test Agent", description: str = "A test agent", id: str = None):
def __init__(self, name: str = "Test-Agent", description: str = "A test agent", id: str = None):
args = {
"name": name,
"description": description,
Expand All @@ -30,7 +30,7 @@ async def create_channel(self) -> AgentChannel:

@pytest.mark.asyncio
async def test_agent_initialization():
name = "Test Agent"
name = "TestAgent"
description = "A test agent"
id_value = str(uuid.uuid4())

Expand Down Expand Up @@ -68,21 +68,21 @@ async def test_create_channel():
async def test_agent_equality():
id_value = str(uuid.uuid4())

agent1 = MockAgent(name="Test Agent", description="A test agent", id=id_value)
agent2 = MockAgent(name="Test Agent", description="A test agent", id=id_value)
agent1 = MockAgent(name="TestAgent", description="A test agent", id=id_value)
agent2 = MockAgent(name="TestAgent", description="A test agent", id=id_value)

assert agent1 == agent2

agent3 = MockAgent(name="Test Agent", description="A different description", id=id_value)
agent3 = MockAgent(name="TestAgent", description="A different description", id=id_value)
assert agent1 != agent3

agent4 = MockAgent(name="Another Agent", description="A test agent", id=id_value)
agent4 = MockAgent(name="AnotherAgent", description="A test agent", id=id_value)
assert agent1 != agent4


@pytest.mark.asyncio
async def test_agent_equality_different_type():
agent = MockAgent(name="Test Agent", description="A test agent", id=str(uuid.uuid4()))
agent = MockAgent(name="TestAgent", description="A test agent", id=str(uuid.uuid4()))
non_agent = "Not an agent"

assert agent != non_agent
Expand All @@ -92,10 +92,10 @@ async def test_agent_equality_different_type():
async def test_agent_hash():
id_value = str(uuid.uuid4())

agent1 = MockAgent(name="Test Agent", description="A test agent", id=id_value)
agent2 = MockAgent(name="Test Agent", description="A test agent", id=id_value)
agent1 = MockAgent(name="TestAgent", description="A test agent", id=id_value)
agent2 = MockAgent(name="TestAgent", description="A test agent", id=id_value)

assert hash(agent1) == hash(agent2)

agent3 = MockAgent(name="Test Agent", description="A different description", id=id_value)
agent3 = MockAgent(name="TestAgent", description="A different description", id=id_value)
assert hash(agent1) != hash(agent3)
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
class MockAgent(Agent):
"""A mock agent for testing purposes."""

def __init__(self, id: str = None, name: str = "Test Agent", description: str = "A test agent"):
def __init__(self, id: str = None, name: str = "TestAgent", description: str = "A test agent"):
args = {
"name": name,
"description": description,
Expand Down
41 changes: 27 additions & 14 deletions python/tests/unit/agents/test_chat_completion_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from unittest.mock import AsyncMock, create_autospec, patch

import pytest
from pydantic import ValidationError

from semantic_kernel.agents import ChatCompletionAgent
from semantic_kernel.agents.channels.chat_history_channel import ChatHistoryChannel
Expand Down Expand Up @@ -33,31 +34,43 @@ async def mock_response(chat_history, settings, kernel):
async def test_initialization():
agent = ChatCompletionAgent(
service_id="test_service",
name="Test Agent",
name="TestAgent",
id="test_id",
description="Test Description",
instructions="Test Instructions",
)

assert agent.service_id == "test_service"
assert agent.name == "Test Agent"
assert agent.name == "TestAgent"
assert agent.id == "test_id"
assert agent.description == "Test Description"
assert agent.instructions == "Test Instructions"


@pytest.mark.asyncio
async def test_initialization_invalid_name_throws():
with pytest.raises(ValidationError):
_ = ChatCompletionAgent(
service_id="test_service",
name="Test Agent",
id="test_id",
description="Test Description",
instructions="Test Instructions",
)


@pytest.mark.asyncio
async def test_initialization_no_service_id():
agent = ChatCompletionAgent(
name="Test Agent",
name="TestAgent",
id="test_id",
description="Test Description",
instructions="Test Instructions",
)

assert agent.service_id == "default"
assert agent.kernel is not None
assert agent.name == "Test Agent"
assert agent.name == "TestAgent"
assert agent.id == "test_id"
assert agent.description == "Test Description"
assert agent.instructions == "Test Instructions"
Expand All @@ -67,15 +80,15 @@ async def test_initialization_no_service_id():
async def test_initialization_with_kernel(kernel: Kernel):
agent = ChatCompletionAgent(
kernel=kernel,
name="Test Agent",
name="TestAgent",
id="test_id",
description="Test Description",
instructions="Test Instructions",
)

assert agent.service_id == "default"
assert kernel == agent.kernel
assert agent.name == "Test Agent"
assert agent.name == "TestAgent"
assert agent.id == "test_id"
assert agent.description == "Test Description"
assert agent.instructions == "Test Instructions"
Expand All @@ -89,7 +102,7 @@ async def test_invoke():
return_value=[ChatMessageContent(role=AuthorRole.SYSTEM, content="Processed Message")]
)
agent = ChatCompletionAgent(
kernel=kernel, service_id="test_service", name="Test Agent", instructions="Test Instructions"
kernel=kernel, service_id="test_service", name="TestAgent", instructions="Test Instructions"
)

history = ChatHistory(messages=[ChatMessageContent(role=AuthorRole.USER, content="Initial Message")])
Expand All @@ -105,7 +118,7 @@ async def test_invoke_tool_call_added():
kernel = create_autospec(Kernel)
chat_completion_service = create_autospec(ChatCompletionClientBase)
kernel.get_service.return_value = chat_completion_service
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="Test Agent")
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="TestAgent")

history = ChatHistory(messages=[ChatMessageContent(role=AuthorRole.USER, content="Initial Message")])

Expand All @@ -128,15 +141,15 @@ async def mock_get_chat_message_contents(chat_history, settings, kernel):
assert len(history.messages) == 3
assert history.messages[1].content == "Processed Message 1"
assert history.messages[2].content == "Processed Message 2"
assert history.messages[1].name == "Test Agent"
assert history.messages[2].name == "Test Agent"
assert history.messages[1].name == "TestAgent"
assert history.messages[2].name == "TestAgent"


@pytest.mark.asyncio
async def test_invoke_no_service_throws():
kernel = create_autospec(Kernel)
kernel.get_service.return_value = None
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="Test Agent")
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="TestAgent")

history = ChatHistory(messages=[ChatMessageContent(role=AuthorRole.USER, content="Initial Message")])

Expand All @@ -150,7 +163,7 @@ async def test_invoke_stream():
kernel = create_autospec(Kernel)
kernel.get_service.return_value = create_autospec(ChatCompletionClientBase)

agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="Test Agent")
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="TestAgent")

history = ChatHistory(messages=[ChatMessageContent(role=AuthorRole.USER, content="Initial Message")])

Expand All @@ -172,7 +185,7 @@ async def test_invoke_stream_tool_call_added(mock_streaming_chat_completion_resp
kernel = create_autospec(Kernel)
chat_completion_service = create_autospec(ChatCompletionClientBase)
kernel.get_service.return_value = chat_completion_service
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="Test Agent")
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="TestAgent")

history = ChatHistory(messages=[ChatMessageContent(role=AuthorRole.USER, content="Initial Message")])

Expand All @@ -190,7 +203,7 @@ async def test_invoke_stream_tool_call_added(mock_streaming_chat_completion_resp
async def test_invoke_stream_no_service_throws():
kernel = create_autospec(Kernel)
kernel.get_service.return_value = None
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="Test Agent")
agent = ChatCompletionAgent(kernel=kernel, service_id="test_service", name="TestAgent")

history = ChatHistory(messages=[ChatMessageContent(role=AuthorRole.USER, content="Initial Message")])

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
class MockAgent(Agent):
"""A mock agent for testing purposes."""

def __init__(self, id: str = None, name: str = "Test Agent", description: str = "A test agent"):
def __init__(self, id: str = None, name: str = "TestAgent", description: str = "A test agent"):
args = {
"name": name,
"description": description,
Expand All @@ -37,14 +37,14 @@ async def create_channel(self) -> AgentChannel:
@pytest.fixture
def agents():
"""Fixture that provides a list of mock agents."""
return [MockAgent(id=f"agent-{i}", name=f"Agent-{i}") for i in range(3)]
return [MockAgent(id=f"agent-{i}", name=f"Agent_{i}") for i in range(3)]


@pytest.mark.asyncio
async def test_kernel_function_selection_next_success(agents):
history = [MagicMock(spec=ChatMessageContent)]
mock_function = AsyncMock(spec=KernelFunction)
mock_function.invoke.return_value = MagicMock(value="Agent-1")
mock_function.invoke.return_value = MagicMock(value="Agent_1")
mock_kernel = MagicMock(spec=Kernel)

strategy = KernelFunctionSelectionStrategy(
Expand All @@ -53,7 +53,7 @@ async def test_kernel_function_selection_next_success(agents):

selected_agent = await strategy.next(agents, history)

assert selected_agent.name == "Agent-1"
assert selected_agent.name == "Agent_1"
mock_function.invoke.assert_awaited_once()


Expand Down Expand Up @@ -115,7 +115,7 @@ async def test_kernel_function_selection_next_exception_during_invoke(agents):
async def test_kernel_function_selection_result_parser_is_async(agents):
history = [MagicMock(spec=ChatMessageContent)]
mock_function = AsyncMock(spec=KernelFunction)
mock_function.invoke.return_value = MagicMock(value="Agent-2")
mock_function.invoke.return_value = MagicMock(value="Agent_2")
mock_kernel = MagicMock(spec=Kernel)

async def async_result_parser(result):
Expand All @@ -127,5 +127,5 @@ async def async_result_parser(result):

selected_agent = await strategy.next(agents, history)

assert selected_agent.name == "Agent-2"
assert selected_agent.name == "Agent_2"
mock_function.invoke.assert_awaited_once()
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
class MockAgent(Agent):
"""A mock agent for testing purposes."""

def __init__(self, id: str = None, name: str = "Test Agent", description: str = "A test agent"):
def __init__(self, id: str = None, name: str = "TestAgent", description: str = "A test agent"):
args = {
"name": name,
"description": description,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
class MockAgent(Agent):
"""A mock agent for testing purposes."""

def __init__(self, id: str = None, name: str = "Test Agent", description: str = "A test agent"):
def __init__(self, id: str = None, name: str = "TestAgent", description: str = "A test agent"):
args = {
"name": name,
"description": description,
Expand Down
2 changes: 1 addition & 1 deletion python/tests/unit/agents/test_termination_strategy.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
class MockAgent(Agent):
"""A mock agent for testing purposes."""

def __init__(self, id: str = None, name: str = "Test Agent", description: str = "A test agent"):
def __init__(self, id: str = None, name: str = "TestAgent", description: str = "A test agent"):
args = {
"name": name,
"description": description,
Expand Down

0 comments on commit 7479657

Please sign in to comment.