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

refactor(message): update message api types #199

Merged
merged 31 commits into from
Feb 5, 2025
Merged

Conversation

0xMochan
Copy link
Contributor

@0xMochan 0xMochan commented Jan 13, 2025

completion::Message refactor

This PR performs a very large refactor for how messages are handled. There are 2 parts that are combined to allow for full API access to all providers.

Message struct

This PR defines a general Message definition that serves to be the singular data point for message histories alongside the interface for prompting moving forward. This interface encapsulates all of the different input and responses you might get when conversing with any provider, including multi-modal inputs (images, etc), tool calls and results, and assistant powered replies. It covers a vast majority of provider specific and general features.

From and TryFrom implementations

Each provider now has it's own set of Message structs representing a closer model of the provider specific API. The provider is responsible for implementing a From and/or TryFrom to convert between the generic message <-> provider specific message converting the data between the formats.

Prompts are supplied as Into<Message> and message history are Message. Providers then convert this into a single stream of their own Message to then pass to the provider's API calls. When a response is given, the reverse is made.

Future

In the future, there will be more implementations of Into<Message> and other abstractions that'll build upon this pattern. This includes further abstractions to make working with images, documents, and audio with specific provider apis more seamless, making tool calling loops easier (prompt -> assistant -> tool call -> tool result -> assistant -> response), and easier management of chat histories.

Caveats

This PR has gotten too large, but to avoid it growing even further, some concessions were made. These should result in their own issues for further discussion:

  • I found that EternalAI currently is able to accidentally have 2 system messages which would cause an issue with the provider.
  • Gemini function arguments are oddly configured and likely needs a second look at
  • completion::completion2 is poorly named (can be resolved in the current PR).
  • message::UserContent(Text), message::AssistantContent(Text), and message::ToolResultContent(Text) take in message::Text but could directly take in a String for simplicity
  • ToolResult is placed under UserContent instead of another top-level enum variant.

Copy link
Contributor

@cvauclair cvauclair left a comment

Choose a reason for hiding this comment

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

Amazing work, that was a big one!

Couple comments while I wait for the final commits

@0xMochan 0xMochan marked this pull request as ready for review January 28, 2025 03:29
@0xMochan 0xMochan requested a review from cvauclair January 28, 2025 03:48
Copy link
Contributor

@carlos-verdes carlos-verdes left a comment

Choose a reason for hiding this comment

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

I find the PR super big, we can try to create smaller PRs to cover these changes progressively.

I left some comments

Copy link
Contributor

@cvauclair cvauclair left a comment

Choose a reason for hiding this comment

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

Excellent work @0xMochan !

Found a couple bugs but overall this looks 🔥

@carlos-verdes
Copy link
Contributor

I really need this PR in order to finish the tool calls issue (send the tool response back to the model to get human readable answers).
Is there anything I can do to help closing this PR faster?

@0xMochan
Copy link
Contributor Author

0xMochan commented Feb 4, 2025

I really need this PR in order to finish the tool calls issue (send the tool response back to the model to get human readable answers). Is there anything I can do to help closing this PR faster?

This is planned to merge in 24 hours!

@0xMochan 0xMochan requested a review from cvauclair February 4, 2025 18:47
Copy link
Contributor

@cvauclair cvauclair left a comment

Choose a reason for hiding this comment

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

Couple of docs changes but after that let's send it! Amazing work! 🔥

@0xMochan
Copy link
Contributor Author

0xMochan commented Feb 4, 2025

LGTM!!!!

@0xMochan 0xMochan mentioned this pull request Feb 4, 2025
Copy link
Contributor

@cvauclair cvauclair left a comment

Choose a reason for hiding this comment

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

Send it! 🚀

@cvauclair cvauclair merged commit fc64717 into main Feb 5, 2025
5 checks passed
@github-actions github-actions bot mentioned this pull request Feb 4, 2025
@carlos-verdes
Copy link
Contributor

This PR breaks DeepSeek provider, the problems is how DeepSeek provider builds the client, I'll fix this on my next PR

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