-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
server : add bad input handling in embeddings #10866
Conversation
// with "content", we only support single prompt | ||
if (!oaicompat && prompt.type() != json::value_t::string) { | ||
res_error(res, format_error_response("\"content\" must be a string", ERROR_TYPE_INVALID_REQUEST)); | ||
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.
I wonder what is the reason to apply this restriction on the "content"
field? Why don't we treat it as an alias to "input"
and allow the same inputs for it? cc @ngxson
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.
Hmm I have no idea why, seems like remnant from the past. It makes more sense to consider content
as an alias of input
as you said.
// create and queue the task | ||
json responses = json::array(); | ||
bool error = false; | ||
{ | ||
std::vector<server_task> tasks; | ||
std::vector<llama_tokens> tokenized_prompts = tokenize_input_prompts(ctx_server.ctx, prompt, /* add_special */ false, true); | ||
for (size_t i = 0; i < tokenized_prompts.size(); i++) { | ||
if (tokenized_prompts[i].size() == 0) { |
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 test should not be here. Also, empty string is still a valid input.
Please remove this check
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.
Hmm ok sorry seems like OAI embedding does not accept empty string either. So this check is relevant, it's just not in the correct place.
Ref: https://platform.openai.com/docs/api-reference/embeddings/create
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.
Should be fixed in my PR and it won't crash if the input is empty. We're now adding BOS token to the sequence.
This patch improves the behavior of the embedding endpoint with bad input.