_summarize_chat can return a non-string object #2630
Replies: 5 comments 5 replies
-
I think part of the issue is that register_reply expects a structure like generate_oai_reply: def generate_oai_reply(
self,
messages: Optional[List[Dict]] = None,
sender: Optional[Agent] = None,
config: Optional[OpenAIWrapper] = None,
) -> Tuple[bool, Union[str, Dict, None]]:
"""Generate a reply using autogen.oai."""
client = self.client if config is None else config
if client is None:
return False, None
if messages is None:
messages = self._oai_messages[sender]
extracted_response = self._generate_oai_reply_from_client(
client, self._oai_system_message + messages, self.client_cache
)
return (False, None) if extracted_response is None else (True, extracted_response) But the summarization logic jumps to use _generate_oai_reply_from_client directly, which looks like this:
With this in mind, seems like the following changes could make sense:
Then, _summarize_chat returns the second value of the tuple if the first value is true, else returns empty. |
Beta Was this translation helpful? Give feedback.
-
I think this makes sense. Do you want to create a PR to address the issue? |
Beta Was this translation helpful? Give feedback.
-
Sounds good. What solution do you think makes the most sense? My inclination is to allow the summary parameter to be Union[str, Dict, None]. A little unrelated, but I'm building the logic to have chatmanagers be in charge of creating a json/dict summary of the "task" (the assumption here is that each chat resolution is considered a task, with a set of inputs and a string output), and the idea is that the summary would be a dict that would track the "outcome" of the task/conversation. I'm using guidance to force the grammar and retrieve the values instead of doing some regex retrieval, hoping this will make it consistent. Looks something like this: def generate_json_summary(recipient, messages: List[Union[str, dict]], sender, config)-> Tuple[bool, Union[str, Dict, None]]:
# config = recipient.llm_config if config is None else config
# if config is None:
# return False, None
# TODO: Consolidate the guidance model building with Autogen's model config
model = guidance_llama_8b_chat()
if messages is None:
messages = recipient._oai_messages[sender]
with system():
lm = model + recipient.system_message
for message in messages:
if isinstance(message, str):
with user():
lm += message
else:
if message.get("role") == "user":
with user():
lm += message.get("content")
else:
with assistant():
lm += message.get("content")
status_options = ["complete", "pending", "failed"]
code_options = [0,1,2]
with assistant():
lm += f"""
TASK SUMMARY:
- **status**: {select(status_options, 'status')}
- **result_code**: {select(code_options, 'result_code')}
- **result_description**: {gen('result_description', stop='- **')}
- **result_content**:
```
{gen('result_content', stop='- **')}
```
- **result_diagnostic**: {gen('result_diagnostic', max_tokens=200, stop="TERMINATE")}
TERMINATE
---
"""
response = lm._variables
if response is None:
return False, None
else:
return True, response Basically, the ChatManager agent has this callable registered as a reply, with an accompanying system_message to hopefully generate consistent results. The unfortunate part is that (this implementation at least) requires a local model so that guidance can restrict the grammar, but I have a feeling that this can be solved with a slightly different logic. This is just to say, I think it makes sense for the ChatResult.summary to potentially be a dict. |
Beta Was this translation helpful? Give feedback.
-
I was thinking about the _summarize_chat() logic and was wondering what you thought: def _summarize_chat(
self,
summary_method,
summary_args,
recipient: Optional[Agent] = None,
cache: Optional[AbstractCache] = None,
) -> str:
"""Get a chat summary from an agent participating in a chat.
Args:
summary_method (str or callable): the summary_method to get the summary.
The callable summary_method should take the recipient and sender agent in a chat as input and return a string of summary. E.g,
```python
def my_summary_method(
sender: ConversableAgent,
recipient: ConversableAgent,
summary_args: dict,
):
return recipient.last_message(sender)["content"]
```
summary_args (dict): a dictionary of arguments to be passed to the summary_method.
recipient: the recipient agent in a chat.
prompt (str): the prompt used to get a summary when summary_method is "reflection_with_llm".
Returns:
str: a chat summary from the agent.
"""
summary = ""
if summary_method is None:
return summary
if "cache" not in summary_args:
summary_args["cache"] = cache
if summary_method == "reflection_with_llm":
summary_method = self._reflection_with_llm_as_summary
elif summary_method == "last_msg":
summary_method = self._last_msg_as_summary
if isinstance(summary_method, Callable):
summary = summary_method(self, recipient, summary_args)
else:
raise ValueError(
"If not None, the summary_method must be a string from [`reflection_with_llm`, `last_msg`] or a callable."
)
return summary def initiate_chat(
...
summary = self._summarize_chat(
summary_method,
summary_args,
recipient,
cache=cache,
) Right now, the idea is that: I'm thinking it would be useful for _reflection_with_llm_as_summary and _last_msg_as_summary to have the structure of a reply_func: def reply_func(
recipient: ConversableAgent,
messages: Optional[List[Dict]] = None,
sender: Optional[Agent] = None,
config: Optional[Any] = None,
) -> Tuple[bool, Union[str, Dict, None]]: This means that the "summary_method" callable can have a compatible structure to reply_func. This could look something like this: def _summarize_chat(
self,
summary_method,
summary_args,
recipient: Optional[Agent] = None,
cache: Optional[AbstractCache] = None,
) -> Union[str, Dict, None]:
"""Get a chat summary from an agent participating in a chat.
Args:
summary_method (str or callable): the summary_method to get the summary.
The callable summary_method should take the recipient and sender agent in a chat as input and return a string of summary. E.g,
```python
def my_summary_method(
sender: ConversableAgent,
recipient: ConversableAgent,
summary_args: dict,
):
return recipient.last_message(sender)["content"]
```
summary_args (dict): a dictionary of arguments to be passed to the summary_method.
recipient: the recipient agent in a chat.
prompt (str): the prompt used to get a summary when summary_method is "reflection_with_llm".
Returns:
str: a chat summary from the agent.
"""
summary = ""
messages = recipient.chat_messages_for_summary(self)
if summary_method is None:
return summary
if "cache" not in summary_args:
summary_args["cache"] = cache
if summary_method == "reflection_with_llm":
summary_method = self._reflection_with_llm_as_summary
messages = self.add_summary_prompt_message(messages, summary_args)
elif summary_method == "last_msg":
summary_method = self._last_msg_as_summary
if isinstance(summary_method, Callable):
bool, summary = summary_method(self, messages, recipient)
else:
raise ValueError(
"If not None, the summary_method must be a string from [`reflection_with_llm`, `last_msg`] or a callable."
)
return summary
@staticmethod
def add_summary_prompt_message(messages, summary_args):
prompt = summary_args.get("summary_prompt")
prompt = ConversableAgent.DEFAULT_SUMMARY_PROMPT if prompt is None else prompt
if not isinstance(prompt, str):
raise ValueError("The summary_prompt must be a string.")
role = summary_args.get("summary_role", "system")
if not isinstance(role, str):
raise ValueError("The summary_role in summary_arg must be a string.")
system_msg = [
{
"role": role,
"content": prompt,
}
]
messages = messages + system_msg
return messages
@staticmethod
def _last_msg_as_summary(sender, messages, recipient, config) -> Tuple[bool, Union[str, Dict, None]]:
"""Get a chat summary from the last message of the recipient."""
summary = ""
try:
content = messages[-1]["content"]
if isinstance(content, str):
summary = content.replace("TERMINATE", "")
elif isinstance(content, list):
# Remove the `TERMINATE` word in the content list.
summary = "\n".join(
x["text"].replace("TERMINATE", "") for x in content if isinstance(x, dict) and "text" in x
)
except (IndexError, AttributeError) as e:
warnings.warn(f"Cannot extract summary using last_msg: {e}. Using an empty str as summary.", UserWarning)
return (False, None) if summary is None else (True, summary)
@staticmethod
def _reflection_with_llm_as_summary(sender, messages, recipient, config) -> Tuple[bool, Union[str, Dict, None]]:
try:
if recipient and recipient.client is not None:
llm_client = recipient.client
elif sender.client is not None:
llm_client = sender.client
else:
raise ValueError("No OpenAIWrapper client is found.")
return recipient.generate_oai_reply(llm_client=llm_client, messages=messages, cache=cache)
except BadRequestError as e:
warnings.warn(
f"Cannot extract summary using reflection_with_llm: {e}. Using an empty str as summary.", UserWarning
)
summary = ""
return (False, None) if summary is None else (True, summary) This would allow a user that builds any reply_func to deploy it with either register_reply or summary_method without making any adjustments. It provides a consistent interface for creating agent response functions.
|
Beta Was this translation helpful? Give feedback.
-
After reviewing this, I am convinced it is correct as is. From the summary_chat() method:
Seems fairly clear here that the expectation is that the summary_method returns a str, and has a specific signature of (sender, recipient, summary_args). This means that functions used to create/register agent responses cannot be used directly as the summary_method. If the idea is to keep summary strictly as a string, this makes sense and doesn't need adjustments. |
Beta Was this translation helpful? Give feedback.
-
The _summarize_chat method is defined as so:
Seems that it is expected for registered replies to return a non-string result (this is a snippet from the register_reply docstrings):
Moreover, ChatResult seems to expect summary to be a string:
To clarify, right now this means _summarize_chat returns a tuple when using a registered reply callable, which is clearly not the expected behavior.
Its a minor thing, but I think it would be a good idea to:
Beta Was this translation helpful? Give feedback.
All reactions