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
feat(streaming): add logger to include status-based messages in chat history file #537
Conversation
There are some considerations here to add:
|
Comments just based on the description (which is really helpful, thanks) before diving into code: Persona: Front End
Persona: Performance Engineer
Persona: Agent SDK Design
Will keep thinking... let me know what's useful/not useful feedback wise, but this is really awesome to read through so far 🚀🚀🚀🚀🚢🚢🚢🚢🚢 |
I am a big fan of this approach generally. I think having all of the inner 'reasoning' steps in the history of the chat makes total sense, and then having them appropriately filtered back out based on tags etc should work really well. I am not particularly concerned about chat history bloat, personally. I think that can be handled via nice filter options. |
…at history file This PR adds context manager capabilities to AgentContext, such that a logs stream of agent, llm, and tool messages will be redirected into a ChatHistory (and appropriately tagged). This will allow our single-file streaming approach to include status messages in the output.
fd56108
to
26092c5
Compare
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.
This looks good to me! The approach makes sense, and nothing jumped out at me in the code.
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.
Sprinkled comments but mostly just talking out loud to myself while reading. This looks great!!
LGTM
"stack_trace": "%(exc_text)s", | ||
"message_type": "%(message_type)s", | ||
"component_name": "%(component_name)s", | ||
AgentLogging.IS_MESSAGE: f"%({AgentLogging.IS_MESSAGE})s", # b doesn't work. Unsure how to make a bool |
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.
Naieve response is the way you probably were avoiding because it felt silly: 'true' if AgentLogging.IS_MESSAGE else 'false'
} | ||
|
||
|
||
class StreamingOpts(BaseModel): |
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.
I like how you've started with the idea that what gets added to the stream can be selected.
One thing that would help me (and therefor I think others?) is to actively add comments here about what distinguishes an agent
and llm
and tool
message.
For me: I find myself wanting to be reassured about my guess about agent
versus llm
specifically.
For ex:
- Is
agent_message
something the agent says that gets added to the chat history? - Is
llm_message
something that the LLM emits to a tool or agent, but doesn't tend to get added to the chat history?
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.
Added some docs. One thing that I wanted to make more distinct in this PR is the boundary between Agent
and LLM
. There is nothing in Agent
that requires LLM
integration or backing. You could have a purely random tool selection agent, for instance. So, I wanted to allow for LLM
s to be responsible for LLM
-scoped messages and Agent
s to focus on the level above in logs. This might come at a cost of a bit more of verbosity, however.
src/steamship/agents/schema/agent.py
Outdated
@@ -49,14 +50,13 @@ def messages_to_prompt_history(messages: List[Block]) -> str: | |||
as_strings = [] | |||
for block in messages: | |||
role = block.chat_role | |||
# DON'T RETURN AGENT MESSAGES -- THOSE ARE INTERNAL STATUS MESSAGES |
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.
Oh interesting. It's not what I guessed above. Yeah little doc notes would def be helpful then :)
return self.append_message_with_role(text, RoleTag.LLM, tags, content, url, mime_type) | ||
|
||
|
||
class ChatHistoryLoggingHandler(StreamHandler): |
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.
I think it's really cool that we're hooking on to the logging system in this way.
# don't bother doing anything if level is below logging level | ||
return | ||
|
||
message_dict = cast(dict, self.format(record)) |
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.
[not high pri; feel free to ignore]
below feels like an opportunity to collapse into one block of code below since so much is repeated.
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.
refactored a bit. the slight diffs between the various modes makes a full collapse uglier than a small bit of near repetition (imho), but the refactor hopefully makes things clearer/more readable.
url=output_block.raw_data_url or output_block.url or output_block.content_url, | ||
mime_type=output_block.mime_type, | ||
) | ||
with self.build_default_context(context_id, **kwargs) as context: |
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.
+1. nice
1b119f8
to
9ad363d
Compare
9ad363d
to
8e58a45
Compare
This PR provides an approach for providing "status"-like events inline in the same "stream" as other File-based (or in the case of agents, History-based) events.
This PR uses a
StreamHandler
to capture existing logs and redirect them into theChatHistory
as blocks tagged with either a role ofAgent
orTool
orLLM
. Those messages are then explicitly ignored when providing blocks for conversation history in LLM interactions (in an attempt to prevent contamination).The
StreamHandler
is added/removed on a request-by-request basis by adding context manager capabilities toAgentContext
.Here is a BEFORE snapshot of an agent history (without status-like messages in stream):
Here is the AFTER: