Skip to content

Conversation

@benwtrent
Copy link
Member

When requesting a model inference, it is helpful to know the following information early:

  1. What type of model the user is expecting
  2. The parameters required for that type of model.

This change creates a new named writable that users will provide when creating an inference processor/making an API request against an existing model.

The design is meant to be extensible, and possibly have new "types" of models in the future (outside of Regression/Classification, looking at you Clustering, NLP weirdness).

Additionally, this cleans up the dependencies between the output aggregation + target type values in the Ensemble model.

@benwtrent benwtrent added >non-issue :ml Machine learning v8.0.0 labels Oct 9, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@benwtrent benwtrent force-pushed the feature/ml-inference-make-params-model-type-specific branch from 3286dfc to 77f11ca Compare October 9, 2019 16:21
Copy link

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

This looks like a good strategy for providing extensibility in the future.

Since these objects could eventually be stored in cluster state via the ingest pipeline definitions I am just wondering if this is opening up the possibility of unparseable cluster state if new types of inference config are added via newer nodes in mixed version clusters.

One possible strategy for defending against this future problem could be to change isTargetTypeSupported(TargetType targetType) to isTargetTypeSupportedAtVersion(TargetType targetType, Version version). When called we'd pass the minimum node version in the cluster to that method rather than the current node version. Then we could defend against people configuring new types of inference model until every node in the cluster was on the required version for that type of model.

Another option might be to simply have a minimumSupportedVersion() method in the InferenceConfig interface and check that at the points where we might get such a config from a user.

Or maybe you already have another plan for defending against mixed version cluster problems.

But I think we need something for this.

@benwtrent
Copy link
Member Author

@droberts195

Thanks for the feedback!

Since these objects could eventually be stored in cluster state via the ingest pipeline definitions I am just wondering if this is opening up the possibility of unparseable cluster state if new types of inference config are added via newer nodes in mixed version clusters.

When the pipeline & processor config is stored in cluster state, it is a simple Map<String, Object>. Consequently, when it is read in, an intermediary step in Processor.Factory#create will have to transform the stored Map<String, Object> into our supported InferenceConfig object.

So, there are no worries about breaking the cluster. The worse thing is that an unsupported node version reads in the config, but fails to create the processor.

Copy link

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

Thanks for the clarifications @benwtrent

LGTM

@benwtrent benwtrent merged commit 33e1880 into elastic:feature/ml-inference Oct 10, 2019
@benwtrent benwtrent deleted the feature/ml-inference-make-params-model-type-specific branch October 10, 2019 11:44
benwtrent added a commit that referenced this pull request Nov 18, 2019
* [ML][Inference] adds lazy model loader and inference (#47410)

This adds a couple of things:

- A model loader service that is accessible via transport calls. This service will load in models and cache them. They will stay loaded until a processor no longer references them
- A Model class and its first sub-class LocalModel. Used to cache model information and run inference.
- Transport action and handler for requests to infer against a local model
Related Feature PRs: 
* [ML][Inference] Adjust inference configuration option API (#47812)

* [ML][Inference] adds logistic_regression output aggregator (#48075)

* [ML][Inference] Adding read/del trained models (#47882)

* [ML][Inference] Adding inference ingest processor (#47859)

* [ML][Inference] fixing classification inference for ensemble (#48463)

* [ML][Inference] Adding model memory estimations (#48323)

* [ML][Inference] adding more options to inference processor (#48545)

* [ML][Inference] handle string values better in feature extraction (#48584)

* [ML][Inference] Adding _stats endpoint for inference (#48492)

* [ML][Inference] add inference processors and trained models to usage (#47869)

* [ML][Inference] add new flag for optionally including model definition (#48718)

* [ML][Inference] adding license checks (#49056)

* [ML][Inference] Adding memory and compute estimates to inference (#48955)
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Nov 18, 2019
* [ML][Inference] adds lazy model loader and inference (elastic#47410)

This adds a couple of things:

- A model loader service that is accessible via transport calls. This service will load in models and cache them. They will stay loaded until a processor no longer references them
- A Model class and its first sub-class LocalModel. Used to cache model information and run inference.
- Transport action and handler for requests to infer against a local model
Related Feature PRs:
* [ML][Inference] Adjust inference configuration option API (elastic#47812)

* [ML][Inference] adds logistic_regression output aggregator (elastic#48075)

* [ML][Inference] Adding read/del trained models (elastic#47882)

* [ML][Inference] Adding inference ingest processor (elastic#47859)

* [ML][Inference] fixing classification inference for ensemble (elastic#48463)

* [ML][Inference] Adding model memory estimations (elastic#48323)

* [ML][Inference] adding more options to inference processor (elastic#48545)

* [ML][Inference] handle string values better in feature extraction (elastic#48584)

* [ML][Inference] Adding _stats endpoint for inference (elastic#48492)

* [ML][Inference] add inference processors and trained models to usage (elastic#47869)

* [ML][Inference] add new flag for optionally including model definition (elastic#48718)

* [ML][Inference] adding license checks (elastic#49056)

* [ML][Inference] Adding memory and compute estimates to inference (elastic#48955)
benwtrent added a commit that referenced this pull request Nov 18, 2019
* [ML] ML Model Inference Ingest Processor (#49052)

* [ML][Inference] adds lazy model loader and inference (#47410)

This adds a couple of things:

- A model loader service that is accessible via transport calls. This service will load in models and cache them. They will stay loaded until a processor no longer references them
- A Model class and its first sub-class LocalModel. Used to cache model information and run inference.
- Transport action and handler for requests to infer against a local model
Related Feature PRs:

* [ML][Inference] Adjust inference configuration option API (#47812)

* [ML][Inference] adds logistic_regression output aggregator (#48075)

* [ML][Inference] Adding read/del trained models (#47882)

* [ML][Inference] Adding inference ingest processor (#47859)

* [ML][Inference] fixing classification inference for ensemble (#48463)

* [ML][Inference] Adding model memory estimations (#48323)

* [ML][Inference] adding more options to inference processor (#48545)

* [ML][Inference] handle string values better in feature extraction (#48584)

* [ML][Inference] Adding _stats endpoint for inference (#48492)

* [ML][Inference] add inference processors and trained models to usage (#47869)

* [ML][Inference] add new flag for optionally including model definition (#48718)

* [ML][Inference] adding license checks (#49056)

* [ML][Inference] Adding memory and compute estimates to inference (#48955)

* fixing version of indexed docs for model inference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants