Skip to content

[ML] Fix serialising the inference update request #122278

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

Merged
davidkyle merged 2 commits into from
Feb 11, 2025
+92 −1
Merged

[ML] Fix serialising the inference update request #122278

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f2fd52
Fix serialising inference update
davidkyle Feb 11, 2025
2c1d292
Update docs/changelog/122278.yaml
davidkyle Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...c/main/java/org/elasticsearch/xpack/core/inference/action/UpdateInferenceModelAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ public Request(String inferenceEntityId, BytesReference content, XContentType co
public Request(StreamInput in) throws IOException {
super(in);
this.inferenceEntityId = in.readString();
this.content = in.readBytesReference();
this.taskType = TaskType.fromStream(in);
this.content = in.readBytesReference();
Copy link
Member Author

@davidkyle davidkyle Feb 11, 2025

Choose a reason for hiding this comment

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

Surprisingly the fix doesn't require a new transport version. This change means that reading from older nodes will start working and reads from same version and future versions will work

Copy link
Member

@kderusso kderusso Feb 11, 2025

Choose a reason for hiding this comment

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

Making sure I understand, we don't need a new transport version because it was incorrect to start? Will this have any expected impact on serverless?

Copy link
Member Author

@davidkyle davidkyle Feb 11, 2025

Choose a reason for hiding this comment

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

The writeTo() code writes taskType then content and the read was reading content then taskType.
My intuition was that a fix would need a new transport version but when I wrote the code it ended up as:

if (version.before(MY_FIX_VERSION)) {
  // change the order as this is what write is doing in the old version
  this.taskType = TaskType.fromStream(in);
  this.content = in.readBytesReference();
} else {
  // change the order as this is what write is doing in the current version
  this.taskType = TaskType.fromStream(in);
  this.content = in.readBytesReference();
}

Both sides of the condition are the same so I took out the transport version check, that suprised me as things are rarely this simple. As for serverless, what was once broken should start working.

this.contentType = in.readEnum(XContentType.class);
}

Expand Down
47 changes: 47 additions & 0 deletions ...java/org/elasticsearch/xpack/inference/action/UpdateInferenceModelActionRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.inference.action;

import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.inference.TaskType;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xpack.core.inference.action.UpdateInferenceModelAction;
import org.elasticsearch.xpack.inference.InferenceNamedWriteablesProvider;

import java.io.IOException;

public class UpdateInferenceModelActionRequestTests extends AbstractWireSerializingTestCase<UpdateInferenceModelAction.Request> {
Copy link
Member

@kderusso kderusso Feb 11, 2025

Choose a reason for hiding this comment

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

Would we want to use AbstractBWCSerializationTestCase here?

Copy link
Member Author

@davidkyle davidkyle Feb 11, 2025

Choose a reason for hiding this comment

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

Because there is on BWC logic on the transport version in the serialisation code the mutateInstanceForVersion() function would have just return the input parameter. I thought it cleaner use the plain AbstractWireSerializingTestCase base class.


@Override
protected Writeable.Reader<UpdateInferenceModelAction.Request> instanceReader() {
return UpdateInferenceModelAction.Request::new;
}

@Override
protected UpdateInferenceModelAction.Request createTestInstance() {
return new UpdateInferenceModelAction.Request(
randomAlphaOfLength(5),
randomBytesReference(50),
randomFrom(XContentType.values()),
randomFrom(TaskType.values()),
randomTimeValue()
);
}

@Override
protected UpdateInferenceModelAction.Request mutateInstance(UpdateInferenceModelAction.Request instance) throws IOException {
return randomValueOtherThan(instance, this::createTestInstance);
}

@Override
protected NamedWriteableRegistry getNamedWriteableRegistry() {
return new NamedWriteableRegistry(InferenceNamedWriteablesProvider.getNamedWriteables());
}
}
39 changes: 39 additions & 0 deletions ...ava/org/elasticsearch/xpack/inference/action/UpdateInferenceModelActionResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.inference.action;

import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xpack.core.inference.action.UpdateInferenceModelAction;
import org.elasticsearch.xpack.inference.InferenceNamedWriteablesProvider;
import org.elasticsearch.xpack.inference.ModelConfigurationsTests;

import java.io.IOException;

public class UpdateInferenceModelActionResponseTests extends AbstractWireSerializingTestCase<UpdateInferenceModelAction.Response> {
@Override
protected Writeable.Reader<UpdateInferenceModelAction.Response> instanceReader() {
return UpdateInferenceModelAction.Response::new;
}

@Override
protected UpdateInferenceModelAction.Response createTestInstance() {
return new UpdateInferenceModelAction.Response(ModelConfigurationsTests.createRandomInstance());
}

@Override
protected UpdateInferenceModelAction.Response mutateInstance(UpdateInferenceModelAction.Response instance) throws IOException {
return randomValueOtherThan(instance, this::createTestInstance);
}

@Override
protected NamedWriteableRegistry getNamedWriteableRegistry() {
return new NamedWriteableRegistry(InferenceNamedWriteablesProvider.getNamedWriteables());
}
}