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

Chore Support base64 embedding format #485

Merged

Conversation

henomis
Copy link
Contributor

@henomis henomis commented Sep 2, 2023

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:

package main

import (
	"context"
	"encoding/json"
	"fmt"
	"os"

	"github.com/sashabaranov/go-openai"
)

func main() {

	client := openai.NewClient(os.Getenv("OPENAI_API_KEY"))

	res, err := client.CreateEmbeddings(
		context.Background(),
		openai.EmbeddingRequest{
			Input:          "Your text string goes here",
			Model:          openai.AdaEmbeddingV2,
			EncodingFormat: openai.EmbeddingEncodingFormatBase64,
		},
	)
	if err != nil {
		panic(err)
	}

	data, err := json.MarshalIndent(res, "", "  ")
	if err != nil {
		panic(err)
	}

	fmt.Println(string(data))
}

The following example is provided to compare the result using the python implementation:

import openai
import os

openai.api_key = os.getenv("OPENAI_API_KEY")

text = "Your text string goes here"

response = openai.Embedding.create(
    model="text-embedding-ada-002", 
    input=text, 
    kwargs={"encoding_format": "base64"},
    )
print(response)

@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #485 (e36e114) into master (25da859) will increase coverage by 0.13%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            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              
Files Changed Coverage Δ
embeddings.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

embeddings_test.go Outdated Show resolved Hide resolved

if baseReq.EncodingFormat == EmbeddingEncodingFormatBase64 {
res, err = embeddingResponse.(*EmbeddingResponseBase64).ToEmbeddingResponse()
}
Copy link
Owner

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?

Copy link
Contributor Author

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

Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Owner

@sashabaranov sashabaranov Sep 7, 2023

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
}

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you so much!

@@ -108,6 +109,15 @@ func TestEmbeddingEndpoint(t *testing.T) {
_, err := client.CreateEmbeddings(context.Background(), EmbeddingRequest{})
Copy link
Owner

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?

Copy link
Owner

@sashabaranov sashabaranov left a 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 henomis requested a review from sashabaranov September 5, 2023 16:30
@sashabaranov
Copy link
Owner

@henomis seems like the test coverage is failing now

@henomis
Copy link
Contributor Author

henomis commented Sep 10, 2023

It's the coverage for this piece of code

err = c.sendRequest(req, base64Response)
if err != nil {
	return
}

in my first implementation c.sendRequest() was the last instruction of the function (like on the other tests files where sendRequest has been used) and the coverage was ok.

@henomis
Copy link
Contributor Author

henomis commented Sep 10, 2023

Ok, inserted a test case to make sendRequest fail

@sashabaranov
Copy link
Owner

@henomis thank you so much!

@sashabaranov sashabaranov merged commit 8e4b796 into sashabaranov:master Sep 11, 2023
3 checks passed
mudler referenced this pull request in mudler/LocalAI Sep 13, 2023
…#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
[@&#8203;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>
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.

2 participants