-
Notifications
You must be signed in to change notification settings - Fork 3k
[router] support Openai router conversation API CRUD #11297
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
Conversation
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces CRUD functionality for conversations, including a ConversationStorage
trait and several implementations (in-memory, no-op, and Oracle). It also exposes these operations via new API endpoints. The implementation is well-structured, particularly the use of spawn_blocking
for synchronous database calls and the clear separation of concerns in the storage modules. I've provided a few suggestions to improve performance in the Oracle store, adhere more closely to Rust idioms, and enhance code readability in the server routing.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces comprehensive CRUD (Create, Read, Update, Delete) functionality for conversations. It adds new storage backends, including in-memory, no-op, and a detailed Oracle implementation, along with the necessary data structures and API endpoints. The changes are well-structured. My review focuses on improving performance by reducing unnecessary data cloning in the Oracle store, fixing a compilation error in the conversation ID generation logic, and suggesting a more efficient and idiomatic approach to JSON serialization in the server handlers. Overall, this is a solid addition of a major feature.
} | ||
} | ||
|
||
async fn v1_conversations_update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should conversation handlers happen in router?
instead of server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. my bad, resolved
|
||
async fn create_conversation(&self, _headers: Option<&HeaderMap>, body: &Value) -> Response { | ||
// Parse metadata: allow object or null/absent | ||
let metadata = match body.get("metadata") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a todo
these spec validation should be elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure added
^^ @slin1237 ci completed, ready to review |
Motivation
Basic support for conversation API CRUD
Modifications
Tests
Benchmarking and Profiling
Checklist