Skip to content
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

[Bug]: name Field Compatibility Issue in AssistantAgent and UserProxyAgent with fireworks.ai API #1129

Closed
zhaoyilun opened this issue Jan 3, 2024 · 13 comments

Comments

@zhaoyilun
Copy link

Describe the bug

Description

When interfacing with the fireworks.ai API using the AssistantAgent and UserProxyAgent classes, I've encountered a compatibility issue due to the mandatory name field within these classes. The fireworks.ai API rejects the request with an InvalidRequestError because the name field is not expected.

Steps to reproduce

  1. Use AssistantAgent or UserProxyAgent with the name field included in the request payload.
  2. Send a request to the fireworks.ai API endpoint.
  3. The API responds with an InvalidRequestError, indicating the name field is extraneous.

Expected Behavior

Expetced Behavior

There should be compatibility with fireworks.ai API, which means either the name field should be accepted, or there should be a way to exclude it from requests depending on the API requirements.

Actual Behavior

The request is rejected by the fireworks.ai API, and the following error message is received:
openai.error.InvalidRequestError: [{'loc': ('body', 'messages', 1, 'name'), 'msg': 'extra fields not permitted', 'type': 'value_error.extra'}]

Possible Solutions

  • Allow conditional inclusion of the name field in the request payload based on the API being used.
  • Modify the AssistantAgent and UserProxyAgent classes to make the name field optional or provide a method to exclude it for certain APIs.

Concerns

While a temporary fix could involve modifying the request information within the classes, this could lead to additional issues upon future updates. Thus, a permanent solution from the autogen team would be preferable to ensure consistent support for various models.

Additional Context

  • The issue occurred with models mixtral-8x7b-instruct and llama-v2-7b-chat.
  • I am willing to implement a temporary fix by modifying the request data myself, but due to work commitments, it might take 3-5 days before I can address this.
  • There is a concern that manual code modifications might introduce new issues with future version updates.

Request

I would appreciate guidance on how this issue might be resolved on an official level. It's not clear whether this is an issue with the settings on fireworks.ai's side or if the model itself does not support the name field. I hope that autogen can officially support multiple models to ensure broad compatibility.

Screenshots and logs

Traceback (most recent call last):
File "/home/zyl/chatdev/autogen/sqlFormat/demo.py", line 57, in
user_proxy.initiate_chat(
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/agentchat/conversable_agent.py", line 531, in initiate_chat
self.send(self.generate_init_message(**context), recipient, silent=silent)
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/agentchat/conversable_agent.py", line 334, in send
recipient.receive(message, self, request_reply, silent)
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/agentchat/conversable_agent.py", line 462, in receive
reply = self.generate_reply(messages=self.chat_messages[sender], sender=sender)
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/agentchat/conversable_agent.py", line 781, in generate_reply
final, reply = reply_func(self, messages=messages, sender=sender, config=reply_func_tuple["config"])
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/agentchat/groupchat.py", line 162, in run_chat
speaker = groupchat.select_speaker(speaker, self)
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/agentchat/groupchat.py", line 91, in select_speaker
final, name = selector.generate_oai_reply(
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/agentchat/conversable_agent.py", line 606, in generate_oai_reply
response = oai.ChatCompletion.create(
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/oai/completion.py", line 803, in create
response = cls.create(
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/oai/completion.py", line 834, in create
return cls._get_response(params, raise_on_ratelimit_or_timeout=raise_on_ratelimit_or_timeout)
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/autogen/oai/completion.py", line 224, in _get_response
response = openai_completion.create(request_timeout=request_timeout, **config)
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/openai/api_resources/chat_completion.py", line 25, in create
return super().create(*args, **kwargs)
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/openai/api_resources/abstract/engine_api_resource.py", line 157, in create
response, _, api_key = requestor.request(
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/openai/api_requestor.py", line 299, in request
resp, got_stream = self._interpret_response(result, stream)
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/openai/api_requestor.py", line 713, in _interpret_response
self._interpret_response_line(
File "/home/zyl/miniconda3/envs/pyautogen/lib/python3.10/site-packages/openai/api_requestor.py", line 779, in _interpret_response_line
raise self.handle_error_response(
openai.error.InvalidRequestError: [{'loc': ('body', 'messages', 1, 'name'), 'msg': 'extra fields not permitted', 'type': 'value_error.extra'}]

Additional Information

AutoGen Version: 0.1.14
Operating System: Linux version 5.15.133.1-microsoft-standard-WSL2 (root@1c602f52c2e4) (gcc (GCC) 11.2.0, GNU ld (GNU Binutils) 2.37) #1 SMP Thu Oct 5 21:02:42 UTC 2023 Ubuntu 22.04.3 LTS
Python Version:Python 3.10.13
Related Issues: not found


Looking forward to your response and a possible solution. Thank you!

@zhaoyilun zhaoyilun added the bug label Jan 3, 2024
@ekzhu
Copy link
Collaborator

ekzhu commented Jan 3, 2024

Thanks for the issue. I think this should be a feature request not a bug. Do you know if there is a reason that fireworks.ai is not compatible with OpenAI API?

@zhaoyilun
Copy link
Author

zhaoyilun commented Jan 3, 2024

Thanks for the issue. I think this should be a feature request not a bug. Do you know if there is a reason that fireworks.ai is not compatible with OpenAI API?

I apologize for choosing the incorrect method to raise an issue; it was an oversight on my part. I have posted the query along with the relevant logs on the fireworks.ai Discord channel and am currently unsure of the cause. However, upon reviewing the documentation for the OpenAI API, I did not find any mention of using the 'name' field within 'message' field. Thank you for your response, and I am hopeful that a solution can be found quickly. It would be fantastic to have Autogen work seamlessly with APIs provided by most mainstream teams. Each model has its unique strengths, and the prospect of integrating an assistant agent that's already equipped with a knowledge base is particularly exciting – the potential synergy is thrilling to consider.

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 3, 2024

No worries. Could you see if this problem still exists in the latest version?

@afourney
Copy link
Member

afourney commented Jan 3, 2024

So the name field is pretty sparingly documented by OpenAI -- it's easy to miss. I am not surprised there are implementations that lack it.

Having said that, we use this field for bookkeeping in GroupChat as well, and I would be nervous about removing it or adding yet another flag. I wonder if there is something we can do that is more general. Perhaps allowing one to provide a validator function in llm_config that could handle last minute transformations right at the moment where the LLM is called. Or perhaps abstracting the Client, so that people can use custom implementations.

TL:DR; I have a strong preference to adding general hooks rather than addressing incompatibilities filed by field, or provider by provider.

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 4, 2024

I remember at one-point OpenAI supports the "name" field, perhaps back in chatml days. @afourney could you point out one example of our code base that uses the "name" field?

@afourney
Copy link
Member

afourney commented Jan 4, 2024

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 4, 2024

@zhaoyilun
Copy link
Author

No worries. Could you see if this problem still exists in the latest version?

Yes, I updated to version 0.2.2, but the issue persists:

openai.BadRequestError: Error code: 400 - {'error': {'object': 'error', 'type': 'invalid_request_error', 'message': "[{'loc': ('body', 'messages', 1, 'name'), 'msg': 'extra fields not permitted', 'type': 'value_error.extra'}]"}}

@BeibinLi
Copy link
Collaborator

BeibinLi commented Jan 4, 2024

Many different ways to resolve this issue, but all would require some coding.

Solution 1: Create a custom _message_to_dict function

Note: this is a hack, because it overrides a private member function (not good coding practice). The advantage is: you can use it without creating a brand-new class; useful if you are in a rush. See Solution 2 for an actual better design.

We need to remove the "name" filed in the _message_to_dict function in ConversableAgent. However, this change would mess up some features in group chat, where the name could be useful for a chat history. So, we need to add back the "name" filed into the message.

def firework_message_to_dict(message: Union[Dict, str]) -> Dict:
    """Convert a message to a dictionary.

    The message can be a string or a dictionary. The string will be put in the "content" field of the new dictionary.
    """
    if isinstance(message, str):
        message = {"content": message}
    elif isinstance(message, dict):
        pass
    else:
        message = dict(message)

    name = message.pop("name", None)
    if name:
        if isinstance(message["content"] , str):
            message["content"] = f"{name}: message['content']"
        elif isinstance(message["content"], list):
            message["content"] = [{"type":"text", "text": f"{name}: "}] + message["content"]
    return message

agent = AssistantAgent(name="fake", ...., llm_config=my_custom_llm_config)
agent._message_to_dict = firework_message_to_dict

Solution 2: Create another class FireworkConversableAgent

Create a class, inherit from ConversableAgent, and then override the _message_to_dict function as shown above. The implementation is similar, and this solution would be more design-correct.

You don't need to create a PR either and can use the customized agent class for the Firework API.

Solution 3: use Capability registration

There is an ongoing PR #1091, and we can wait for that PR and then register a preprocessing function to process_last_message

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 4, 2024

My feeling is that we may not be able to prioritize compatibility with every LLM provider. The current approach of using vllm and perhaps as a next step using litellm proxy to achieve OpenAI compatibility should be the goal.

@afourney
Copy link
Member

afourney commented Jan 4, 2024

Agreed. But even the discussion about gpt-v is important here. There may be some fields that some agents just don't care about (e.g., image data), or can't handle, and we should handle that gracefully perhaps.

@sonichi
Copy link
Contributor

sonichi commented Jan 6, 2024

Yes. @rickyloynd-microsoft would the hook methods in #1091 be applicable?

@rickyloynd-microsoft
Copy link
Contributor

Unless I'm missing something, it could be done by modifying _message_to_dict along these lines:

def _message_to_dict(message: Union[Dict, str]) -> Dict:
    """Convert a message to a dictionary.
     The message can be a string or a dictionary. The string will be put in the "content" field of the new dictionary.
    """
    if isinstance(message, str):
        message = {"content": message}
    elif isinstance(message, dict):
        pass
    else:
        message = dict(message)

    # Let any registered hooks modify the message.
    message = self.process_message_dict(message)

    return message

where process_message_dict would be a new hookable method similar to process_last_message. Then a new AgentCapability subclass like SupportFireworksAI(AgentCapability) would implement a hook method to remove the name item from a dict, as done by @BeibinLi's code above.

Users could then make any agent compatible with fireworks.ai with one line of code, which would attach the capability's hook method to the agent's hookable method:

SupportFireworksAI().add_to_agent(agent)

If this works, then future message-format-conversion capabilities could be added to agents without further modifications to ConversableAgent.

@ekzhu ekzhu mentioned this issue Jan 6, 2024
3 tasks
@gagb gagb closed this as completed Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants