-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Chore Support base64 embedding format #485
Chore Support base64 embedding format #485
Conversation
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
==========================================
+ Coverage 97.30% 97.43% +0.13%
==========================================
Files 18 18
Lines 780 820 +40
==========================================
+ Hits 759 799 +40
Misses 15 15
Partials 6 6
|
|
||
if baseReq.EncodingFormat == EmbeddingEncodingFormatBase64 { | ||
res, err = embeddingResponse.(*EmbeddingResponseBase64).ToEmbeddingResponse() | ||
} |
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.
Shouldn't we update res
otherwise?
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, yes! My bad! I'll fix 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.
Thank you for the update! It's a bit hard to figure out res
manipulations here. Could we please get rid of the named res
return 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.
Yeah! It wasn't so clear, but I left it as I found it to minimize the modifications. However, now named returns have been removed.
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.
Ahhh, I didn't realize how much more problematic would it make if err != nil
returns!
Sorry for bothering you so much, feelks like we've stumbled upon a tricky piece here :D I think that the var embeddingResponse any = &response
creates a level of indirection which is not trivial to understand.
What do you think of this approach?
func (c *Client) CreateEmbeddings(
ctx context.Context,
conv EmbeddingRequestConverter
) (res EmbeddingResponse, err error) {
baseReq := conv.Convert()
req, err := c.newRequest(ctx, http.MethodPost, c.fullURL("/embeddings", baseReq.Model.String()), withBody(baseReq))
if err != nil {
return
}
if baseReq.EncodingFormat != EmbeddingEncodingFormatBase64 {
err = c.sendRequest(req, &res)
return
}
base64Response := &EmbeddingResponseBase64{}
err = c.sendRequest(req, base64Response)
if err != nil {
return
}
res, err = base64Response.ToEmbeddingResponse()
return
}
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 was one of my first ideas. Even though it will duplicate the code on calling twice sendRequest
is the one with a good readability.
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.
Code and tests have been refactored.
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.
Thank you so much!
embeddings_test.go
Outdated
@@ -108,6 +109,15 @@ func TestEmbeddingEndpoint(t *testing.T) { | |||
_, err := client.CreateEmbeddings(context.Background(), EmbeddingRequest{}) |
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, this test would not catch res
not being updated, could you please update test server here so it would return non-empty EmbeddingResponse
?
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.
Seems like we have a problem with non-base64 cases here
@henomis seems like the test coverage is failing now |
It's the coverage for this piece of code err = c.sendRequest(req, base64Response)
if err != nil {
return
} in my first implementation |
Ok, inserted a test case to make sendRequest fail |
@henomis thank you so much! |
…#1035) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/sashabaranov/go-openai](https://togithub.com/sashabaranov/go-openai) | require | patch | `v1.15.2` -> `v1.15.3` | --- ### Release Notes <details> <summary>sashabaranov/go-openai (github.com/sashabaranov/go-openai)</summary> ### [`v1.15.3`](https://togithub.com/sashabaranov/go-openai/releases/tag/v1.15.3) [Compare Source](https://togithub.com/sashabaranov/go-openai/compare/v1.15.2...v1.15.3) #### What's Changed - Chore Support base64 embedding format by [@​henomis](https://togithub.com/henomis) in [https://github.com/sashabaranov/go-openai/pull/485](https://togithub.com/sashabaranov/go-openai/pull/485) **Full Changelog**: sashabaranov/go-openai@v1.15.2...v1.15.3 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/go-skynet/LocalAI). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi44My4wIiwidXBkYXRlZEluVmVyIjoiMzYuODMuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Issue: #473
This PR adds the support for base64 embedding format. As discussed in the issue, this format is not officially documented, however it is used in the official python library.
The following go example can be used to test the change:
The following example is provided to compare the result using the python implementation: