-
Notifications
You must be signed in to change notification settings - Fork 23
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
Extract ChatCompletionMessage type #19
Extract ChatCompletionMessage type #19
Conversation
type JSONSchema = | ||
& ( | ||
| ObjectSchema | ||
| StringSchema | ||
| NumberSchema | ||
| BooleanSchema | ||
| ArraySchema | ||
) | ||
& { description?: string }; |
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.
Small change unrelated to the PR: this is auto-formatting from the vscode deno formatter.
export type ChatCompletionMessage = | ||
| SystemCompletionMessage | ||
| UserCompletionMessage | ||
| FunctionAwareAssistantCompletionMessage | ||
| FunctionCompletionMessage | ||
| AssistantCompletionMessage; |
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.
Here I'm summing both the "functional" version of the API and the "regular". That means having both a FunctionAwareAssistantCompletionMessage
and a AssistantCompletionMessage
which is a little strange.
I thought about doing something more sophisticated (i.e. breaking out a type for "functional" models and altering the options here dynamically) but it started to head down a generic heavy path – which, in my opinion, complicated the typing more than it's worth.
interface FunctionCompletionMessage { | ||
content: string; | ||
role: "function"; | ||
name: string; |
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.
Are we sure name can only show up here? If yes, LGTM! Thanks!
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.
Good question. The main docs really only refers to name as something used by function, BUT while trying to verify this I stumbled on examples from
the cookbook using message name to further instruct system (Cases 10 and 13).
With that said, in f81bdd8, I marked them as optional again.
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.
Thanks, let me verify that this is still fine and I'll merge.
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
This was mentioned in a comment as part of #17, but the differences between
ChatCompletion
andChatCompletionOptions
make it difficult to exchange sequences of messages with the API (without needing to limit fields passed to OpenAI). This refactor makes it a little easier.Let me know if you have any suggestions or feedback.