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.

Sign up for GitHub

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

Jump to bottom

Fix ml.get_trained_models #1479

Closed
wants to merge 7 commits into from
Closed

Fix ml.get_trained_models #1479

wants to merge 7 commits into from

Conversation

philkra
Copy link
Contributor

@philkra philkra commented Feb 15, 2022

as titled.

@philkra philkra requested a review from a team February 15, 2022 16:26
@sethmlarson sethmlarson requested a review from lcawl February 15, 2022 18:12
lcawl
lcawl reviewed Feb 15, 2022
View reviewed changes
Copy link
Contributor

@lcawl lcawl left a comment

Choose a reason for hiding this comment

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

I added some comments, but think it should also be reviewed by @davidkyle

specification/_types/aggregations/pipeline.ts
@@ -162,6 +162,7 @@ export class InferenceConfigContainer {
regression?: RegressionInferenceOptions
/** Classification configuration for inference. */
classification?: ClassificationInferenceOptions
ner?: NerInferenceOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

The inclusion of this ner option here is a bit confusing to me, since IMO it implies NER is supported in the inference bucket aggregation, which AFAIK is not true per the docs https://www.elastic.co/guide/en/elasticsearch/reference/master/search-aggregations-pipeline-inference-bucket-aggregation.html.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these NLP properties are supported in inference processors in the create pipeline APi and in the create trained model API, so iMO it makes more sense to add these in https://github.com/elastic/elasticsearch-specification/blob/main/specification/ingest/_types/Processors.ts or https://github.com/elastic/elasticsearch-specification/blob/main/specification/ml/_types/TrainedModel.ts

Copy link
Member

Choose a reason for hiding this comment

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

Yes the pipeline aggregation does not support any of the NLP features, this option is not valid here.

@lcawl lcawl requested a review from davidkyle February 15, 2022 21:33
lcawl
lcawl reviewed Feb 15, 2022
View reviewed changes
specification/ml/get_trained_models/MlGetTrainedModelRequest.ts Outdated
tags?: string
tags?: string | string[]
/**
* @since 7.17.0
Copy link
Contributor

@lcawl lcawl Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like this was introduced in 7.13.0 and deprecated then too: https://github.com/elastic/elasticsearch/blob/7.13/rest-api-spec/src/main/resources/rest-api-spec/api/ml.get_trained_models.json#L46

Suggested change
* @since 7.17.0
* @since 7.13.0
* @deprecated 7.13.0

Copy link
Member

Choose a reason for hiding this comment

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

The entire API and include_model_definition parameter were first added in 7.6: elastic/elasticsearch#49052

include_model_definition was later deprecated in 7.10 by elastic/elasticsearch#62834

It looks like theJSON spec was not correctly updated at the same time hence the confusion.

@github-actions
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
ml.close_job 🟢 62/62 61/61
ml.delete_calendar_event 🟢 4/4 4/4
ml.delete_calendar_job 🟢 3/3 3/3
ml.delete_calendar 🟢 5/5 5/5
ml.delete_data_frame_analytics 🟢 2/2 2/2
ml.delete_datafeed 🟢 3/3 3/3
ml.delete_expired_data 🟢 5/5 5/5
ml.delete_filter 🟢 27/27 27/27
ml.delete_forecast 🟢 3/3 3/3
ml.delete_job 🟢 47/47 47/47
ml.delete_model_snapshot 🟢 2/2 2/2
ml.delete_trained_model_alias 🟢 3/3 3/3
ml.delete_trained_model 🟢 3/3 3/3
ml.estimate_model_memory 🟢 16/16 16/16
ml.evaluate_data_frame 🟢 22/22 22/22
ml.explain_data_frame_analytics 🟢 7/7 7/7
ml.flush_job 🟢 15/15 15/15
ml.forecast 🟢 1/1 1/1
ml.get_buckets 🟢 14/14 14/14
ml.get_calendar_events 🟢 17/17 17/17
ml.get_calendars 🟢 17/17 17/17
ml.get_categories 🟢 12/12 12/12
ml.get_data_frame_analytics_stats 🟢 12/12 12/12
ml.get_data_frame_analytics 🟢 17/17 17/17
ml.get_datafeed_stats 🟢 27/27 27/27
ml.get_datafeeds 🟢 20/20 20/20
ml.get_filters 🟢 13/13 13/13
ml.get_influencers 🟢 11/11 11/11
ml.get_job_stats 🟢 31/31 31/31
ml.get_jobs 🟢 31/31 31/31
ml.get_model_snapshot_upgrade_stats 🟠 Missing recording Missing recording
ml.get_model_snapshots 🔴 18/18 11/18
ml.get_overall_buckets 🟢 16/16 15/15
ml.get_records 🟢 8/8 8/8
ml.get_trained_models_stats 🟢 12/12 12/12
ml.get_trained_models 🟢 27/27 27/27
ml.infer_trained_model_deployment ⚪ Missing test Missing test
ml.info 🟢 10/10 10/10
ml.open_job 🔴 83/83 9/83
ml.post_calendar_events 🟢 21/21 21/21
ml.post_data 🔴 8/10 13/17
ml.preview_data_frame_analytics 🟢 3/3 3/3
ml.preview_datafeed 🔴 10/16 8/16
ml.put_calendar_job 🔴 11/12 12/12
ml.put_calendar 🟢 135/135 135/135
ml.put_data_frame_analytics 🟢 33/33 33/33
ml.put_datafeed 🔴 70/71 12/71
ml.put_filter 🟢 27/27 27/27
ml.put_job 🔴 0/226 1/224
ml.put_trained_model_alias 🟢 9/9 9/9
ml.put_trained_model_definition_part ⚪ Missing test Missing test
ml.put_trained_model_vocabulary ⚪ Missing test Missing test
ml.put_trained_model 🔴 3/5 5/5
ml.reset_job 🟢 2/2 2/2
ml.revert_model_snapshot 🟢 2/2 2/2
ml.set_upgrade_mode 🟢 6/6 6/6
ml.start_data_frame_analytics 🟢 1/1 1/1
ml.start_datafeed 🟢 24/24 24/24
ml.start_trained_model_deployment ⚪ Missing test Missing test
ml.stop_data_frame_analytics 🟢 5/5 5/5
ml.stop_datafeed 🟢 17/17 17/17
ml.stop_trained_model_deployment ⚪ Missing test Missing test
ml.update_data_frame_analytics 🟢 2/2 2/2
ml.update_datafeed 🔴 6/7 6/7
ml.update_filter 🟢 3/3 3/3
ml.update_job 🔴 3/5 5/5
ml.update_model_snapshot 🟢 3/3 3/3
ml.upgrade_job_snapshot 🟢 3/3 3/3
ml.validate_detector 🟢 2/2 2/2
ml.validate 🟢 3/3 3/3

You can validate these APIs yourself by using the make validate target.

2 similar comments
@github-actions
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
ml.close_job 🟢 62/62 61/61
ml.delete_calendar_event 🟢 4/4 4/4
ml.delete_calendar_job 🟢 3/3 3/3
ml.delete_calendar 🟢 5/5 5/5
ml.delete_data_frame_analytics 🟢 2/2 2/2
ml.delete_datafeed 🟢 3/3 3/3
ml.delete_expired_data 🟢 5/5 5/5
ml.delete_filter 🟢 27/27 27/27
ml.delete_forecast 🟢 3/3 3/3
ml.delete_job 🟢 47/47 47/47
ml.delete_model_snapshot 🟢 2/2 2/2
ml.delete_trained_model_alias 🟢 3/3 3/3
ml.delete_trained_model 🟢 3/3 3/3
ml.estimate_model_memory 🟢 16/16 16/16
ml.evaluate_data_frame 🟢 22/22 22/22
ml.explain_data_frame_analytics 🟢 7/7 7/7
ml.flush_job 🟢 15/15 15/15
ml.forecast 🟢 1/1 1/1
ml.get_buckets 🟢 14/14 14/14
ml.get_calendar_events 🟢 17/17 17/17
ml.get_calendars 🟢 17/17 17/17
ml.get_categories 🟢 12/12 12/12
ml.get_data_frame_analytics_stats 🟢 12/12 12/12
ml.get_data_frame_analytics 🟢 17/17 17/17
ml.get_datafeed_stats 🟢 27/27 27/27
ml.get_datafeeds 🟢 20/20 20/20
ml.get_filters 🟢 13/13 13/13
ml.get_influencers 🟢 11/11 11/11
ml.get_job_stats 🟢 31/31 31/31
ml.get_jobs 🟢 31/31 31/31
ml.get_model_snapshot_upgrade_stats 🟠 Missing recording Missing recording
ml.get_model_snapshots 🔴 18/18 11/18
ml.get_overall_buckets 🟢 16/16 15/15
ml.get_records 🟢 8/8 8/8
ml.get_trained_models_stats 🟢 12/12 12/12
ml.get_trained_models 🟢 27/27 27/27
ml.infer_trained_model_deployment ⚪ Missing test Missing test
ml.info 🟢 10/10 10/10
ml.open_job 🔴 83/83 9/83
ml.post_calendar_events 🟢 21/21 21/21
ml.post_data 🔴 8/10 13/17
ml.preview_data_frame_analytics 🟢 3/3 3/3
ml.preview_datafeed 🔴 10/16 8/16
ml.put_calendar_job 🔴 11/12 12/12
ml.put_calendar 🟢 135/135 135/135
ml.put_data_frame_analytics 🟢 33/33 33/33
ml.put_datafeed 🔴 70/71 12/71
ml.put_filter 🟢 27/27 27/27
ml.put_job 🔴 0/226 1/224
ml.put_trained_model_alias 🟢 9/9 9/9
ml.put_trained_model_definition_part ⚪ Missing test Missing test
ml.put_trained_model_vocabulary ⚪ Missing test Missing test
ml.put_trained_model 🔴 3/5 5/5
ml.reset_job 🟢 2/2 2/2
ml.revert_model_snapshot 🟢 2/2 2/2
ml.set_upgrade_mode 🟢 6/6 6/6
ml.start_data_frame_analytics 🟢 1/1 1/1
ml.start_datafeed 🟢 24/24 24/24
ml.start_trained_model_deployment ⚪ Missing test Missing test
ml.stop_data_frame_analytics 🟢 5/5 5/5
ml.stop_datafeed 🟢 17/17 17/17
ml.stop_trained_model_deployment ⚪ Missing test Missing test
ml.update_data_frame_analytics 🟢 2/2 2/2
ml.update_datafeed 🔴 6/7 6/7
ml.update_filter 🟢 3/3 3/3
ml.update_job 🔴 3/5 5/5
ml.update_model_snapshot 🟢 3/3 3/3
ml.upgrade_job_snapshot 🟢 3/3 3/3
ml.validate_detector 🟢 2/2 2/2
ml.validate 🟢 3/3 3/3

You can validate these APIs yourself by using the make validate target.

@github-actions
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
ml.close_job 🟢 62/62 61/61
ml.delete_calendar_event 🟢 4/4 4/4
ml.delete_calendar_job 🟢 3/3 3/3
ml.delete_calendar 🟢 5/5 5/5
ml.delete_data_frame_analytics 🟢 2/2 2/2
ml.delete_datafeed 🟢 3/3 3/3
ml.delete_expired_data 🟢 5/5 5/5
ml.delete_filter 🟢 27/27 27/27
ml.delete_forecast 🟢 3/3 3/3
ml.delete_job 🟢 47/47 47/47
ml.delete_model_snapshot 🟢 2/2 2/2
ml.delete_trained_model_alias 🟢 3/3 3/3
ml.delete_trained_model 🟢 3/3 3/3
ml.estimate_model_memory 🟢 16/16 16/16
ml.evaluate_data_frame 🟢 22/22 22/22
ml.explain_data_frame_analytics 🟢 7/7 7/7
ml.flush_job 🟢 15/15 15/15
ml.forecast 🟢 1/1 1/1
ml.get_buckets 🟢 14/14 14/14
ml.get_calendar_events 🟢 17/17 17/17
ml.get_calendars 🟢 17/17 17/17
ml.get_categories 🟢 12/12 12/12
ml.get_data_frame_analytics_stats 🟢 12/12 12/12
ml.get_data_frame_analytics 🟢 17/17 17/17
ml.get_datafeed_stats 🟢 27/27 27/27
ml.get_datafeeds 🟢 20/20 20/20
ml.get_filters 🟢 13/13 13/13
ml.get_influencers 🟢 11/11 11/11
ml.get_job_stats 🟢 31/31 31/31
ml.get_jobs 🟢 31/31 31/31
ml.get_model_snapshot_upgrade_stats 🟠 Missing recording Missing recording
ml.get_model_snapshots 🔴 18/18 11/18
ml.get_overall_buckets 🟢 16/16 15/15
ml.get_records 🟢 8/8 8/8
ml.get_trained_models_stats 🟢 12/12 12/12
ml.get_trained_models 🟢 27/27 27/27
ml.infer_trained_model_deployment ⚪ Missing test Missing test
ml.info 🟢 10/10 10/10
ml.open_job 🔴 83/83 9/83
ml.post_calendar_events 🟢 21/21 21/21
ml.post_data 🔴 8/10 13/17
ml.preview_data_frame_analytics 🟢 3/3 3/3
ml.preview_datafeed 🔴 10/16 8/16
ml.put_calendar_job 🔴 11/12 12/12
ml.put_calendar 🟢 135/135 135/135
ml.put_data_frame_analytics 🟢 33/33 33/33
ml.put_datafeed 🔴 70/71 12/71
ml.put_filter 🟢 27/27 27/27
ml.put_job 🔴 0/226 1/224
ml.put_trained_model_alias 🟢 9/9 9/9
ml.put_trained_model_definition_part ⚪ Missing test Missing test
ml.put_trained_model_vocabulary ⚪ Missing test Missing test
ml.put_trained_model 🔴 3/5 5/5
ml.reset_job 🟢 2/2 2/2
ml.revert_model_snapshot 🟢 2/2 2/2
ml.set_upgrade_mode 🟢 6/6 6/6
ml.start_data_frame_analytics 🟢 1/1 1/1
ml.start_datafeed 🟢 24/24 24/24
ml.start_trained_model_deployment ⚪ Missing test Missing test
ml.stop_data_frame_analytics 🟢 5/5 5/5
ml.stop_datafeed 🟢 17/17 17/17
ml.stop_trained_model_deployment ⚪ Missing test Missing test
ml.update_data_frame_analytics 🟢 2/2 2/2
ml.update_datafeed 🔴 6/7 6/7
ml.update_filter 🟢 3/3 3/3
ml.update_job 🔴 3/5 5/5
ml.update_model_snapshot 🟢 3/3 3/3
ml.upgrade_job_snapshot 🟢 3/3 3/3
ml.validate_detector 🟢 2/2 2/2
ml.validate 🟢 3/3 3/3

You can validate these APIs yourself by using the make validate target.

@benwtrent
Copy link
Member

@philkra take a look at: #1600

I think it addresses your issues here.

@JoshMock
Copy link
Member

JoshMock commented Sep 6, 2023

This spec change hasn't been touched in over a year. Can we close?

@davidkyle
Copy link
Member

@JoshMock yes we can close this is hopelessly outdated.

I will review which changes should be carried forward and create a new PR if necessary.

@davidkyle davidkyle closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidkyle davidkyle davidkyle left review comments

@lcawl lcawl lcawl left review comments

Assignees
No one assigned
Labels
backport 7.17 backport 8.0 backport 8.1 specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@philkra @benwtrent @JoshMock @davidkyle @lcawl