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.

Sign up for GitHub

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

Jump to bottom

Fix tools for OpenAI messages #1194

Merged
merged 3 commits into from
Nov 22, 2024
Merged

Fix tools for OpenAI messages #1194

merged 3 commits into from
Nov 22, 2024

Conversation

chris-sanders
Copy link
Contributor

@chris-sanders chris-sanders commented Nov 19, 2024 •
edited by ellipsis-dev bot
Loading

Fixes two different issues with tool use and OpenAI

  • OpenAI can (will?) return an empty context when it asks for tool_call. This context is Null and failed to iterate. Null context is replaced with empty list instead.
  • Extra arguments were stripped keeping only role content and type. This prevents OpenAI from recognizing tool calls, the non-processed arguments are passed through untouched to preserve them.

Important

Fixes handling of null contexts and preserves additional arguments in OpenAI messages in convert_messages() in instructor/multimodal.py.

  • Behavior:
    • Replace null content with an empty list in convert_messages() to handle empty contexts from OpenAI.
    • Preserve additional message arguments in convert_messages() by including them in the message dictionary.
  • Misc:
    • Minor changes in convert_messages() to ensure compatibility with OpenAI tool calls.

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

@chris-sanders chris-sanders mentioned this pull request Nov 19, 2024
Closed
2 tasks
ellipsis-dev[bot]
ellipsis-dev bot reviewed Nov 19, 2024
View reviewed changes
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 586300c in 35 seconds

More details
  • Looked at 25 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. instructor/multimodal.py:336
  • Draft comment:
    Ensure that content is expected to be a list when not a string. This change assumes that None should be replaced with an empty list, which might not be the intended behavior in all contexts.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change from content = message["content"] to content = message["content"] or [] is intended to handle cases where content might be None. This is a good change to prevent iteration over None, which would cause an error. However, this change assumes that content should always be a list when it's not a string. This assumption should be verified in the context of the application.
2. instructor/multimodal.py:337
  • Draft comment:
    Good use of other_kwargs to preserve additional message attributes. This ensures that extra information is not lost during conversion.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The addition of other_kwargs ensures that any additional keys in the message dictionary are preserved when creating the new message dictionary. This is important for maintaining any extra information that might be needed later. The change is correctly applied in both places where converted_messages.append is called.
3. instructor/multimodal.py:337
  • Draft comment:
    Use a more descriptive variable name than other_kwargs to indicate what these additional keyword arguments represent.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code changes in the PR involve adding other_kwargs to the converted_messages list. However, the variable name other_kwargs is not very descriptive. It would be better to use a more descriptive name that indicates what these additional keyword arguments represent.
4. instructor/multimodal.py:360
  • Draft comment:
    Use a more descriptive variable name than other_kwargs to indicate what these additional keyword arguments represent. This applies to line 363 as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code changes in the PR involve adding other_kwargs to the converted_messages list. However, the variable name other_kwargs is not very descriptive. It would be better to use a more descriptive name that indicates what these additional keyword arguments represent.

Workflow ID: wflow_tDf49L9HzJKWK1HW

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

@chris-sanders
Copy link
Contributor Author

I'll review those failed tests today unless you know any of them to be expected failures.

@chris-sanders
Copy link
Contributor Author

That fixes black, note that I added it to the dev requirements as well, if you don't want that it's all in one commit easy to revert.

I looked and the failures now seem to be unrelated, running black on non-python files, connection errors in a tests that appear to reach out to the API direct but are failing long before the changes made here.

@jxnl jxnl enabled auto-merge (squash) November 22, 2024 22:09
@jxnl jxnl disabled auto-merge November 22, 2024 22:09
@jxnl jxnl merged commit ad046fb into instructor-ai:main Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellipsis-dev ellipsis-dev[bot] ellipsis-dev[bot] left review comments

@jxnl jxnl jxnl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chris-sanders @jxnl