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

Make user field optional in embedding request #899

Conversation

nagar-ajay
Copy link
Contributor

Describe the change
Added omitempty json tag for the User field in the EmbeddingRequest struct. This field is optional in embedding requests as per OpenAI spec.

Issue: If you don't specify any user in the embedding request, the current spec sends the user as an empty string for which embedding models throw errors.

Provide OpenAI documentation link
https://platform.openai.com/docs/api-reference/embeddings/create#embeddings-create-user

Describe your solution
Added omitempty json tag for the User field in the EmbeddingRequest struct. This will make the User field optional as per openai embeddings spec.

Tests
Tested the above change by sending embeddings requests to the embeddings model with following payload:

curl -k -X 'POST' '/embeddings' \
 -H 'accept: application/json' \
 -H 'Content-Type: application/json' \
 -d '{
      "model": "embedding-model",
      "input": "Hello world",
      "user": "test"
}'

curl -k -X 'POST' '/embeddings' \
 -H 'accept: application/json' \
 -H 'Content-Type: application/json' \
 -d '{
      "model": "embedding-model",
      "input": "Hello world",
      "user": ""
}'

curl -k -X 'POST' '/embeddings' \
 -H 'accept: application/json' \
 -H 'Content-Type: application/json' \
 -d '{
      "model": "embedding-model",
      "input": "Hello world"
}'

@sashabaranov
Copy link
Owner

@nagar-ajay thank you for the PR! Please the latest changes from master here, so the CI will pass

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.86%. Comparing base (774fc9d) to head (df90855).
Report is 73 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
+ Coverage   98.46%   98.86%   +0.40%     
==========================================
  Files          24       26       +2     
  Lines        1364     1761     +397     
==========================================
+ Hits         1343     1741     +398     
+ Misses         15       14       -1     
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@nagar-ajay
Copy link
Contributor Author

Updated the PR.

@sashabaranov sashabaranov merged commit 74ed75f into sashabaranov:master Nov 20, 2024
3 checks passed
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