Skip to content

Conversation

@Ccheers
Copy link
Contributor

@Ccheers Ccheers commented Jun 14, 2023

implement #360

@Ccheers
Copy link
Contributor Author

Ccheers commented Jun 14, 2023

@sashabaranov

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #367 (4671367) into master (646989c) will decrease coverage by 0.06%.
The diff coverage is 94.91%.

❗ Current head 4671367 differs from pull request most recent head 3ab422b. Consider uploading reports for the commit 3ab422b to get more accurate results

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
- Coverage   95.22%   95.16%   -0.06%     
==========================================
  Files          17       18       +1     
  Lines         670      786     +116     
==========================================
+ Hits          638      748     +110     
- Misses         22       26       +4     
- Partials       10       12       +2     
Impacted Files Coverage Δ
completion.go 100.00% <ø> (ø)
func_chat.go 94.82% <94.82%> (ø)
chat.go 100.00% <100.00%> (ø)
chat_stream.go 89.28% <100.00%> (ø)

@zbysir
Copy link

zbysir commented Jun 14, 2023

Looking forward to it

@vvatanabe
Copy link
Collaborator

@sashabaranov @Ccheers CreateChatCompletionWithFunctionCall function can aggregate multiple API calls related to "Function calling". Very useful. Excellent! 👍 👍

But go-openai is just a client library for OpenAI API. Should go-openai provide high-level abstraction like CreateCompletionWithFunctionCall function?

It may be difficult to design for abstraction between Azure OpenAI and OpenAI in the future when Azure OpenAI supports "Function calling".

I'm a bit concerned about other additional dependent libraries. Perhaps go-openai values maintenance cost and lightweight and dares to have zero dependencies.

It might be better to first just add some fields for "Function calling" to the CreateChatCompletion function request and response, and then gather user feedback to make a decision.

@zbysir
Copy link

zbysir commented Jun 14, 2023

@sashabaranov @Ccheers CreateChatCompletionWithFunctionCall function can aggregate multiple API calls related to "Function calling". Very useful. Excellent! 👍 👍

But go-openai is just a client library for OpenAI API. Should go-openai provide high-level abstraction like CreateCompletionWithFunctionCall function?

It may be difficult to design for abstraction between Azure OpenAI and OpenAI in the future when Azure OpenAI supports "Function calling".

I'm a bit concerned about other additional dependent libraries. Perhaps go-openai values maintenance cost and lightweight and dares to have zero dependencies.

It might be better to first just add some fields for "Function calling" to the CreateChatCompletion function request and response, and then gather user feedback to make a decision.

I quite agree! The signature of withfunctioncall is not universal enough, so users should decide for themselves.

@ushakov
Copy link

ushakov commented Jun 14, 2023

My 2p: it seems gjson library is only used to extract choices.0.message."+fmt.Sprintf("args_%d", i) field from JSON, which should be relatively easy to work around without introducing the additional dependency

@Ccheers
Copy link
Contributor Author

Ccheers commented Jun 14, 2023

@sashabaranov @Ccheers CreateChatCompletionWithFunctionCall function can aggregate multiple API calls related to "Function calling". Very useful. Excellent! 👍 👍

But go-openai is just a client library for OpenAI API. Should go-openai provide high-level abstraction like CreateCompletionWithFunctionCall function?

It may be difficult to design for abstraction between Azure OpenAI and OpenAI in the future when Azure OpenAI supports "Function calling".

I'm a bit concerned about other additional dependent libraries. Perhaps go-openai values maintenance cost and lightweight and dares to have zero dependencies.

It might be better to first just add some fields for "Function calling" to the CreateChatCompletion function request and response, and then gather user feedback to make a decision.

This is too difficult because the parameters of function_call are unknown to us, and we cannot define them in the Go Struct because it may cause unexpected changes to CreateChatCompletion.

@Ccheers
Copy link
Contributor Author

Ccheers commented Jun 14, 2023

My 2p: it seems gjson library is only used to extract choices.0.message."+fmt.Sprintf("args_%d", i) field from JSON, which should be relatively easy to work around without introducing the additional dependency

It seems that using map[string]interface{} to deconstruct JSON for parameter extraction is also possible at present, but it is relatively more complicated.

@jmacwhyte
Copy link
Contributor

jmacwhyte commented Jun 14, 2023

I agree with the issues raised here, I think this library should be kept very simple.

Although the "function call" information that the API returns is meant to be used to call a function in the client's local environment, that might not be how everyone intends to use it. I would prefer to simply have the ChatCompletionMessage (etc) structs updated to include to support the function call details, and leave it up to the client to decide how they will consume that information.

That said, I do think @Ccheers has done some great work which I will use as reference in my own project!

@Ccheers
Copy link
Contributor Author

Ccheers commented Jun 14, 2023

@sashabaranov @Ccheers CreateChatCompletionWithFunctionCall function can aggregate multiple API calls related to "Function calling". Very useful. Excellent! 👍 👍
But go-openai is just a client library for OpenAI API. Should go-openai provide high-level abstraction like CreateCompletionWithFunctionCall function?
It may be difficult to design for abstraction between Azure OpenAI and OpenAI in the future when Azure OpenAI supports "Function calling".
I'm a bit concerned about other additional dependent libraries. Perhaps go-openai values maintenance cost and lightweight and dares to have zero dependencies.
It might be better to first just add some fields for "Function calling" to the CreateChatCompletion function request and response, and then gather user feedback to make a decision.

I quite agree! The signature of withfunctioncall is not universal enough, so users should decide for themselves.

In the current implementation, JSON deserialization relies on Go Struct. However, the definition of function call seems to contradict our traditional deserialization approach. Therefore, I am considering extracting a separate function to support function call.

@Ccheers
Copy link
Contributor Author

Ccheers commented Jun 14, 2023

I agree with the issues raised here, I think this library should be kept very simple.

Although the "function call" information that the API returns is meant to be used to call a function in the client's local environment, that might not be how everyone intends to use it. I would prefer to simply have the ChatCompletionMessage (etc) structs updated to include to support the function call details, and leave it up to the client to decide how they will consume that information.

That said, I do think @Ccheers has done some great work which I will use as reference in my own project!

I think this is a trade-off. If we consider introducing function_call into the native API, we may need to parse JSON twice in the native API, as discussed above. And not everyone needs function_call.

@Ccheers Ccheers closed this Jun 14, 2023
@Ccheers Ccheers force-pushed the master branch 2 times, most recently from 3a153ef to 7e76a68 Compare June 14, 2023 14:38
@Ccheers Ccheers reopened this Jun 14, 2023
@jmacwhyte
Copy link
Contributor

My vote is to not accept this PR just yet. We already have CreateChatCompletion() as well as CreateChatCompletionStream(), adding CreateChatCompletionWithFunctionCall() is a bit over the top when it's all the same API! I think the new function call feature should be able to fit nicely into the existing ChatCompletion structure (like @vvatanabe said), and I am willing to work on an alternate solution (tomorrow, probably) if no one else gets to it first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants