Skip to content

[ML] fixing bwc for trained model deployment inference #88289

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

Conversation

benwtrent
Copy link
Member

When the internal objects were renamed, an inappropriate bwc version restriction was put in place.

This commit fixes this by allowing trained model assignment metadata updates to be serialized to nodes > 8.0.0.

This is OK as the object serialization handles its BWC conditions when serializing over the wire.

This closes: #87959

@benwtrent benwtrent added >bug >test Issues or PRs that are addressing/adding tests :ml Machine learning test-full-bwc Trigger full BWC version matrix tests labels Jul 5, 2022
@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Jul 5, 2022
@elasticmachine
Copy link
Collaborator

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

return Version.V_8_3_0;
return Version.V_8_0_0;
Copy link
Member Author

@benwtrent benwtrent Jul 5, 2022

Choose a reason for hiding this comment

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

This prevents the object from being serialized to older nodes. Since we don't update the object metadata custom name until all nodes are upgraded now, it is OK to serialize to older nodes.

assertInfer(modelId);
assertInfer(modelId);
// attempt inference on new and old nodes multiple times
for (int i = 0; i < 100; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to keep the 100 inferences here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dimitris-athanasiou we do not, I will reduce it to 10.

Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

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

LGTM Should we backport this to 8.3 branch too?

@benwtrent
Copy link
Member Author

LGTM Should we backport this to 8.3 branch too?

Did we backport the "don't rename until all nodes are upgraded" change to 8.3?

@dimitris-athanasiou
Copy link
Contributor

Did we backport the "don't rename until all nodes are upgraded" change to 8.3?

Yes, in #87822.

@benwtrent benwtrent added v8.3.3 auto-backport Automatically create backport pull requests when merged labels Jul 6, 2022
@benwtrent
Copy link
Member Author

@elasticmachine update branch

@benwtrent benwtrent merged commit 37c17cb into elastic:master Jul 6, 2022
@benwtrent benwtrent deleted the feature/ml-fix-deployment-upgrader-test branch July 6, 2022 14:20
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.3

benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Jul 6, 2022
When the internal objects were renamed, an inappropriate bwc version restriction was put in place.

This commit fixes this by allowing trained model assignment metadata updates to be serialized to nodes > 8.0.0.

This is OK as the object serialization handles its BWC conditions when serializing over the wire.

This closes: elastic#87959
benwtrent added a commit that referenced this pull request Jul 8, 2022
When the internal objects were renamed, an inappropriate bwc version restriction was put in place.

This commit fixes this by allowing trained model assignment metadata updates to be serialized to nodes > 8.0.0.

This is OK as the object serialization handles its BWC conditions when serializing over the wire.

This closes: #87959
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >bug :ml Machine learning Team:ML Meta label for the ML team >test Issues or PRs that are addressing/adding tests test-full-bwc Trigger full BWC version matrix tests v8.3.3 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] MLModelDeploymentsUpgradeIT testTrainedModelDeployment failing
4 participants