-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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][Inference] add inference processors and trained models to usage #47869
[ML][Inference] add inference processors and trained models to usage #47869
Conversation
Pinging @elastic/ml-core (:ml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments about naming and whether the structure is future-proof. I don't know the answers right now - I think this should be a discussion topic for the team.
}) | ||
); | ||
|
||
Map<String, Object> ingestUsage = new HashMap<>(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the default load factor is 0.75, so creating a hash table of size 4 and then putting 4 items in it will result in a resize, negating the benefit of up-front sizing.
|
||
Map<String, Object> ingestUsage = new HashMap<>(4); | ||
ingestUsage.put("pipelines", createCountUsageEntry(pipelines.size())); | ||
ingestUsage.put("docs", docCountStats); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right that this is going to create an object called docs
containing the fields min
, max
and sum
? It would be unclear to me what that meant. Is it docs_processed_per_ingest_node
? That's probably too verbose, but I think the 3 object names need to convey that the values are per node. Maybe we should discuss in a call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly docs_processed
. This is not per_node
. It is a summation across all nodes and only contains data pertaining to the inference
processor.
ingestUsage.put("docs", docCountStats); | ||
ingestUsage.put("time", timeStats); | ||
ingestUsage.put("failures", failureStats); | ||
inferenceUsage.put("processors", Collections.singletonMap(MachineLearningFeatureSetUsage.ALL, ingestUsage)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe spell out more clearly as ingest_processors
. We need to think as well about how these stats will evolve once we allow inference in searches (either as pipeline aggregations or on individual documents retrieved). Would we want to include stats for that? If so what could the names be? And would the current structure need changing if search time stats were added? (I am not saying we need to add these stats now, just that we should think about whether they would fit without changing the structure of the ingest pipeline stats.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ingest_processors
makes sense.
Would we want to include stats for [searches]? If so what could the names be? And would the current structure need changing if search time stats were added?
I am not sure how we would unless we record at search time. Depends on the implementation I suppose. As for the current structure, I don't see any conflict. All these stats are under the processors
(or ingest_processors
) field. Clearly indicating that this is ONLY for ingest stats.
I do agree a larger number of folks should give feedback :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ture/ml-inference-add-to-usage
run elasticsearch-ci/packaging-sample-matrix |
* [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)
* [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)
* [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
This adds inference processor and trained model statistics to the usage endpoint.
I opted for putting trained model counts in a separate hierarchy than the processors. This is to keep our future statistics flexible.
Additionally, I only added the
_all
support, but this should allow us to add more focused statistics around user vs. us provided models, and possibly model types as well.