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

Allow embeddings requests to be tokens or strings #417

Merged
merged 11 commits into from
Jul 5, 2023
Merged

Allow embeddings requests to be tokens or strings #417

merged 11 commits into from
Jul 5, 2023

Conversation

jacksors
Copy link
Contributor

@jacksors jacksors commented Jun 29, 2023

Addresses #240

There may be a cleaner way to do this, but this is the best way I could think of quickly to not cause any breaking changes and to maintain some semblance of type checking for the user (instead of just changing the Input field type in EmbeddingRequest from string to interface{}).

Also modified the test case for it.

Thanks for all of the work you guys do on this project!

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #417 (28c670d) into master (9c99f36) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
+ Coverage   96.95%   97.02%   +0.06%     
==========================================
  Files          17       17              
  Lines         690      705      +15     
==========================================
+ Hits          669      684      +15     
  Misses         15       15              
  Partials        6        6              
Impacted Files Coverage Δ
embeddings.go 100.00% <100.00%> (ø)

embeddings.go Outdated
}

type BaseEmbeddingRequest struct {
Input interface{} `json:"input"`
Copy link
Owner

Choose a reason for hiding this comment

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

Looks good, thank you for the PR! Could you please change interface{} to any?

@vvatanabe
Copy link
Collaborator

@jacksors @sashabaranov

I would also like to make a suggestion.

  • Rename EmbeddingRequestBody to EmbeddingRequestConverter to better reflect its sole responsibility of converting to EmbeddingRequest.
  • Rename BaseEmbeddingRequest to EmbeddingRequest and implemented EmbeddingRequestConverter, enabling developers to directly use EmbeddingRequest.
  • Rename EmbeddingRequest to EmbeddingRequestStrings for naming consistency with EmbeddingRequestTokens.
type EmbeddingRequestConverter interface {
	Convert() EmbeddingRequest
}

type EmbeddingRequest struct {
	Input any            `json:"input"`
	Model EmbeddingModel `json:"model"`
	User  string         `json:"user"`
}

func (r EmbeddingRequest) Convert() EmbeddingRequest {
	return r
}

type EmbeddingRequestStrings struct {
	Input []string       `json:"input"`
	Model EmbeddingModel `json:"model"`
	User  string         `json:"user"`
}

func (r EmbeddingRequestStrings) Convert() EmbeddingRequest {
	return EmbeddingRequest{
		Input: r.Input,
		Model: r.Model,
		User:  r.User,
	}
}

type EmbeddingRequestTokens struct {
	Input [][]int        `json:"input"`
	Model EmbeddingModel `json:"model"`
	User  string         `json:"user"`
}

func (r EmbeddingRequestTokens) Convert() EmbeddingRequest {
	return EmbeddingRequest{
		Input: r.Input,
		Model: r.Model,
		User:  r.User,
	}
}

func (c *Client) CreateEmbeddings(ctx context.Context, conv EmbeddingRequestConverter) (resp EmbeddingResponse, err error) {

embeddings.go Outdated
// https://beta.openai.com/docs/api-reference/embeddings/create
func (c *Client) CreateEmbeddings(ctx context.Context, request EmbeddingRequest) (resp EmbeddingResponse, err error) {
req, err := c.newRequest(ctx, http.MethodPost, c.fullURL("/embeddings", request.Model.String()), withBody(request))
func (c *Client) CreateEmbeddings(ctx context.Context, body EmbeddingRequestBody) (resp EmbeddingResponse, err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be clearer to specify the usable structs in the comments, now that the argument is an interface

@jacksors
Copy link
Contributor Author

jacksors commented Jul 5, 2023

@vvatanabe @sashabaranov

Thanks for the feedback, everything mentioned should be included in my latest commits. Let me know if there's any other suggestions you have.

embeddings.go Outdated Show resolved Hide resolved
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.

3 participants