-
Notifications
You must be signed in to change notification settings - Fork 284
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 chat completion mutation #5255
base: main
Are you sure you want to change the base?
Conversation
provider_key = input.model.provider_key | ||
llm_client_class = PLAYGROUND_CLIENT_REGISTRY.get_client(provider_key, input.model.name) | ||
if llm_client_class is None: | ||
raise BadRequest(f"No LLM client registered for provider '{provider_key}'") |
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 get this returned as an error so it's easier to display in the front end, the FinishedChatCompletion type has an errors field right?
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.
BadRequest
is a custom graphql error, so it should be displayable in the frontend
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.
Relay has an onError
hook https://relay.dev/docs/api-reference/use-mutation/#return-value
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 it just requires a whole lot of parsing to get the real error from the relay error
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.
i see. probably a broader discussion if we want to add error types on mutations / query. we've avoided that pattern for the sake of simplicity so far.
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.
don't we have that on the streaming version of this though? an error member of the union, i guess that is technically not mutation or query (subscription) but was thinking it would set the precedent here
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.
but yes don't need to use it here just saying it can be helpful for handling known errors more gracefully for us, I know it's been shared before but this is the inspiration https://sachee.medium.com/200-ok-error-handling-in-graphql-7ec869aec9bc
and
https://productionreadygraphql.com/2020-08-01-guide-to-graphql-errors
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.
don't we have that on the streaming version of this though? an error member of the union, i guess that is technically not mutation or query (subscription) but was thinking it would set the precedent here
the error types in the subscription were unavoidable because we need to continue yielding payloads (FinishedChatCompletion
in particular) even after an error has occurred. We don't currently have the same pattern on mutations and queries afaik
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.
ahh okay, i didn't know that was why those were added, we can punt it for now, we need to parse errors from relay anyways for various other pages so will make a ticket to get that working
@strawberry.type | ||
class PlaygroundChatCompletionMutationMixin: | ||
@strawberry.mutation | ||
async def playground_chat_completion( |
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.
nit: I know lots of other things are named as playground in here, any reason they would not be usable for generating a chat completion outside of the playground? would probably prefer to name it something like generate_chat_completion unless there is a reason we think this won't work outside of hte playground
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.
there are a ton of playground-specific type definitions and I think our codebase has a lot of generic names that aren't actually reusable
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 that makes a ton of sense was just curious but feel free to keep it!
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.
I think this should be chat_completion
since it is part of the GraphQL API. A consumer of the GraphQL API could use this resolver to do a chat completion regardless of whether they are in playground.
|
||
info.context.event_queue.put(SpanInsertEvent(ids=(span.project_id,))) | ||
|
||
return span.finished_chat_completion |
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.
oh interesting is this just called span because it's named that on line 73, feels a bit weird to have the chat completion attached to a span
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 is the way the streaming_llm_span
context manager works atm. it's changed in my open pr
chunks = [] | ||
async for chunk in llm_client.chat_completion_create( | ||
messages=messages, tools=input.tools or [], **invocation_parameters | ||
): | ||
span.add_response_chunk(chunk) | ||
chunks.append(chunk) |
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.
chunks = [] | |
async for chunk in llm_client.chat_completion_create( | |
messages=messages, tools=input.tools or [], **invocation_parameters | |
): | |
span.add_response_chunk(chunk) | |
chunks.append(chunk) | |
async for chunk in llm_client.chat_completion_create( | |
messages=messages, tools=input.tools or [], **invocation_parameters | |
): | |
span.add_response_chunk(chunk) |
@strawberry.mutation | ||
async def playground_chat_completion( | ||
self, info: Info[Context, None], input: ChatCompletionInput | ||
) -> FinishedChatCompletion: |
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.
I think this return type needs to be something like
@strawberry.type
class ChatCompletionText:
content: str
span: Span
@strawberry.type
class ChatCompletionError:
message: str
span: Span # maybe don't need this or definitely optional
@strawberry.type
class ChatCompletionToolCall:
id: str
function: FunctionCall # this is just json i think
span: Span
ChatCompletionMutationPayload: TypeAlias = Annotated[
Union[ChatCompletionText, ChatCompletionToolCall, ChatCompletionError],
strawberry.union("ChatCompletionMutationPayload"),
]
not just the span, if i'm understanding correctly
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.
I agree
class ChatCompletionResult: | ||
content: Optional[str] | ||
tool_calls: List[ChatCompletionToolCall] | ||
span: Span | ||
error_message: Optional[str] |
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.
Any reason to not mke this a union, also do tool_calls need to be Optional?
class ChatCompletionToolCall: | ||
id: str | ||
function: strawberry.scalars.JSON | ||
span: Optional[Span] |
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.
if this is not apart of a union but an optional portion of the result type, it doesn't need the sapn since the span will be on the top level result
resolves #4774
Adds a
playgroundChatCompletion
mutation that returns aFinishedChatCompletion
and accepts the same payload as thechatCompletion
subscriptionFor simplicity, for now it simply reuses the streaming interfaces we've used, collates the results and returns them all at the end