-
Notifications
You must be signed in to change notification settings - Fork 115
Add Nvidia inference specification #5794
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
Conversation
|
Following you can find the validation changes against the target branch for the API.
You can validate this API yourself by using the |
# Conflicts: # output/openapi/elasticsearch-openapi.json # output/openapi/elasticsearch-serverless-openapi.json # output/schema/schema.json
package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@redocly/cli": "^1.34.5" | ||
| "@redocly/cli": "^1.34.6" |
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 don't think this should be getting changed 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.
This is done by make setup & make contrib execution. It is mandatory to execute that before merging. Changing it to ^1.34.5 results in it going back to ^1.34.6 as soon as make setup is called.
| "rerank", | ||
| "text_embedding", | ||
| "completion", | ||
| "chat_completion" |
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.
Nitpick, but could these be in alphabetical order?
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.
Done.
| */ | ||
| model_id: string | ||
| /** | ||
| * For a `text_embedding` task, the maximum number of tokens per input before chunking occurs. |
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 should be "For a `text_embedding` task, the maximum number of tokens per input. Inputs exceeding this value are truncated prior to sending to the Nvidia API."
This is wrong almost everywhere in the docs; there's an issue describing some of the problems with max_input_tokens.
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.
Fixed.
| text_embedding, | ||
| completion, | ||
| chat_completion, | ||
| rerank |
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.
For consistency, could these be in alphabetical order?
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.
Fixed.
| */ | ||
| input_type?: NvidiaInputType | ||
| /** | ||
| * For a `text_embedding` task, the method to handle inputs longer than the maximum token length. |
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.
To help differentiate this from max_input_tokens it might be better to word it like "the method used by the Nvidia model to handle inputs longer than..."
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.
Good thinking. Fixed.
| /** | ||
| * The URL of the Nvidia model endpoint. | ||
| */ |
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.
Would it be helpful to include the default URLs for each task type if url isn't specified?
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.
Good idea. Added.
| text_embedding, | ||
| chat_completion, | ||
| completion, | ||
| rerank |
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.
For consistency, could these be in alphabetical order?
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.
Done.
|
Hi @DonalEvans |
| mistral | ||
| } | ||
|
|
||
| export class NvidiaServiceSettings { |
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.
There should also be a dimensions field documented for the text_embedding task, sorry for missing this in the first review.
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 don't think we should declare the parameters the user can't specify.
According to your suggestion from this comment - dimensions cannot be set during the creation of an endpoint and are not sent to the Nvidia
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.
Urgh, my mistake, sorry for forgetting about that, I've been up to my eyeballs in the Jina text embedding code lately (which does support specifying dimensions) and the context switch tripped me up.
| * * `ingest`: Mapped to Nvidia's `passage` value in request. Used when generating embeddings during indexing. | ||
| * * `search`: Mapped to Nvidia's `query` value in request. Used when generating embeddings during querying. | ||
| * | ||
| * IMPORTANT: If not specified `input_type` field in request to Nvidia endpoint is set as `query` by default. |
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 would be better as "For Nvidia endpoints, if the `input_type` field is not specified, it defaults to `query`."
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.
Fixed.
| */ | ||
| max_input_tokens?: integer | ||
| /** | ||
| * For a `text_embedding` task, the similarity measure. One of cosine, dot_product, l2_norm. |
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.
It would be good to add that if no similarity measure is specified, the default value is dot_product.
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.
Added.
# Conflicts: # output/openapi/elasticsearch-openapi.json # output/openapi/elasticsearch-serverless-openapi.json # output/schema/schema.json
This PR adds changes to specification caused by elastic/elasticsearch#132388
Additional actions