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 non-function calls messages #5026

Merged
merged 6 commits into from
Nov 21, 2024
Merged

Fix non-function calls messages #5026

merged 6 commits into from
Nov 21, 2024

Conversation

enyst
Copy link
Collaborator

@enyst enyst commented Nov 15, 2024

End-user friendly description of the problem this fixes or functionality that this introduces

  • Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below
  • fix 400 error on HuggingFace models
  • fix KeyError on multiple LLMs

Give a summary of what the PR does, explaining any non-trivial design decisions

This PR proposes a refactoring of Messages and function calling. It will fix the LLMs without function calling enabled:

  • 400 error on HF for top_p=1
  • 'content' error on many LLMs
  • serialization of tool-like messages
  • misc

HF model example that works:
Screenshot 2024-11-15 at 14 23 55


Link of any specific issues this addresses
Fix #4960
Fix #5090
Fix #5142


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:f6e70f5-nikolaik   --name openhands-app-f6e70f5   docker.all-hands.dev/all-hands-ai/openhands:f6e70f5

@enyst enyst requested a review from xingyaoww November 15, 2024 14:55
Copy link
Collaborator

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

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

Except for some concern in adding function_calling_enabled -- otherwise LGTM!

openhands/core/message.py Show resolved Hide resolved
openhands/core/message.py Outdated Show resolved Hide resolved
@@ -567,6 +567,7 @@ def format_messages_for_llm(self, messages: Message | list[Message]) -> list[dic
for message in messages:
message.cache_enabled = self.is_caching_prompt_active()
message.vision_enabled = self.vision_is_active()
message.function_calling_enabled = self.is_function_calling_active()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will only be True for the selected set of model -- but we still send messages WITH tool_ids to the LLM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh so we need the part of the serializer that adds tool_call_id. OK, sure, I'll revert that. Sigh, this is messy. The attempt to separate things when function calling is native is because we need to send a single thing, not a list.

(I'll follow up on litellm too, they actually do this kind of conversion, so we don't have to play whack-a-mole anymore)

Copy link
Collaborator Author

@enyst enyst Nov 21, 2024

Choose a reason for hiding this comment

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

@xingyaoww I actually restored this check, but it does send tool ids now. It just sends them as string, if non-native function calling, and as list if native. (because this applies now)

I started to work on refactoring the Message class. I think it needs to include tool calls/ids conversion part (at least as a single Message), do full serialization, and become closer to litellm's Message. I started to work on it on this branch, but in the meantime, I found out that people rely on this PR, and use it via docker because it solves what's broken with non-native function calling.

So I'll make another branch to work on the refactoring, and this PR is up for review again. If it's not too terrible, I'd say let's just fix the problems first, and refactor later.

(I'll clean up some leftover comment, too, but I prefer to do it if it's approved, because I think it would again break people's use via docker on the last commit.)

openhands/llm/llm.py Show resolved Hide resolved
@enyst enyst marked this pull request as draft November 15, 2024 15:34
@enyst enyst marked this pull request as ready for review November 21, 2024 00:42
@enyst enyst requested a review from xingyaoww November 21, 2024 01:06
@neubig
Copy link
Contributor

neubig commented Nov 21, 2024

Updated the comment to note this fixes Fix #5142

Copy link
Collaborator

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@enyst enyst enabled auto-merge (squash) November 21, 2024 17:57
@enyst enyst merged commit d08886f into main Nov 21, 2024
13 checks passed
@enyst enyst deleted the enyst/conversion-fixes branch November 21, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants