-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[Semantic Text UI] Adapt inference endpoint calls to new API spec #183320
[Semantic Text UI] Adapt inference endpoint calls to new API spec #183320
Conversation
We already have several tests that cover the usage of inference_models received from _inference/_all. Should we add additional tests specifically for register_get_route? |
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.
Changes lgtm, didn't test locally. I was wondering though if we should add a test to make sure we catch potential ES api changes like this one that would break in kibana.
I saw this comment after I left the review, I think it would be a good idea to include some here for sure |
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.
LGTM. Tested locally. New inference endpoint that are created from inference flyout are shown in the select inference endpoint dropdown
++ on adding additional tests as well.
Yup. Specifically we should have FTR tests that test these endpoints in isolation, as well as FTR tests that run through the UI. And ideally we should have them for both stateful and Serverless. |
6857738
to
52959f8
Compare
52959f8
to
dc7b41f
Compare
@sphilipse I added some FTR tests for the endpoint. I will the FTR tests that run through the UI in a separate PR. Thank you. |
dc7b41f
to
d0e9e2f
Compare
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.
ML related changes LGTM.
d0e9e2f
to
534e9b1
Compare
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
The inference endpoint API spec has changed. Now, we need to refer to 'endpoints' property instead of the 'models' property while gathering the inference_endpoints informations using _inference/_all