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

fix: convert_messages drops tool_call_id field (and other fields) #1164 #1170

Closed

Conversation

chiradeepv
Copy link

@chiradeepv chiradeepv commented Nov 12, 2024

Please use conventional commits to describe your changes. For example, feat: add new feature or fix: fix a bug. If you are unsure, leave the title as ... and AI will handle it.

Describe your changes

As described in #1164, when the message is of the form {"role": "tool"...}, convert_messages drops essential fields.
...

Issue ticket number and link

#1164

Checklist before requesting a review

  • [x ] I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.
  • If it is a core feature, I have added documentation.

Important

Fixes convert_messages in multimodal.py to retain all fields in message dictionaries when content is a string.

  • Behavior:
    • Fixes convert_messages in multimodal.py to retain all fields in message dictionaries when content is a string.
  • Misc:
    • Minor comment and formatting adjustments in multimodal.py.

This description was created by Ellipsis for 23f938c. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 23f938c in 9 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. instructor/multimodal.py:359
  • Draft comment:
    The change from {"role": role, "content": content} to message ensures that all fields in the message, such as 'tool_call_id', are preserved. This is consistent with the PR's intent to prevent dropping essential fields.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in line 359 is intended to preserve the entire message object, including any additional fields like 'tool_call_id'. This is a valid change as it aligns with the PR description and issue convert_messages throws away required data (openai tool calling) #1164.
2. instructor/multimodal.py:359
  • Draft comment:
    Ensure that the message object is consistently transformed before appending to converted_messages. This will maintain a consistent data structure.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The function convert_messages has a line where it appends the entire message object to converted_messages without any transformation. This is inconsistent with the rest of the function where the content is transformed before appending. This could lead to inconsistent data structures in converted_messages.

Workflow ID: wflow_RGZ9KxdEUEX0frlu


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@chiradeepv chiradeepv changed the title convert_messages drops tool_call_id field (and other fields) #1164 fix: convert_messages drops tool_call_id field (and other fields) #1164 Nov 12, 2024
@chris-sanders
Copy link
Contributor

chris-sanders commented Nov 19, 2024

+1 I was trying to do this too and hit this same issue. As far as I can tell you can't use tool_calling with OpenAI until this is landed, can we get a release with this?

I've tested this edit and it resolves the issue for me.

Edit: nevermind this didn't fully handle it I'm going to open a PR with a few more changes.

@chris-sanders
Copy link
Contributor

Ok here's the solution that works now for me with 1.6.4 as a base: #1194

@jxnl jxnl closed this Nov 23, 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

Successfully merging this pull request may close these issues.

3 participants