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

Updating inference services API response #118491

Merged
merged 14 commits into from
Jan 8, 2025
Merged

Updating inference services API response #118491

merged 14 commits into from
Jan 8, 2025

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Dec 11, 2024

❗❗This should not be merged until elastic/kibana#203363 is released to serverless

Resolves https://github.com/elastic/ml-team/issues/1428

Summary

Updates the GET _inference/_services API to follow the new agreed upon response format.

For the new updatable field, I followed the guidance from API docs which says you can modify task_settings, secrets (within service_settings), or num_allocations, depending on the specific endpoint service and task_type you’ve created, so the only fields with updatable: true are secret settings field and num_allocations.

@ymao1 ymao1 self-assigned this Dec 16, 2024
@ymao1 ymao1 added :ml Machine learning Team:ML Meta label for the ML team v9.0.0 v8.18.0 >enhancement labels Dec 16, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @ymao1, I've created a changelog YAML for you.

@ymao1 ymao1 marked this pull request as ready for review December 17, 2024 00:17
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I have a couple of suggestions

The output contains lots of entries like "default_value": null,. In SettingsConfiguration#toXContent() please can you wrap writing default_value in a null check.

The service configurations are not sorted as order they are written depends on the iteration order of a map. Sorting the individual configs by service would make the output more readable.

@ymao1
Copy link
Contributor Author

ymao1 commented Jan 2, 2025

The output contains lots of entries like "default_value": null,. In SettingsConfiguration#toXContent() please can you wrap writing default_value in a null check.

Updated in 8e1562d

The service configurations are not sorted as order they are written depends on the iteration order of a map. Sorting the individual configs by service would make the output more readable.

Updated in bc1922d

@ymao1 ymao1 requested a review from YulNaumenko January 7, 2025 13:26
Copy link

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested locally, works as expected.

@ymao1 ymao1 merged commit 82b1f2a into elastic:main Jan 8, 2025
16 checks passed
@ymao1 ymao1 deleted the ml-1428 branch January 8, 2025 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml Machine learning >refactoring Team:ML Meta label for the ML team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants