[ML] add new inference_config field to trained model config #54421
Conversation
|
Pinging @elastic/ml-core (:ml) |
|
@elasticmachine update branch |
…of github.com:benwtrent/elasticsearch into feature/ml-inference-add-infer-config-to-model-config
|
@elasticmachine update branch |
| <5> Optionally, a human-readable description | ||
| <6> Optionally, an object map contain metadata about the model | ||
| <7> Optionally, an array of tags to organize the model | ||
| <8> The default inference config to use with the model. Must match the underlying |
There was a problem hiding this comment.
I think the new object needs to be added in the definitions section too (e.g. https://www.elastic.co/guide/en/elasticsearch/reference/master/put-inference.html#ml-put-inference-trained-model)
davidkyle
left a comment
davidkyle
left a comment
There was a problem hiding this comment.
Great change! LGTM
One request could we now make the inference config for the inference processor optional? i.e. remove the need for
"inference_config": {
"regression": {}
}
| public class ClassificationConfigTests extends AbstractXContentTestCase<ClassificationConfig> { | ||
|
|
||
| public static ClassificationConfig randomClassificationConfig() { | ||
| return new ClassificationConfig(randomBoolean() ? null : randomIntBetween(-1, 10), |
| } | ||
|
|
||
| boolean isNoop(ClassificationConfig originalConfig) { | ||
| return (resultsField == null || originalConfig.getResultsField().equals(resultsField)) |
There was a problem hiding this comment.
Flip the test around so you call equals on the one you've proved to be non-null.
resultsField == null || resultsField.equals(originalConfig.getResultsField())
I know getResultsField() can't return null as it has a default value but it seems sensible to reverse it anyway. Also topClassesResultsField below
| Regression regression = ((Regression)analytics.getAnalysis()); | ||
| return new RegressionConfig(RegressionConfig.DEFAULT_RESULTS_FIELD, | ||
| regression.getBoostedTreeParams().getNumTopFeatureImportanceValues() == null ? | ||
| 0 : |
There was a problem hiding this comment.
Let RegressionConfig pick the default value (it does this in the constructor anyway)
RegressionConfig(RegressionConfig.DEFAULT_RESULTS_FIELD,
regression.getBoostedTreeParams().getNumTopFeatureImportanceValues())
There was a problem hiding this comment.
Same with ClassificationConfig
Part of me is still reticent to do that. It is right now, the only indication of the "inference type". I suppose we can remove it in the future for sure. I don't think we should do it in this PR though. |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
benwtrent
deleted the
feature/ml-inference-add-infer-config-to-model-config
branch
…54421) A new field called `inference_config` is now added to the trained model config object. This new field allows for default inference settings from analytics or some external model builder. The inference processor can still override whatever is set as the default in the trained model config.
…4421) (#54647) * [ML] add new inference_config field to trained model config (#54421) A new field called `inference_config` is now added to the trained model config object. This new field allows for default inference settings from analytics or some external model builder. The inference processor can still override whatever is set as the default in the trained model config. * fixing for backport
5 participants
A new field called
inference_configis now added to the trained model config object. This new field allows for default inference settings from analytics or some external model builder.The inference processor can still override whatever is set as the default in the trained model config.
Docs preview: