Python: [WIP] Bedrock agent #10307
Python: [WIP] Bedrock agent #10307
Conversation
TaoChenOSU
added
PR: in progress
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import asyncio | ||
| import uuid | ||
|
|
||
| from samples.concepts.agents.bedrock_agent.setup_utils import use_existing_agent, use_new_agent |
There was a problem hiding this comment.
ideally a simple chat sample shouldn't have to import a bunch of stuff, I think for this only doing create a new agent would be fine
|
|
||
| # If you want to enable streaming, set this to True. | ||
| # In order to perform streaming, you need to have the permission on action: bedrock:InvokeModelWithResponseStream | ||
| STREAMING = False |
There was a problem hiding this comment.
also just choose this one
| # [Note] You may have to request access to the foundation model if you don't have it. | ||
| # [Note] When using function calling, the success rate of function calling may vary | ||
| # depending on the foundation model. Advanced models may have better performance. | ||
| FOUNDATION_MODEL = os.getenv("FOUNDATION_MODEL") |
There was a problem hiding this comment.
can this be moved into a BedrockAgentSettings/BedrockSettings envar?
|
|
||
| return bedrock_agent | ||
|
|
||
| async def create_agent( |
There was a problem hiding this comment.
could we use the same pattern for create as with OpenAIAssistantAgent?
python/semantic_kernel/agents/bedrock/models/bedrock_agent_event_type.py
Outdated
Show resolved
Hide resolved
python/semantic_kernel/agents/bedrock/models/bedrock_agent_event_type.py
Outdated
Show resolved
Hide resolved
python/samples/concepts/agents/bedrock_agent/bedrock_agent_simple_chat.py
Outdated
Show resolved
Hide resolved
python/samples/concepts/agents/bedrock_agent/bedrock_agent_simple_chat_with_code_interpreter.py
Outdated
Show resolved
Hide resolved
python/samples/concepts/agents/bedrock_agent/bedrock_agent_simple_chat_with_code_interpreter.py
Outdated
Show resolved
Hide resolved
| channel_type: ClassVar[type[BedrockAgentChannel]] = BedrockAgentChannel | ||
|
|
||
| @classmethod | ||
| async def create_new_agent( |
There was a problem hiding this comment.
Thoughts on aligning this create_new_agent method to be similar with other agents (like the Assistant Agent) that have a create method?
| ) -> "BedrockAgent": | ||
| """Create a new agent asynchronously.""" | ||
| agent_base_class_kwargs = {} | ||
| if "kernel" in kwargs: |
There was a problem hiding this comment.
Any reason why we can't add an explicit kernel argument to the method? Similarly arguments: KernelArguments | None as well as a prompt_template_config: PromptTemplateConfig | None?
| kwargs.setdefault("streamingConfigurations", {})["streamFinalResponse"] = True | ||
| kwargs.setdefault("sessionState", {}) | ||
|
|
||
| for request_index in range(self.function_choice_behavior.maximum_auto_invoke_attempts): |
There was a problem hiding this comment.
Is the function_choice_behavior that drives the interaction of invoking the agent the only possible way of handling this?
| """List associated agent knowledge bases.""" | ||
| return await self._list_associated_agent_knowledge_bases(**kwargs) | ||
|
|
||
| async def invoke( |
There was a problem hiding this comment.
Our agents should support both kernel arguments and kernel invoke-time overrides. Reference for handling those items: https://github.com/microsoft/semantic-kernel/blob/d0157424b4600a65ad58d699250b80c7162c17e4/python/semantic_kernel/agents/chat_completion/chat_completion_agent.py#L121C9-L127C52
| if not isinstance(agent, BedrockAgent): | ||
| raise AgentChatException(f"Agent is not of the expected type {type(BedrockAgent)}.") | ||
| if not self.messages: | ||
| raise AgentChatException("No chat history available.") |
There was a problem hiding this comment.
In ChatHistory we have: messages: list[ChatMessageContent] = Field(default_factory=list, kw_only=False). This exception wouldn't get hit?
| raise AgentChatException("No chat history available.") | ||
|
|
||
| # Preprocess chat history | ||
| if await agent.reduce_history(self): |
There was a problem hiding this comment.
Apologies - this chat history reducer stuff was influx as you were doing this -- I removed the reduce_history method from the agent. The history reduce is called directly on the reducer object now, outside of this -- and for a group chat, there's a specific agent group chat message to reduce the history.
| # Agent Model | ||
| agent_model: BedrockAgentModel | None = None | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
Forgot to include this on how instructions/prompt template config is handled, with precedence to prompt_template_config's template for instructions:
| AGENT_ROLE_AMAZON_RESOURCE_NAME=[YOUR_AGENT_ROLE_AMAZON_RESOURCE_NAME] | ||
| FOUNDATION_MODEL=[YOUR_FOUNDATION_MODEL] |
There was a problem hiding this comment.
Are these env variable names fixed or up to us? If up to us, it would be good to have 'bedrock' in there somewhere.
| if CREATE_NEW_AGENT: | ||
| bedrock_agent = await use_new_agent(AGENT_NAME, INSTRUCTION) | ||
| else: | ||
| bedrock_agent = await use_existing_agent(AGENT_ID, AGENT_NAME) |
There was a problem hiding this comment.
To keep it simple I would suggest separating the new and existing scenarios into separate samples. That will allow people to navigate to the scenario that fit's their needs and copy paste to get started.
There was a problem hiding this comment.
To clarify, I think the rest of the samples are ok to support new and existing with a flag, but it would be good to start with a single sample for chat using an existing agent and one using new agent creation.
| cls, | ||
| agent_name: str, | ||
| foundation_model: str, | ||
| role_arn: str, |
There was a problem hiding this comment.
This might be obvious to others, but I can't tell what role_arn is from the name. Is there something more descriptive?
Motivation and Context
Address #10284
Integration of AWS Bedrock agent to SK Python
Description
Contribution Checklist