-
Notifications
You must be signed in to change notification settings - Fork 115
Add OpenShift AI integration specifications #5662
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 |
| inference-api-put-llama,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-inference-put-llama,, | ||
| inference-api-put-mistral,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-inference-put-mistral,https://www.elastic.co/guide/en/elasticsearch/reference/8.18/infer-service-mistral.html, | ||
| inference-api-put-openai,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-inference-put-openai,https://www.elastic.co/guide/en/elasticsearch/reference/8.18/infer-service-openai.html, | ||
| inference-api-put-openshift-ai,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-inference-put-openshift-ai,, |
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 URL does not exist yet, which will cause some clients' docs builds to fail. Will this page be available soon?
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 believe this will be added by merging of elastic/elasticsearch#138025 PR.
And in order to merge - overall changes to specification must be reviewed. Any change to mentioned PR - will result in changes to this PR.
Please correct me if my understanding is not valid.
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.
You're correct! Just making sure any broken URLs are temporary.
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.
You're correct! Just making sure any broken URLs are temporary.
Sure thing! Thank you for your vigilance.
DonalEvans
left a comment
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 are 5 different request examples given for creating different OpenShift AI endpoints, but only one response example, which doesn't seem to match any of the requests, since it uses "string" for both the service and the inference_id. Would it be possible for the example response to match at least one of the given requests?
If the response is significantly different for each of the task types, then an example response for each would be good, but not mandatory.
# Conflicts: # output/schema/schema.json
…d add response examples for various task types
|
Hi @DonalEvans But still - I have added XML response for OpenShift AI for every request example. That might be good practice to establish for our next integrations if you see that suitable. |
Thanks for this context, I wasn't aware that that was the common practice in the past. Thank you also for adding response examples for each of the task types, I think it improves the usefulness of the docs for users. |
This PR adds changes to specification caused by elastic/elasticsearch#136624
Corresponding PR for elasticsearch repo that has to be merged first:
elastic/elasticsearch#138025
Additional actions