-
Couldn't load subscription status.
- Fork 1.2k
feat: Add OpenAI Conversations API #3429
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
feat: Add OpenAI Conversations API #3429
Conversation
5251b17 to
2843862
Compare
c6844e4 to
03d0116
Compare
| # Generate item ID based on item type | ||
| random_bytes = secrets.token_bytes(24) | ||
| item_type = getattr(item, "type", None) | ||
| if item_type == "message": |
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.
we'll have to handle other types as they come up.
03d0116 to
4bc51be
Compare
| ] | ||
| register_schema(ConversationItem, name="ConversationItem") | ||
|
|
||
| # Using OpenAI types directly caused issues but some notes for reference: |
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.
noting this as it was a bit troublesome.
| created_items.append(item_dict) | ||
|
|
||
| # Get existing items from database | ||
| record = await self.sql_store.fetch_one(table="openai_conversations", where={"id": conversation_id}) |
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.
this seems like the wrong semantic. I don't think the API ever says one is appending. a create is a create.
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.
a create is a create.
Agreed! But this is a create for the /conversations/{conversation_id}/items API so we are adding items to the conversation.
Let me know if I misunderstood you though.
| adapter: TypeAdapter[ConversationItem] = TypeAdapter(ConversationItem) | ||
| response_items: list[ConversationItem] = [adapter.validate_python(item_dict) for item_dict in created_items] | ||
|
|
||
| return ConversationItemList( |
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.
why are we returning the list again? I think the spec says it returns a Conversation object on create
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.
Updated based on other comment.
Signed-off-by: Francisco Javier Arceo <[email protected]>
Signed-off-by: Francisco Javier Arceo <[email protected]>
…ls, fixed first/last id, and updated tests Signed-off-by: Francisco Javier Arceo <[email protected]>
Signed-off-by: Francisco Javier Arceo <[email protected]>
Signed-off-by: Francisco Javier Arceo <[email protected]>
a1cdd9a to
bfdae80
Compare
Signed-off-by: Francisco Javier Arceo <[email protected]>
bfdae80 to
0dbc522
Compare
| ... | ||
|
|
||
| @webmethod(route="/conversations/{conversation_id}/items", method="POST", level=LLAMA_STACK_API_V1) | ||
| async def create(self, conversation_id: str, items: list[ConversationItem]) -> ConversationItemList: |
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.
this method should not be create but add_items or extend
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.
done.
| ) | ||
|
|
||
| if items: | ||
| for item in items: |
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.
can we insert in batch?
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.
added!
while our existing implementation of SqlAlchemySqlStoreImpl.insert() supports batch inserts, i did have to update the signature as well as for authorized_sqlstore. I added a test for it.
Signed-off-by: Francisco Javier Arceo <[email protected]>
…pe adjustments and conversations Signed-off-by: Francisco Javier Arceo <[email protected]>
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.
lgtm, thank you
What does this PR do?
Initial implementation for
ConversationsandConversationItemsusingAuthorizedSqlStorewith endpoints to:Set
level=LLAMA_STACK_API_V1.NOTE: This does not currently incorporate changes for Responses, that'll be done in a subsequent PR.
Closes #3235
Test Plan
Also comparison of OpenAPI spec for OpenAI API
Note I still have some uncertainty about this, I borrowed this info from @cdoern on #3514 but need to spend more time to confirm it's working, at the moment it suggests it does.
UPDATE on
oasdiff, I investigated the OpenAI spec further and it looks like currently the spec does not list Conversations, so that analysis is useless. Noting for future reference.