[ML] removing old 7.x bwc serialization code #80029
Conversation
|
Pinging @elastic/ml-core (Team:ML) |
| // NOTE: Always rewrite potentially old date histogram intervals. | ||
| // This should occur in 8.x+ but not 7.x. | ||
| // 7.x is BWC with versions that do not support the new date_histogram fields |
There was a problem hiding this comment.
I think the changes in this file should be reverted, because 8.x needs to be able to coexist with 7.16.
There was a problem hiding this comment.
Ah, all of 8.x -> 7.16. Will revert the various places.
| @@ -131,36 +130,14 @@ public DataDescription(String timeFieldName, String timeFormat) { | |||
| } | |||
|
|
|||
| public DataDescription(StreamInput in) throws IOException { | |||
| if (in.getVersion().before(Version.V_8_0_0)) { | |||
There was a problem hiding this comment.
The changes in this file should be reverted too, in case 8.x is talking to 7.16.
| @@ -68,9 +68,6 @@ public static AggProvider createRandomValidAggProvider() { | |||
|
|
|||
| @Override | |||
| protected AggProvider mutateInstanceForVersion(AggProvider instance, Version version) { | |||
| if (version.before(Version.V_8_0_0)) { | |||
There was a problem hiding this comment.
This should be reverted to match the AggProvider class.
| @@ -47,13 +46,8 @@ | |||
| public MlScalingReason(StreamInput in) throws IOException { | |||
| this.waitingAnalyticsJobs = in.readStringList(); | |||
| this.waitingAnomalyJobs = in.readStringList(); | |||
| if (in.getVersion().onOrAfter(Version.V_8_0_0)) { | |||
There was a problem hiding this comment.
It looks like this is genuinely new in 8.0 too.
| @@ -162,21 +162,15 @@ public MlMetadata(StreamInput in) throws IOException { | |||
| this.datafeeds = datafeeds; | |||
| this.groupOrJobLookup = new GroupOrJobLookup(jobs.values()); | |||
| this.upgradeMode = in.readBoolean(); | |||
| if (in.getVersion().onOrAfter(Version.V_8_0_0)) { | |||
There was a problem hiding this comment.
This one should really have been V_7_13_0, so this is actually a bad bug that would mean 8.0.0-alpha1, 8.0.0-alpha2 and 8.0.0-beta1 cannot safely run in a mixed version cluster with 7.16.x if the ML metadata exists and an MlMetadata diff gets bundled up into the same cluster state update as some other update. I guess we never hit that in testing. 😬
I'm wondering if we need a known issue for 8.0.0-beta1 telling people not to attempt a rolling upgrade to it if they are using ML.
There was a problem hiding this comment.
Looking at 7.16, at least its correct there 😌
IDK if we have a list of known issues or not for beta1. If we do, this should definitely be on it.
| @@ -886,11 +879,7 @@ public static LazyModelDefinition fromBase64String(String base64String) { | |||
| } | |||
|
|
|||
| public static LazyModelDefinition fromStreamInput(StreamInput input) throws IOException { | |||
| if (input.getVersion().onOrAfter(Version.V_8_0_0)) { // TODO adjust on backport | |||
There was a problem hiding this comment.
This never was backported, so the check needs to stay here, and in the corresponding writeTo method below. The TODOs should be removed. This functionality was added in the PyTorch work. I think at the time these comments were written we must have been intending to backport to 7.16 but we changed our minds.
There was a problem hiding this comment.
LGTM
Please open a separate bug fix PR against 8.0 that makes the changes to MlMetadata.java in 8.0. Maybe also remove the TODOs from TrainedModelConfig.java in case they cause alarm to somebody reading the 8.0 branch code in the future. The rest of the changes are fine to be master only.
|
This is a follow up to elastic#80029.
This is a follow up to #80029.
This commit removes all the old serialization code to that was added when serializing to nodes with a version before 8.0.0.
It also removes specific tests for those versions and some code only used by older versions.