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

Correcting tool calling with Cohere #3271

Merged
merged 4 commits into from
Aug 3, 2024
Merged

Conversation

jaygdesai
Copy link
Contributor

@jaygdesai jaygdesai commented Aug 1, 2024

Key in the directory should be 'message' and not 'content' as it checks for message empty at a later point in code.

Why are these changes needed?

When using Cohere API, internally in the file cohere.py, there is a mapping from openai tool calling format to cohere tool calling format. While using Cohere to perform tool calling, I encounted an error which ultimately resulted in the error message as under:
BadRequestError: status_code: 400, body: {'message': 'invalid request: all elements in history must have a message.'}
Upon deeper investigation and back-tracing the error step-by-step, it was found that text of a key in a directory is checked to be 'message' instead of 'content' which is present in the code. As this condition is checked in the code, it results in an error as described above. Hence, changing it to 'message'.

Related issue number

#3270

Checks

Key in the directory should be 'message' and not 'content' as it checks for message empty at a later point in code.
Added required comments to the changes made in previous commit.
@marklysze
Copy link
Collaborator

Hey @jaygdesai, thanks so much for finding this bug. Would you be able to list some code that caused the crash? I'll test the PR, thanks!

@marklysze marklysze added the models Pertains to using alternate, non-GPT, models (e.g., local models, llama, etc.) label Aug 1, 2024
@jaygdesai
Copy link
Contributor Author

Hey @jaygdesai, thanks so much for finding this bug. Would you be able to list some code that caused the crash? I'll test the PR, thanks!

Added code that produces crash as a comment in bug issue here

Copy link
Collaborator

@marklysze marklysze left a comment

Choose a reason for hiding this comment

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

Worked with test program, nice one, thanks!

@marklysze
Copy link
Collaborator

Hey @jaygdesai, thanks so much for finding this bug. Would you be able to list some code that caused the crash? I'll test the PR, thanks!

Added code that produces crash as a comment in bug issue here

Hey @jaygdesai, perfect - thanks for sharing that. I've tested it and, yep, it fixes the bug. thank you!

@qingyun-wu, this is good to go :)

@jaygdesai
Copy link
Contributor Author

@marklysze I clicked on 'Update Repository' (wasn't sure if it was not supposed to be clicked as this is my first contribution). Please check if it needs approval again for merge - as it is showing that Workflow is awaiting approval. Thanks.

@marklysze
Copy link
Collaborator

Thanks @jaygdesai, running checks now, no problem...

@qingyun-wu qingyun-wu added this pull request to the merge queue Aug 3, 2024
Merged via the queue into microsoft:main with commit 03bfb8f Aug 3, 2024
137 of 151 checks passed
@Alaya-Con
Copy link

which version of pyautogen has fix this bug?

@marklysze
Copy link
Collaborator

which version of pyautogen has fix this bug?

Hi @Alaya-Con, this bug will be in the current release. The next release will have this corrected.

victordibia pushed a commit that referenced this pull request Aug 28, 2024
* Update cohere.py

Key in the directory should be 'message' and not 'content' as it checks for message empty at a later point in code.

* Update cohere.py

Added required comments to the changes made in previous commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
models Pertains to using alternate, non-GPT, models (e.g., local models, llama, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants