Skip to content

Replace DeploymentState with DeploymentAssignmentState #2942

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

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

svalbuena
Copy link
Contributor

@svalbuena svalbuena commented Sep 26, 2024

Fixes #2941.

While the server uses the AssignmentState enum for both the state and assignment_state fields, the spec defines two different enums (DeploymentState and DeploymentAssignmentState) for it and one of them has not been updated with all the possible values:

  • The assignment_state field is serialized here and it is of type AssignmentState.
  • The state field is serialized here and it is of type AssignmentState too (this is the one I'm raising the issue for).

In this PR I've replaced TrainedModel.DeploymentState with TrainedModel.DeploymentAssignmentState so that the spec is aligned with the server model and added comments to the cases of TrainedModel.DeploymentAssignmentState.

@svalbuena svalbuena requested review from a team as code owners September 26, 2024 08:24
@svalbuena svalbuena force-pushed the svalb/fix_deployement_state branch from d7ddbb2 to dab0a09 Compare September 26, 2024 08:27
@svalbuena
Copy link
Contributor Author

Here is another bug I found in the ml model endpoints, atm this is failing in our production environment as we've built a detector for failing models that re-deploys them but we can't get the config back due to the deserialization error it throws. I'm using the .NET client but I suppose this issue is in all the clients generated from this spec. cc @pquentin @flobernd

Copy link
Contributor

@l-trotta l-trotta left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@flobernd flobernd left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@flobernd flobernd force-pushed the svalb/fix_deployement_state branch from dab0a09 to f77291f Compare September 27, 2024 09:59
@flobernd flobernd merged commit 9332d2b into elastic:main Sep 27, 2024
5 of 6 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 27, 2024
l-trotta pushed a commit that referenced this pull request Sep 27, 2024
(cherry picked from commit 9332d2b)

Co-authored-by: Sergi Valbuena Garrido <[email protected]>
@svalbuena svalbuena deleted the svalb/fix_deployement_state branch September 27, 2024 13:36
@svalbuena
Copy link
Contributor Author

svalbuena commented Sep 27, 2024

Thanks! When do you plan to release a new version of the .NET client? @flobernd

@flobernd
Copy link
Member

@svalbuena Probably early next week 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TrainedModel.DeploymentState missing Failed state
4 participants