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

[ML] handle new model metadata stream from native process #59725

Merged

Conversation

benwtrent
Copy link
Member

This adds the serialization handling for the new model_metadata object from the native process.

Right now, this document only stores total_feature_importance information.

It is also not yet currently consumed by any user consumed API.

relates: elastic/ml-cpp#1387

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@@ -2,7 +2,7 @@
"order" : 0,
"version" : ${xpack.ml.version.id},
"index_patterns" : [
".ml-inference-000002"
".ml-inference-000003"
Copy link
Member Author

Choose a reason for hiding this comment

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

Are these templates automatically installed when any new node comes online? Or does the new node have to be a master node?

@dimitris-athanasiou do you know?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aside of the question, I believe the index pattern here should be .ml-inference-* and we shouldn't have to change this each time.

Back to the question, all nodes get the templates installed regardless of whether they're master or not.

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 I am not sure. What if an older node has a specific mapping in mind and tries to write to the index?

A new index would be created with the new mapping (if it didn't already exist) and the new node does not have code to support the new mapping.

Keeping it versioned allows only NEW nodes to reference the new index mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it makes sense.

@davidkyle
Copy link
Member

The test failures are

{"type":"illegal_argument_exception","reason":"Text fields are not optimised for operations that require per-document field data like aggregations and sorting, so these operations are disabled by default. Please use a keyword field instead. Alternatively, set fielddata=true on [model_id]

model_id is missing from the mappings in the template

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.

Looks good, no comments just the test failures to fix

@benwtrent
Copy link
Member Author

run elasticsearch-ci/2

@benwtrent
Copy link
Member Author

run elasticsearch-ci/2

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

@davidkyle davidkyle merged commit b99234b into elastic:master Aug 17, 2020
benwtrent added a commit that referenced this pull request Aug 24, 2020
…61251)

This adds the serialization handling for the new model_metadata object from the native process.

Co-authored-by: Benjamin Trent <ben.w.trent@gmail.com>
@benwtrent benwtrent deleted the feature/ml-add-new-model-metadata-docs branch October 19, 2020 19:34
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.

6 participants