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] add default inference config to lang ident model #65986

Merged

Conversation

benwtrent
Copy link
Member

This adds a default inference_config object to the stored lang ident model.

This doesn't change behavior as there are no default settings set in the config.

Additionally, when serializing over the wire in older versions, we handle bwc.

This commit also slightly changes the error message when a model stored as a resource is attempted to be deleted.

relates: elastic/kibana#85179

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@@ -105,7 +105,7 @@
public static final String MODEL_DEFINITION_NOT_FOUND = "Could not find trained model definition [{0}]";
public static final String MODEL_METADATA_NOT_FOUND = "Could not find trained model metadata {0}";
public static final String INFERENCE_CANNOT_DELETE_MODEL =
"Unable to delete model [{0}]";
"Unable to delete model [{0}] as it is self-contained in the machine learning plugin";
Copy link
Member Author

Choose a reason for hiding this comment

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

@sophiec20 I am open to better suggestions here.

Copy link
Member

Choose a reason for hiding this comment

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

The string INFERENCE_CANNOT_DELETE_MODEL is named generically as if the the message could apply to any model but it is strictly about ml models.

I see it is only used in one place but can you rename to INFERENCE_CANNOT_DELETE_*ML*_MODEL or something pls.

Suggestions:
"... as it is managed by machine learning"

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest Cannot delete built-in model [{0}] as it is required by machine learning

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@benwtrent benwtrent requested a review from davidkyle December 8, 2020 12:52
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

@benwtrent benwtrent merged commit 38a0c33 into elastic:master Dec 8, 2020
@benwtrent benwtrent deleted the feature/ml-add-field-to-lang-ident-model branch December 8, 2020 15:25
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Dec 8, 2020
This adds a default `inference_config` object to the stored lang ident model.

This doesn't change behavior as there are no default settings set in the config.

Additionally, when serializing over the wire in older versions, we handle bwc.

This commit also slightly changes the error message when a model stored as a resource is attempted to be deleted.

relates: elastic/kibana#85179
benwtrent added a commit that referenced this pull request Dec 9, 2020
This adds a default `inference_config` object to the stored lang ident model.

This doesn't change behavior as there are no default settings set in the config.

Additionally, when serializing over the wire in older versions, we handle bwc.

This commit also slightly changes the error message when a model stored as a resource is attempted to be deleted.

relates: elastic/kibana#85179
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.

5 participants