[ML] More advanced model snapshot retention options #56125

droberts195 · 2020-05-04T16:16:02Z

This PR implements the following changes to make ML model snapshot
retention more flexible in advance of adding a UI for the feature in
an upcoming release.

The default for model_snapshot_retention_days for new jobs is now
10 instead of 1
There is a new job setting, daily_model_snapshot_retention_after_days,
that defaults to 1 for new jobs and model_snapshot_retention_days
for pre-7.8 jobs
For days that are older than model_snapshot_retention_days, all
model snapshots are deleted as before
For days that are in between daily_model_snapshot_retention_after_days
and model_snapshot_retention_days all but the first model snapshot
for that day are deleted
The retain setting of model snapshots is still respected to allow
selected model snapshots to be retained indefinitely

Closes #52150

This PR implements the following changes to make ML model snapshot retention more flexible in advance of adding a UI for the feature in an upcoming release. - The default for `model_snapshot_retention_days` for new jobs is now 10 instead of 1 - There is a new job setting, `daily_model_snapshot_retention_after_days`, that defaults to 1 for new jobs and `model_snapshot_retention_days` for pre-7.8 jobs - For days that are older than `model_snapshot_retention_days`, all model snapshots are deleted as before - For days that are in between `daily_model_snapshot_retention_after_days` and `model_snapshot_retention_days` all but the first model snapshot for that day are deleted - The `retain` setting of model snapshots is still respected to allow selected model snapshots to be retained indefinitely Closes elastic#52150

elasticmachine · 2020-05-04T16:16:04Z

Pinging @elastic/ml-core (:ml)

benwtrent · 2020-05-04T18:24:59Z

docs/reference/ml/ml-shared.asciidoc

+setting was available, the default is the same as that job's
+`model_snapshot_retention_days` setting, to preserve the original behavior
+that no thinning out of model snapshots is done.
+end::daily-model-snapshot-retention-after-days[]


Docs should be updated for tag::model-snapshot-retention-days[] as it says the default value is 1.

I've added that change in a commit.

benwtrent · 2020-05-04T18:32:13Z

...ode-tests/src/test/java/org/elasticsearch/xpack/ml/integration/ModelSnapshotRetentionIT.java

+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
+
+public class ModelSnapshotRetentionIT extends MlNativeAutodetectIntegTestCase {


#nit, do these tests NEED to be the native-multi-node-tests? These take a while to spin-up and run.

They don't NEED to be multi-node tests, but I think they need to be external cluster tests.

I initially tried adding the integration test as an internal cluster test, but that didn't work because delete-by-query didn't work in the internal cluster tests.

We have a single node external cluster test suite too but I am not sure there is a huge performance benefit in moving the test here. Also, over the years we've found numerous bugs in our multi-node clusters caused by differences between requests getting sent directly to the master node and requests getting sent to a non-master node, so I don't think it hurts to have a bit more coverage of inter-node communications.

davidkyle

LGTM

docs/reference/ml/ml-shared.asciidoc

lcawl · 2020-05-04T19:43:30Z

docs/reference/ml/ml-shared.asciidoc

+tag::daily-model-snapshot-retention-after-days[]
+Advanced configuration option. Specifies a number of days between 0 and the
+value of `model_snapshot_retention_days`. After this period of time, only one
+model snapshot per day is retained for this job. Age is calculated relative to


Age is calculated relative to the timestamp of the newest model snapshot...

Is this sentence needed? If yes, it's unclear to me what this age affects.

I think something is needed. Not sure how best to phrase it though.

Suppose you have the following model snapshots:

Tuesday 9:04 am

Monday 10:39 am

Sunday 9:03 am

Saturday 10:46 pm

Friday 8:02 pm

Friday 7:49 pm

Friday 8:59 am

Thursday 9:04 am

And suppose model_snapshot_retention_days is 4 and daily_model_snapshot_retention_after_days is 2.

Then we will delete all snapshots older than Friday 9:04 am (= Tuesday 9:04 am - 4 days). So we will delete snapshots 7 and 8 by this rule. And for snapshots at or after Friday 9:04 am (= Tuesday 9:04 am - 4 days) but before Sunday 9:04 am (= Tuesday 9:04 am - 2 days) we will keep the first per 24 hour period. So we will keep snapshots 6 and 4 by this rule and delete snapshots 5 and 3.

You can see that this is all highly dependent on the timestamp of the latest model snapshot, because everything is measured relative to its timestamp. So this is what "Age is calculated relative to the timestamp of the newest model snapshot" was trying to say. Can you recommend a good way to explain it? I will probably do this docs change in a followup PR though if the PR build goes green soon as I need to get the main code change merged before the 7.8 branch is split.

I've created elastic/stack-docs#1047 to update the Model snapshots (https://www.elastic.co/guide/en/machine-learning/current/ml-model-snapshots.html) page and can revisit the API details thereafter.

Thanks Lisa!

Co-authored-by: Lisa Cawley <[email protected]>

dimitris-athanasiou · 2020-05-05T09:30:31Z

...ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/AbstractExpiredJobDataRemover.java

@@ -84,15 +87,15 @@ private WrappedBatchedJobsIterator newJobIterator() {
        return new WrappedBatchedJobsIterator(jobsIterator);
    }

-    abstract void calcCutoffEpochMs(String jobId, long retentionDays, ActionListener<Long> listener);
+    abstract void calcCutoffEpochMs(String jobId, long retentionDays, ActionListener<Tuple<Long, Long>> listener);


It might be worth replacing the tuple with a struct-like class that better explains what each value is.

dimitris-athanasiou · 2020-05-05T10:39:32Z

...ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/AbstractExpiredJobDataRemover.java

+     */
+    protected static final class CutoffDetails {
+
+        public long latestTimeMs;


Make those two members final?

dimitris-athanasiou

LGTM

This PR implements the following changes to make ML model snapshot retention more flexible in advance of adding a UI for the feature in an upcoming release. - The default for `model_snapshot_retention_days` for new jobs is now 10 instead of 1 - There is a new job setting, `daily_model_snapshot_retention_after_days`, that defaults to 1 for new jobs and `model_snapshot_retention_days` for pre-7.8 jobs - For days that are older than `model_snapshot_retention_days`, all model snapshots are deleted as before - For days that are in between `daily_model_snapshot_retention_after_days` and `model_snapshot_retention_days` all but the first model snapshot for that day are deleted - The `retain` setting of model snapshots is still respected to allow selected model snapshots to be retained indefinitely Backport of elastic#56125

This PR implements the following changes to make ML model snapshot retention more flexible in advance of adding a UI for the feature in an upcoming release. - The default for `model_snapshot_retention_days` for new jobs is now 10 instead of 1 - There is a new job setting, `daily_model_snapshot_retention_after_days`, that defaults to 1 for new jobs and `model_snapshot_retention_days` for pre-7.8 jobs - For days that are older than `model_snapshot_retention_days`, all model snapshots are deleted as before - For days that are in between `daily_model_snapshot_retention_after_days` and `model_snapshot_retention_days` all but the first model snapshot for that day are deleted - The `retain` setting of model snapshots is still respected to allow selected model snapshots to be retained indefinitely Backport of #56125

Relates: elastic/elasticsearch#56125, #4803 This commit adds the DailyModelSnapshotRetentionAfterDays to ML jobs. It also updates the XML comment for ModelSnapshotRetentionDays to align with the new default in Elasticsearch 7.8.

Relates: elastic/elasticsearch#56125, #4803 This commit adds the DailyModelSnapshotRetentionAfterDays to ML jobs. It also updates the XML comment for ModelSnapshotRetentionDays to align with the new default in Elasticsearch 7.8. Co-authored-by: Russ Cam <[email protected]>

droberts195 added >enhancement :ml Machine learning v8.0.0 v7.8.0 labels May 4, 2020

benwtrent reviewed May 4, 2020

View reviewed changes

davidkyle approved these changes May 4, 2020

View reviewed changes

[DOCS] Updates retention descriptions

3e7bf0e

lcawl reviewed May 4, 2020

View reviewed changes

docs/reference/ml/ml-shared.asciidoc Outdated Show resolved Hide resolved

lcawl reviewed May 4, 2020

View reviewed changes

droberts195 and others added 3 commits May 5, 2020 09:32

Update docs/reference/ml/ml-shared.asciidoc

ee14fd3

Co-authored-by: Lisa Cawley <[email protected]>

Fixing tests

7951a77

Merge branch 'master' into daily_model_snapshot_retention

394be6d

dimitris-athanasiou reviewed May 5, 2020

View reviewed changes

Use a dedicated class instead of Tuple

3201549

dimitris-athanasiou reviewed May 5, 2020

View reviewed changes

droberts195 added 2 commits May 5, 2020 11:48

Merge branch 'master' into daily_model_snapshot_retention

0bb92d9

Make members final

c3f6a84

dimitris-athanasiou approved these changes May 5, 2020

View reviewed changes

benwtrent approved these changes May 5, 2020

View reviewed changes

droberts195 merged commit c99021c into elastic:master May 5, 2020

droberts195 deleted the daily_model_snapshot_retention branch May 5, 2020 11:55

droberts195 mentioned this pull request May 5, 2020

[7.x][ML] More advanced model snapshot retention options #56194

Merged

tylersmalley mentioned this pull request May 6, 2020

Failing ES Promotion: machine learning anomaly detection single metric job cloning displays the summary step elastic/kibana#65377

Closed

jgowdyelastic mentioned this pull request May 6, 2020

[ML] Adding daily_model_snapshot_retention_after_days to types and schemas elastic/kibana#65417

Merged

1 task

lcawl mentioned this pull request May 8, 2020

[DOCS] Clarify model snapshot retention properties #56477

Merged

davidkyle mentioned this pull request May 27, 2020

[6.8] Fix delete_expired_data/nightly maintenance when many model snapshots need deleting #57174

Merged

russcam mentioned this pull request May 29, 2020

7.8.0 Meta ticket elastic/elasticsearch-net#4718

Closed

17 tasks

russcam mentioned this pull request Jun 23, 2020

7.8.1 Meta ticket elastic/elasticsearch-net#4803

Closed

4 tasks

russcam mentioned this pull request Jun 24, 2020

Add DailyModelSnapshotRetentionAfterDays to ml jobs elastic/elasticsearch-net#4812

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] More advanced model snapshot retention options #56125

[ML] More advanced model snapshot retention options #56125

droberts195 commented May 4, 2020

elasticmachine commented May 4, 2020

benwtrent May 4, 2020

lcawl May 4, 2020

benwtrent May 4, 2020

droberts195 May 5, 2020

davidkyle left a comment

lcawl May 4, 2020

droberts195 May 5, 2020

lcawl May 7, 2020

droberts195 May 7, 2020

dimitris-athanasiou May 5, 2020

dimitris-athanasiou May 5, 2020

dimitris-athanasiou left a comment

[ML] More advanced model snapshot retention options #56125

[ML] More advanced model snapshot retention options #56125

Conversation

droberts195 commented May 4, 2020

elasticmachine commented May 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidkyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitris-athanasiou left a comment

Choose a reason for hiding this comment