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

New content format #122

Closed
wants to merge 24 commits into from
Closed

New content format #122

wants to merge 24 commits into from

Conversation

rawnly
Copy link
Contributor

@rawnly rawnly commented Nov 14, 2023

What

Includes changes from #115

content of ChatQuery is now ChatContent.

Link to the api spec

Missing

Some tests. tools / tool_calls / content_filter.

Problems

One thing I noticed is that even tho the available format for the content is Array<ChatContent>/String the response received from the api will always be a string.

I tested this branch on a real application and sometimes I have decoding errors, even tho analyzing the response I really can't tell what's wrong.

Affected Areas

Chat related models

@rawnly
Copy link
Contributor Author

rawnly commented Nov 14, 2023

I'd love to hear some feedback about the ChatContent. Swift is not my main language so I might have done something dumb or over-engineered 😅

@ingvarus-bc ingvarus-bc added the enhancement New feature or request label Nov 14, 2023
@ingvarus-bc
Copy link
Contributor

Addressing the issue about decoding errors, can't it be the case with partial json receiving, the issue which was just recently solved here?

Additionally, what do you think about this OpenAI Chat Chunk Object? As it says in docs, it seems to be returning chat completion now, that is why it is just a string.
chat_return

I may be wrong, but it seems intuitive to me that if we are about to receive mixed content, it should be done by streaming methods instead of completions. 🤔

@rawnly
Copy link
Contributor Author

rawnly commented Nov 14, 2023

Addressing the issue about decoding errors, can't it be the case with partial json receiving, the issue which was just recently solved here?

Absolutely I missed that! I'll take another spin to check if this happens again

@ingvarus-bc
Copy link
Contributor

@rawnly I must really highlight your amazing work here, thank you, for your contribution and that you care about the project! ✨

There is a lot going on in code, will take me some time to get into it, what can tell as of now, we definitely need to break down everything that is going on inside ChatQuery.swift into separate files as OpenAI have a lot of things encapsulated in one another in their Chat API. And it is best to preserve everything that is already there as it is, just "deprecate" it upon release.

ps. Do you still have decoding errors after the fix?

@ingvarus-bc ingvarus-bc added the help wanted Extra attention is needed label Nov 14, 2023
@rawnly
Copy link
Contributor Author

rawnly commented Nov 14, 2023

ps. Do you still have decoding errors after the fix?

looks like it's fixed now 🚀

@rawnly
Copy link
Contributor Author

rawnly commented Nov 14, 2023

Btw one thing i noticed is that new models kind of ignore the functions passed to them, maybe it's worth support both for backwards compatibility and mark functions as deprecated(?)

@ingvarus-bc
Copy link
Contributor

Btw one thing i noticed is that new models kind of ignore the functions passed to them, maybe it's worth support both for backwards compatibility and mark functions as deprecated(?)

Yes, totally agree, that what I was suggesting, here:

There is a lot going on in code, will take me some time to get into it, what can tell as of now, we definitely need to break down everything that is going on inside ChatQuery.swift into separate files as OpenAI have a lot of things encapsulated in one another in their Chat API. And it is best to preserve everything that is already there as it is, just "deprecate" it upon release.

@rawnly rawnly requested a review from ingvarus-bc November 14, 2023 13:08
@rawnly
Copy link
Contributor Author

rawnly commented Nov 14, 2023

Now it should be more clear where things are and it should be easier to manage new models too

@rawnly
Copy link
Contributor Author

rawnly commented Nov 14, 2023

Regarding tools etc, what do you think about smth like

    Tool(
        type: .function,
        value: .function(
            .init( // `Tool.Function`
                name: "get_weather",
                description: "Get the weather" 
            )
        )
    )

parameters is still JSONObject but now i added an .empty static let to allow no params (which is the default value in Tool.Function constructor

@ingvarus-bc
Copy link
Contributor

ingvarus-bc commented Nov 14, 2023

Regarding tools etc, what do you think about smth like

    Tool(
        type: .function,
        value: .function(
            Tool.Function(
                name: "get_weather",
                description: "Get the weather" 
            )
        )
    )

parameters is still JSONObject but now i added an .empty static let to allow no params (which is the default value in Tool.Function constructor

Is this the one from message/tool_calls/ ?
image

Seems pretty nice and logical to me 👍

@rawnly
Copy link
Contributor Author

rawnly commented Nov 14, 2023

nop it's the one from here https://platform.openai.com/docs/api-reference/chat/create#chat-create-tools

@ingvarus-bc
Copy link
Contributor

nop it's the one from here https://platform.openai.com/docs/api-reference/chat/create#chat-create-tools

Got you, yeah, cool. There is one additional field for parameters which function accepts 🤔
I am wondering if we can reuse the same tools in messages as well

@rawnly
Copy link
Contributor Author

rawnly commented Nov 14, 2023

I am wondering if we can reuse the same tools in messages as well

let me push this code so you can look at the model code :)

still missing tests

Signed-off-by: rawnly <[email protected]>
Sources/OpenAI/Public/Models/Chat/Chat.swift Outdated Show resolved Hide resolved
case functionCall = "function_call"
}

public init(role: Role, content codable: Codable? = nil ,name: String? = nil, functionCall: ChatFunctionCall? = nil) {
Copy link
Contributor

@ingvarus-bc ingvarus-bc Nov 14, 2023

Choose a reason for hiding this comment

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

What do you think about typealiasing this Codable? to something like StringOrChatContent(as a suggestion, maybe you'll come up with something better), because I am testing it now and it is not very clear what to pass there 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutely nice idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe also throwing an error if passing the wrong codable?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that would be neat 🔥

@rawnly
Copy link
Contributor Author

rawnly commented Nov 17, 2023

What do you think of adding one more level of abstraction for chats method, it is the most complex and the most customisable one and there are a lot of cases when incorrect setting will cause an error for sure

what are you thinking about?

@ingvarus-bc
Copy link
Contributor

what are you thinking about?

I have two things in mind:

  1. A couple of protocol level methods which will build the Chat query according to the settings and properties provided, to be mindful about the errors which we can predict for sure.
  2. Or we can add some error handling and fallbacks inside of ChatQuery constructor.

Why I think about it, is because OpenAI documentation for chats is clearly not very straightforward and doesn't cover most of the cases, so devs who will be just figuring it out may have a lot of struggle with these settings and properties.

What do you think?

@ingvarus-bc
Copy link
Contributor

@rawnly, hey, we are on our way to make a release tag today, do you have some work in progress or is it okay if I push a couple of things here? ✨

@rawnly
Copy link
Contributor Author

rawnly commented Nov 17, 2023

@rawnly, hey, we are on our way to make a release tag today, do you have some work in progress or is it okay if I push a couple of things here? ✨

Go! I don't have anything wip atm

@rawnly
Copy link
Contributor Author

rawnly commented Nov 17, 2023

What do you think ?

makes sense

A couple of protocol level methods which will build the Chat query according to the settings and properties provided, to be mindful about the errors which we can predict for sure

you mean something like a builder?

@ingvarus-bc
Copy link
Contributor

ingvarus-bc commented Nov 17, 2023

you mean something like a builder?

yep, sort of 👍
I am gonna push slight refactoring and will deprecate completions (as they're 'legacy' now and chats fully cover their functionality) and will merge the PR

And as of the builder, I think it will be a good addition in the next patch

may I be missing something? what do you think, have you encountered any issues in scope of this PR?🤔

@rawnly
Copy link
Contributor Author

rawnly commented Nov 17, 2023

you mean something like a builder?

yep, sort of 👍 I am gonna push slight refactoring and will deprecate completions (as they're 'legacy' now and chats fully cover their functionality) and will merge the PR

And as of the builder, I think it will be a good addition in the next patch

may I be missing something? what do you think, have you encountered any issues in scope of this PR?🤔

lgtm can you test the tools in a real request? I didn't have the time and now i'm not at home 👀

Copy link

sonarcloud bot commented Nov 19, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 21 Code Smells

No Coverage information No Coverage information
3.6% 3.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@rawnly
Copy link
Contributor Author

rawnly commented Nov 19, 2023

Current problem is that according to the api spec id and type are required. But when streaming they can be optional. But we need type to decode the function value :/

Below a stream response returned from the api:
pretty response here

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"role":"assistant","content":null,"tool_calls":[{"index":0,"id":"call_S1iPLro8pNoUH2WOqYvQt6aD","type":"function","function":{"name":"add_to_calendar","arguments":""}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"{\n"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" "}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" \""}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"start"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"\":"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" \""}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"202"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"3"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"-"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"11"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"-"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"20"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"T"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"00"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":":"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"00"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":":"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"00"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"Z"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"\",\n"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" "}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" \""}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"title"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"\":"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" \""}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"F"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"eder"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"ico"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"'s"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" Birthday"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"\",\n"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" "}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" \""}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"end"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"\":"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" \""}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"202"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"3"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"-"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"11"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"-"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"21"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"T"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"00"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":":"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"00"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":":"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"00"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"Z"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"\"\n"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"}"}}]},"finish_reason":null}]}

{"id":"chatcmpl-8MYjk8CJDQT2OhPpeaaTuUAoEFeBI","object":"chat.completion.chunk","created":1700387436,"model":"gpt-4-1106-preview","system_fingerprint":"fp_a24b4d720c","choices":[{"index":0,"delta":{},"finish_reason":"tool_calls"}]}

[DONE]

@ingvarus-bc
Copy link
Contributor

Current problem is that according to the api spec id and type are required. But when streaming they can be optional. But we need type to decode the function value :/

First what comes to mind with this problem is trying to decode chunk and in case of decoding error to decode a completion result. I wonder if it is actually possible to ask gpt for what kind of result we want. There is stream parameter, isn't it responsible for that?
image

@bcherry
Copy link

bcherry commented Dec 18, 2023

I'm using this PR in a project and I found that if you pass a single instance of ChatContent, rather than array of [ChatContent], it will just set the content to nil and fail only at request time with a cryptic "content and role must not be nil" message. The ergonomics of this seem problematic to me, since the swift type checker is unable to help. Maybe at least allowing a single instance of ChatContent to be passed, and then converted to an array, would be good. Otherwise maybe best to not offer an initializer that accepts StringOrChatContent (aka any Codable), so the user isn't exposed to the API variability under the hood. They would either use the explicit string-based or explicit array-of-content-based initializer.

self.content = content
self.name = name
self.functionCall = functionCall
self.toolCalls = []
Copy link

Choose a reason for hiding this comment

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

this initializer is a trap for vision requests - you'll get an invalid request error from sending an empty array in toolCalls (which isn't supported for this model yet). You need to use the other initializer with explicit toolCalls: nil.

Would it be safe to default to nil here?


import Foundation

public struct Message: Codable, Equatable {
Copy link

Choose a reason for hiding this comment

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

This is a pretty generic name for a type - I found it to be a problem in my project that already had its own Message type defined, and OpenAI.Message didn't work because the OpenAI module also contains a class called OpenAI (see this discussion on this potential flaw in Swift)

The only thing that worked for me was renaming the type in my own project so Message was available for this package to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, I think it would be a good idea to use some naming pattern, to avoid confusions, though this would mean that all projects using this package would need to update namings.

}

public struct ToolCall: Codable, Equatable {
public let index: Int

Choose a reason for hiding this comment

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

I'm getting decoding errors because index is not present in the response.

▿ DecodingError
  ▿ keyNotFound : 2 elements
    - .0 : CodingKeys(stringValue: "index", intValue: nil)
    ▿ .1 : Context
      ▿ codingPath : 5 elements
        - 0 : CodingKeys(stringValue: "choices", intValue: nil)
        ▿ 1 : _JSONKey(stringValue: "Index 0", intValue: 0)
          ▿ rep : Rep
            - index : 0
        - 2 : CodingKeys(stringValue: "message", intValue: nil)
        - 3 : CodingKeys(stringValue: "tool_calls", intValue: nil)
        ▿ 4 : _JSONKey(stringValue: "Index 0", intValue: 0)
          ▿ rep : Rep
            - index : 0
      - debugDescription : "No value associated with key CodingKeys(stringValue: \"index\", intValue: nil) (\"index\")."
      - underlyingError : nil

@oxy9en
Copy link

oxy9en commented Jan 31, 2024

Hi Guys. Do you have any updates about this PR?

@kalafus
Copy link
Contributor

kalafus commented Feb 15, 2024

@oxy9en , main is in sync with api specification as of pull (excepting that Edits endpoint is obsolete, betas not implemented).

@rawnly rawnly closed this Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants