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

LMM: use _generate_oai_reply_from_client #58

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BabyCNM
Copy link
Collaborator

@BabyCNM BabyCNM commented Oct 6, 2024

Why are these changes needed?

This PR only has one-line change (as major change) and a new test case. However, it addresses several issues mentioned before:
microsoft#2550
microsoft#3507

Details:

In 2024, we introduced the function _generate_oai_reply_from_client in Conversable Agent (microsoft#1575), which can handle function calling, model dumping, reflection, and many other great features. However, this change is not updated in the MultimodalConversableAgent, which causes lots of issues afterwards for the multimodal agent in function calling, group chat, and many other locations.

The fix is simple.

Updates:

  1. In MultimodalConversableAgent, override the OAI's client call with _generate_oai_reply_from_client.
  2. We also added a new test cases accordingly.
  3. We now use gpt-4-turbo as the default model instead of gpt-4-vision-preview, as the preview model is deprecated by OpenAI now.

Related issue number

Checks

@BabyCNM
Copy link
Collaborator Author

BabyCNM commented Oct 6, 2024

NOTE: the test case is failing because of API key. Let me know how to address it.

openai.AuthenticationError: Error code: 401 - {'error': {'message': 'Incorrect API key provided: sk-mocko***************************************only. You can find your API key at https://platform.openai.com/account/api-keys.', 'type': 'invalid_request_error', 'param': None, 'code': 'invalid_api_key'}}

@Hk669
Copy link
Collaborator

Hk669 commented Oct 7, 2024

NOTE: the test case is failing because of API key. Let me know how to address it.

openai.AuthenticationError: Error code: 401 - {'error': {'message': 'Incorrect API key provided: sk-mocko***************************************only. You can find your API key at https://platform.openai.com/account/api-keys.', 'type': 'invalid_request_error', 'param': None, 'code': 'invalid_api_key'}}

We can just skip the tests for this, If it is tested locally on all OS.

@Hk669
Copy link
Collaborator

Hk669 commented Oct 7, 2024

@BabyCNM im on windows, let me know if it is already tested.

@BabyCNM
Copy link
Collaborator Author

BabyCNM commented Oct 7, 2024

@Hk669 Thanks! I have tested in Mac OS.

@qingyun-wu
Copy link

@BabyCNM im on windows, let me know if it is already tested.

@Hk669 did you test it on windows? Thank you!

Copy link

@qingyun-wu qingyun-wu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! This change simplifies the LMM agent a lot! Thanks for the contribution!

@Hk669
Copy link
Collaborator

Hk669 commented Oct 20, 2024

@Hk669 did you test it on windows? Thank you!

I couldn't test it, let me do it tomorrow. But it looks good to me though.

@sonichi
Copy link
Collaborator

sonichi commented Oct 21, 2024

The LMM tests should be skiped when skip-openai is specified. Please add a skip condition similar to other tests in contrib tests, and add the test back to contrib-openai CI.
@qingyun-wu I think the LMM tests were removed from contrib-openai CI before. Could you chime in if there's an issue that prevents this test to be added to CI?

@qingyun-wu
Copy link

The LMM tests should be skiped when skip-openai is specified. Please add a skip condition similar to other tests in contrib tests, and add the test back to contrib-openai CI. @qingyun-wu I think the LMM tests were removed from contrib-openai CI before. Could you chime in if there's an issue that prevents this test to be added to CI?

Ok. Will take a look at this and get back soon.

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

Successfully merging this pull request may close these issues.

4 participants